Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-522

In remote JDBC driver, transmit static database properties as a map

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0-incubating
    • Component/s: avatica
    • Labels:
      None

      Description

      A lot of DatabaseMetaData methods are not parameterized, and give the same answer every time. Some examples:

      • allProceduresAreCallable
      • getUserName
      • isReadOnly
      • getDatabaseProductName
      • getDatabaseProductVersion
      • getDriverName
      • getDriverVersion
      • getDriverMajorVersion
      • getDriverMinorVersion
      • getSqlKeywords
      • getNumericFunctions
      • getStringFunctions
      • getSystemFunctions
      • getTimeDateFunctions

      This task would define an enum of properties, add a method to Meta that returns (property, value) map.

      See DatabaseMetaData and Connection for the full list.

      Obsolete the following Meta methods:

      • getSqlKeywords
      • getNumericFunctions
      • getStringFunctions
      • getSystemFunctions
      • getTimeDateFunctions

        Issue Links

          Activity

          Hide
          xhoong Xavier FH Leong added a comment -

          I was looking into implementing getXXFunctions for Avatica, saw this JIRA. Will you be looking into this? Or I can take a stab in getting the properties done.

          Show
          xhoong Xavier FH Leong added a comment - I was looking into implementing getXXFunctions for Avatica, saw this JIRA. Will you be looking into this? Or I can take a stab in getting the properties done.
          Hide
          julianhyde Julian Hyde added a comment -

          I'm not working on this. It would be great if you could provide a patch. Click "start progress" in this issue when you're working on it. (Let me know if you need karma for that.)

          Show
          julianhyde Julian Hyde added a comment - I'm not working on this. It would be great if you could provide a patch. Click "start progress" in this issue when you're working on it. (Let me know if you need karma for that.)
          Hide
          xhoong Xavier FH Leong added a comment -

          Yes, I do need karma to change the status, thanks.

          Show
          xhoong Xavier FH Leong added a comment - Yes, I do need karma to change the status, thanks.
          Hide
          xhoong Xavier FH Leong added a comment -

          Quite some changes, using pull request instead:

          https://github.com/apache/incubator-calcite/pull/77

          Separate test for Avatica and Calcite.

          The following are session properties (non-static and from connection)

          • allProceduresAreCallable
          • getUserName
          • isReadOnly

          The following is driver specific properties (over-writable at Driver implementation)

          • getDatabaseProductName
          • getDatabaseProductVersion
          • getDriverName
          • getDriverVersion
          • getDriverMajorVersion
          • getDriverMinorVersion
          Show
          xhoong Xavier FH Leong added a comment - Quite some changes, using pull request instead: https://github.com/apache/incubator-calcite/pull/77 Separate test for Avatica and Calcite. The following are session properties (non-static and from connection) allProceduresAreCallable getUserName isReadOnly The following is driver specific properties (over-writable at Driver implementation) getDatabaseProductName getDatabaseProductVersion getDriverName getDriverVersion getDriverMajorVersion getDriverMinorVersion
          Hide
          julianhyde Julian Hyde added a comment - - edited

          I was thinking of making a call that has no parameters and returns a Map<Property, Object> of all properties whose value is not equal to the default. (The default is defined in the Property enum, and therefore is available both client and server side.)

          That map can then be cached, so the request only needs to be made once.

          Fine to deprecate methods, but you need to provide a comment when they will be removed. Generally '// will be removed before 2.0'. In this case, since Avatica is still experimental, I think you can just remove them. But just mention that they've been removed in the commit comment. Nick Dimiduk, do you agree?

          Rename PropertyName to DatabaseProperty. ("Name" implies string.)

          I don't think you need class DatabaseProperty. You can just return a map, right? The value in the map should be an Object (albeit a simple one that is JSON-serializable) not a String.

          I don't tend to supply the message argument (e.g. "Fail to get SYSTEM_FUNCTIONS") to assertEquals. In fact I would write

          assertThat(connection.getMetaData().getSystemFunctions(),
              is("DATABASE,IFNULL,USER"));

          but it's a matter of personal preference.

          Show
          julianhyde Julian Hyde added a comment - - edited I was thinking of making a call that has no parameters and returns a Map<Property, Object> of all properties whose value is not equal to the default. (The default is defined in the Property enum, and therefore is available both client and server side.) That map can then be cached, so the request only needs to be made once. Fine to deprecate methods, but you need to provide a comment when they will be removed. Generally '// will be removed before 2.0'. In this case, since Avatica is still experimental, I think you can just remove them. But just mention that they've been removed in the commit comment. Nick Dimiduk , do you agree? Rename PropertyName to DatabaseProperty. ("Name" implies string.) I don't think you need class DatabaseProperty. You can just return a map, right? The value in the map should be an Object (albeit a simple one that is JSON-serializable) not a String. I don't tend to supply the message argument (e.g. "Fail to get SYSTEM_FUNCTIONS") to assertEquals. In fact I would write assertThat(connection.getMetaData().getSystemFunctions(), is( "DATABASE,IFNULL,USER" )); but it's a matter of personal preference.
          Hide
          ndimiduk Nick Dimiduk added a comment -

          Fine to deprecate methods, but you need to provide a comment when they will be removed. Generally '// will be removed before 2.0'. In this case, since Avatica is still experimental, I think you can just remove them. But just mention that they've been removed in the commit comment.

          I don't think we make any promises about RPC API stability yet. I think we should focus on improvements (breaking if necessary) until we have a very broad feature coverage. Maybe the Calcite-2.0 release can define some stable RPC API for rolling upgrades and external projects to depend upon. This means that any non-Java clients should be included in Calcite for now so that they can be tested along with the rest of the release.

          Show
          ndimiduk Nick Dimiduk added a comment - Fine to deprecate methods, but you need to provide a comment when they will be removed. Generally '// will be removed before 2.0'. In this case, since Avatica is still experimental, I think you can just remove them. But just mention that they've been removed in the commit comment. I don't think we make any promises about RPC API stability yet. I think we should focus on improvements (breaking if necessary) until we have a very broad feature coverage. Maybe the Calcite-2.0 release can define some stable RPC API for rolling upgrades and external projects to depend upon. This means that any non-Java clients should be included in Calcite for now so that they can be tested along with the rest of the release.
          Hide
          xhoong Xavier FH Leong added a comment -

          I originally put all the properties in a map during a static block for the ENUM, but I figure that's another data structure to maintain for that 4 properties.

          I'll removed the interface instead of marking them DEPRECATED.

          I also more pro on returning what you ask for on the Avatica side, that's why I break up the request to have a parameter to indicate the ask and return just what you ask. Instead of giving you the whole stack (as you can see, property like SqlKeywords can be lengthy). But caching the response seem to be a good idea.

          I thought the DatabaseProperty class serve as a POJO to encapsulate the payload (may include type in future), map works as well, but map does require more memory footprint and serialized with the same (key,value) pair (not able to extend to include type if needed).

          But if we concluded to transfer 1 time property stack, map serve well.

          Show
          xhoong Xavier FH Leong added a comment - I originally put all the properties in a map during a static block for the ENUM, but I figure that's another data structure to maintain for that 4 properties. I'll removed the interface instead of marking them DEPRECATED. I also more pro on returning what you ask for on the Avatica side, that's why I break up the request to have a parameter to indicate the ask and return just what you ask. Instead of giving you the whole stack (as you can see, property like SqlKeywords can be lengthy). But caching the response seem to be a good idea. I thought the DatabaseProperty class serve as a POJO to encapsulate the payload (may include type in future), map works as well, but map does require more memory footprint and serialized with the same (key,value) pair (not able to extend to include type if needed). But if we concluded to transfer 1 time property stack, map serve well.
          Hide
          xhoong Xavier FH Leong added a comment -

          To add, JdbcMeta do not know the underlying connection is Calcite, it gets the property by calling the connection databasemeta class functions, so to build the map, it needs to call all the functions and cache it. Then there will be 3 layers of cache, at remote, server and Calcite.

          So I'm very keen to have the server and database layer service the single call, and only have the remote meta to cache the results in it's internal map to eliminate double request for static values.

          Show
          xhoong Xavier FH Leong added a comment - To add, JdbcMeta do not know the underlying connection is Calcite, it gets the property by calling the connection databasemeta class functions, so to build the map, it needs to call all the functions and cache it. Then there will be 3 layers of cache, at remote, server and Calcite. So I'm very keen to have the server and database layer service the single call, and only have the remote meta to cache the results in it's internal map to eliminate double request for static values.
          Hide
          xhoong Xavier FH Leong added a comment -

          New commit on pull request https://github.com/apache/incubator-calcite/pull/77/commits

          Somehow I'm getting the `assertThat` fail using `is("STRING")` from static string, getting from property is ok.

          Show
          xhoong Xavier FH Leong added a comment - New commit on pull request https://github.com/apache/incubator-calcite/pull/77/commits Somehow I'm getting the `assertThat` fail using `is("STRING")` from static string, getting from property is ok.
          Hide
          julianhyde Julian Hyde added a comment -

          I reworked your patch to return a single map. I checked into my own branch. Please review https://github.com/julianhyde/incubator-calcite/tree/522-DatabaseProperties.

          Show
          julianhyde Julian Hyde added a comment - I reworked your patch to return a single map. I checked into my own branch. Please review https://github.com/julianhyde/incubator-calcite/tree/522-DatabaseProperties .
          Hide
          xhoong Xavier FH Leong added a comment - - edited

          Looks good, tested OK on my side, thanks.

          Show
          xhoong Xavier FH Leong added a comment - - edited Looks good, tested OK on my side, thanks.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/e218cb15 .
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.3.0-incubating (2015-05-30).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.3.0-incubating (2015-05-30).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              julianhyde Julian Hyde
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development