Derby
  1. Derby
  2. DERBY-924

new JDBC4 metadata API getFunctions() needs to be implemented

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC
    • Labels:
      None
    • Environment:
      JDK 1.6

      Description

      For now I am implementing this to return empty result set so at least we're compliant, but we should be able to implement this one

      1. derby-924.v1.diff
        8 kB
        Dyre Tjeldvoll
      2. derby-924.v1.stat
        0.7 kB
        Dyre Tjeldvoll
      3. derbyall_report.v1.txt
        4 kB
        Dyre Tjeldvoll
      4. derby-924.v2.diff
        116 kB
        Dyre Tjeldvoll
      5. derby-924.v2.stat
        1 kB
        Dyre Tjeldvoll
      6. derbyall_report.txt
        50 kB
        Dyre Tjeldvoll
      7. derby-924.v3.diff
        44 kB
        Dyre Tjeldvoll
      8. derby-924.v3.report.txt
        4 kB
        Dyre Tjeldvoll
      9. derby-924.v3.stat
        2 kB
        Dyre Tjeldvoll

        Issue Links

          Activity

          David Van Couvering created issue -
          David Van Couvering made changes -
          Field Original Value New Value
          Component/s JDBC [ 11407 ]
          Affects Version/s 10.2.0.0 [ 11187 ]
          David Van Couvering made changes -
          Link This issue is part of DERBY-968 [ DERBY-968 ]
          Hide
          David Van Couvering added a comment -

          This is in a way a subtask. This metadata method needs to return new values. Right now it returns an empty result set.

          Show
          David Van Couvering added a comment - This is in a way a subtask. This metadata method needs to return new values. Right now it returns an empty result set.
          Dyre Tjeldvoll made changes -
          Assignee Dyre Tjeldvoll [ dyret ]
          Hide
          Dyre Tjeldvoll added a comment -

          I think this could be done in much the same way as for getProcedures. I think we can re-use the same query, as long as we change the restriction from ALIASTYPE = 'P' to ALIASTYPE = 'F'.

          I'm wondering if it is a good idea to select JAVACLASSNAME as REMARKS. But since this is not done for getProcedures it may not be a good idea to do it here either...

          Show
          Dyre Tjeldvoll added a comment - I think this could be done in much the same way as for getProcedures. I think we can re-use the same query, as long as we change the restriction from ALIASTYPE = 'P' to ALIASTYPE = 'F'. I'm wondering if it is a good idea to select JAVACLASSNAME as REMARKS. But since this is not done for getProcedures it may not be a good idea to do it here either...
          Dyre Tjeldvoll made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Actually, my prvious comment was wrong. getProcedures returns CAST ((JAVACLASSNAME || '.' || ALIASINFO->getMethodName()) AS VARCHAR(32672)) AS REMARKS, so I'll use that for getFunctions() as well.

          There is a comment in metadata.properties saying:

          1. 'REMARKS' column is VARCHAR(32672), which is the max length allowed
          2. for a VARCHAR. This is because Java methods with the complete
          3. package name plus possible signature can grow to be rather long.

          That is probably true, but ODBC 2.0 SQLProcedures specifies, that this column should be VARCHAR(254). But I guess it is better to be consitent with the existing getProcedures() implementation.

          Show
          Dyre Tjeldvoll added a comment - Actually, my prvious comment was wrong. getProcedures returns CAST ((JAVACLASSNAME || '.' || ALIASINFO->getMethodName()) AS VARCHAR(32672)) AS REMARKS, so I'll use that for getFunctions() as well. There is a comment in metadata.properties saying: 'REMARKS' column is VARCHAR(32672), which is the max length allowed for a VARCHAR. This is because Java methods with the complete package name plus possible signature can grow to be rather long. That is probably true, but ODBC 2.0 SQLProcedures specifies, that this column should be VARCHAR(254). But I guess it is better to be consitent with the existing getProcedures() implementation.
          Hide
          A B added a comment -

          > That is probably true, but ODBC 2.0 SQLProcedures specifies, that this column should be VARCHAR(254).

          Can you give a link reference that says this? I don't deny it, I'm just curious where this is. When I look here:

          http://msdn.microsoft.com/library/default.asp?url=/library/en-us/odbc/htm/odbcsqlprocedurecolumns.asp

          I see the type of REMARKS as "VARCHAR" with no length specification. It's possible that "254" is hidden somewhere in there, but if you can give a direct reference that'd be nice My work with ODBC metadata in the past (esp. DERBY-107) has suggested that while type is important (ex. CHAR vs VARCHAR, INT vs SMALLINT, nullable vs. not nullable, etc), the specific length of the VARCHAR hasn't been a concern. But of course that's just based on observation from the tests/scenarios I was working with. If length is specified as part of the ODBC specification, then perhaps that's something that needs further investigation (as part of a different Jira issue).

          Show
          A B added a comment - > That is probably true, but ODBC 2.0 SQLProcedures specifies, that this column should be VARCHAR(254). Can you give a link reference that says this? I don't deny it, I'm just curious where this is. When I look here: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/odbc/htm/odbcsqlprocedurecolumns.asp I see the type of REMARKS as "VARCHAR" with no length specification. It's possible that "254" is hidden somewhere in there, but if you can give a direct reference that'd be nice My work with ODBC metadata in the past (esp. DERBY-107 ) has suggested that while type is important (ex. CHAR vs VARCHAR, INT vs SMALLINT, nullable vs. not nullable, etc), the specific length of the VARCHAR hasn't been a concern. But of course that's just based on observation from the tests/scenarios I was working with. If length is specified as part of the ODBC specification, then perhaps that's something that needs further investigation (as part of a different Jira issue).
          Hide
          A B added a comment -

          Oops, wrong link in previous comment.

          http://msdn.microsoft.com/library/default.asp?url=/library/en-us/odbc/htm/odbcsqlprocedures.asp

          Same kind of thing, though--VARCHAR has no length specified...

          Show
          A B added a comment - Oops, wrong link in previous comment. http://msdn.microsoft.com/library/default.asp?url=/library/en-us/odbc/htm/odbcsqlprocedures.asp Same kind of thing, though--VARCHAR has no length specified...
          Hide
          Dyre Tjeldvoll added a comment -

          Yes, I see that those links say VARCHAR.

          I got VARCHAR(254) from "ODBC 3.5 Developer's Guide" by Roger E. Sanders, McGraw-Hill, (page 836). In the text it says "Adapted from the table on pages 878-879 of Microsoft ODBC 3.0 Software Development Kit & Programmer's reference".

          Show
          Dyre Tjeldvoll added a comment - Yes, I see that those links say VARCHAR. I got VARCHAR(254) from "ODBC 3.5 Developer's Guide" by Roger E. Sanders, McGraw-Hill, (page 836). In the text it says "Adapted from the table on pages 878-879 of Microsoft ODBC 3.0 Software Development Kit & Programmer's reference".
          Hide
          Dyre Tjeldvoll added a comment -

          Patch (derby-924.v1.*). Ran derbyall (1.5) verify that there are no regressions. Also ran the jdbc4 suite without failures (except the report-problem caused by a Mustang bug). Please review. Thanks.

          Show
          Dyre Tjeldvoll added a comment - Patch (derby-924.v1.*). Ran derbyall (1.5) verify that there are no regressions. Also ran the jdbc4 suite without failures (except the report-problem caused by a Mustang bug). Please review. Thanks.
          Dyre Tjeldvoll made changes -
          Attachment derby-924.v1.diff [ 12323561 ]
          Attachment derbyall_report.v1.txt [ 12323563 ]
          Attachment derby-924.v1.stat [ 12323562 ]
          Hide
          Dyre Tjeldvoll added a comment -

          I'm checking the 'Patch available' box to signal that a new, as of yet unreviewed, patch has been uploaded to this issue. If that is not the intended use of this feature, please let me know, (there seems to be some discussion about this).

          Show
          Dyre Tjeldvoll added a comment - I'm checking the 'Patch available' box to signal that a new, as of yet unreviewed, patch has been uploaded to this issue. If that is not the intended use of this feature, please let me know, (there seems to be some discussion about this).
          Dyre Tjeldvoll made changes -
          Other Info [Patch available]
          Hide
          Rick Hillegas added a comment -

          Looks clean to me. Derbyall passes modulo the expected failure in wisconsin. The jdbc4 tests also pass modulo the mustang timestamp formatting problem. I have checked in this patch as subversion revision 382379.

          Show
          Rick Hillegas added a comment - Looks clean to me. Derbyall passes modulo the expected failure in wisconsin. The jdbc4 tests also pass modulo the mustang timestamp formatting problem. I have checked in this patch as subversion revision 382379.
          Dyre Tjeldvoll made changes -
          Fix Version/s 10.2.0.0 [ 11187 ]
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Resolved [ 5 ]
          Hide
          Dyre Tjeldvoll added a comment -

          I'm reopening this issue since the client and NS part was missing from the first patch. (The TestDbMetaData test appeared to run successfully with all frameworks, but it actually ignored the framework and always ran in embedded mode. This has been fixed now).

          Show
          Dyre Tjeldvoll added a comment - I'm reopening this issue since the client and NS part was missing from the first patch. (The TestDbMetaData test appeared to run successfully with all frameworks, but it actually ignored the framework and always ran in embedded mode. This has been fixed now).
          Dyre Tjeldvoll made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Dyre Tjeldvoll made changes -
          Status Reopened [ 4 ] In Progress [ 3 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching new patch (derby-924.v2) that also implements getFunctions() in the client (through a stored procedure). Added a new test case to metadata.java to ensure that this is tested in the upgrade test. The new upgrade test ran successfully. Please review, thanks.

          Show
          Dyre Tjeldvoll added a comment - Attaching new patch (derby-924.v2) that also implements getFunctions() in the client (through a stored procedure). Added a new test case to metadata.java to ensure that this is tested in the upgrade test. The new upgrade test ran successfully. Please review, thanks.
          Dyre Tjeldvoll made changes -
          Attachment derbyall_report.txt [ 12325085 ]
          Attachment derby-924.v2.diff [ 12325083 ]
          Attachment derby-924.v2.stat [ 12325084 ]
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Hide
          Knut Anders Hatlen added a comment -

          The patch looks very good. There are however two issues that I would
          like to have resolved before I commit the patch.

          1) In DatabaseMetaData.getFunctionsX() you have this comment:

          // Uncomment the following line when DERBY-970 is committed
          // checkServerJdbcVersionX()

          Since your patch is likely to get into trunk before my DERBY-970
          patch, please copy checkServerJdbcVersionX() into your patch and
          enable the check.

          2) I liked the way you managed to test JDBC 4.0 functionality in a
          test that doesn't require jdk 1.6 to compile and run, but I don't
          see any good reason why the methods are invoked only when
          jvm>=1.6. That is, I would have removed these lines from
          metadata_test.java:

          // Make sure the method is available in the interface
          Class.forName("java.sql.DatabaseMetaData").
          getMethod("getFunctions", a);

          With this change, getFunctions() will be tested in JVM 1.4/1.5 as
          well, and you don't need to add separate canons for 1.6. Also note
          that this change will require you to update
          Upgrade_10_1_10_2.out. (Actually, in your current patch you should
          have created a jdk16 version of Upgrade_10_1_10_2.out, but that
          won't be necessary now.) I ran the upgrade test (jvm 1.5 and 1.6)
          with this change, and the diff looked reasonable.

          Show
          Knut Anders Hatlen added a comment - The patch looks very good. There are however two issues that I would like to have resolved before I commit the patch. 1) In DatabaseMetaData.getFunctionsX() you have this comment: // Uncomment the following line when DERBY-970 is committed // checkServerJdbcVersionX() Since your patch is likely to get into trunk before my DERBY-970 patch, please copy checkServerJdbcVersionX() into your patch and enable the check. 2) I liked the way you managed to test JDBC 4.0 functionality in a test that doesn't require jdk 1.6 to compile and run, but I don't see any good reason why the methods are invoked only when jvm>=1.6. That is, I would have removed these lines from metadata_test.java: // Make sure the method is available in the interface Class.forName("java.sql.DatabaseMetaData"). getMethod("getFunctions", a); With this change, getFunctions() will be tested in JVM 1.4/1.5 as well, and you don't need to add separate canons for 1.6. Also note that this change will require you to update Upgrade_10_1_10_2.out. (Actually, in your current patch you should have created a jdk16 version of Upgrade_10_1_10_2.out, but that won't be necessary now.) I ran the upgrade test (jvm 1.5 and 1.6) with this change, and the diff looked reasonable.
          Hide
          Dyre Tjeldvoll added a comment -

          Third version of the patch which addresses the review comments. getFunctions is now tested with all jvm versions, and new master for the Upgrade test is included.

          Show
          Dyre Tjeldvoll added a comment - Third version of the patch which addresses the review comments. getFunctions is now tested with all jvm versions, and new master for the Upgrade test is included.
          Dyre Tjeldvoll made changes -
          Attachment derby-924.v3.stat [ 12325208 ]
          Attachment derby-924.v3.diff [ 12325207 ]
          Attachment derby-924.v3.report.txt [ 12325209 ]
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 393304.

          I made some small changes to the patch before committing it:

          • two of the jdk16 canons touched by the patch were removed by the
            DERBY-1178 fix
          • updated jdk13 canon for metadata.java on DerbyNetClient

          I still have one minor question about the patch. getFunctionsX() ends
          with these two lines:

          lastGetProceduresResultSet_ = executeCatalogQuery(cs);
          return lastGetProceduresResultSet_;

          Should this have been lastGetFunctionsResultSet_ instead? Not that it
          matters, since none of the lastGetXXXResultSet_ variables seem to have
          any other purpose than ensuring that a lot of garbage is kept on the
          heap.

          Show
          Knut Anders Hatlen added a comment - Committed revision 393304. I made some small changes to the patch before committing it: two of the jdk16 canons touched by the patch were removed by the DERBY-1178 fix updated jdk13 canon for metadata.java on DerbyNetClient I still have one minor question about the patch. getFunctionsX() ends with these two lines: lastGetProceduresResultSet_ = executeCatalogQuery(cs); return lastGetProceduresResultSet_; Should this have been lastGetFunctionsResultSet_ instead? Not that it matters, since none of the lastGetXXXResultSet_ variables seem to have any other purpose than ensuring that a lot of garbage is kept on the heap.
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Dyre Tjeldvoll made changes -
          Status In Progress [ 3 ] Open [ 1 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Thanks for committing Knut-Anders.

          Show
          Dyre Tjeldvoll added a comment - Thanks for committing Knut-Anders.
          Dyre Tjeldvoll made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Rick Hillegas made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Dag H. Wanvik made changes -
          Issue Type New Feature [ 2 ] Improvement [ 4 ]
          Gavin made changes -
          Workflow jira [ 12346642 ] Default workflow, editable Closed status [ 12797681 ]

            People

            • Assignee:
              Dyre Tjeldvoll
              Reporter:
              David Van Couvering
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development