Derby
  1. Derby
  2. DERBY-1417

Add new, lengthless overloads to the streaming api

    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
    • Urgency:
      Normal

      Description

      The JDBC4 Expert Group has approved a new set of overloads for the streaming methods. These overloads do not take a length argument. Here are the new overloads:

      PreparedStatement.setAsciiStream(int parameterIndex, java.io.InputStream x)
      PreparedStatement.setBinaryStream(int parameterIndex, java.io.InputStream x)
      PreparedStatement.setCharacterStream(int parameterIndex, java.io.Reader reader)
      PreparedStatement.setNCharacterStream(int parameterIndex, java.io.Reader reader)
      PreparedStatement.setBlob(int parameterIndex, java.io.InputStream inputStream)
      PreparedStatement.setClob(int parameterIndex, java.io.Reader reader)
      PreparedStatement.setNClob(int parameterIndex, java.io.Reader reader)
      CallableStatement.setAsciiStream(java.lang.String parameterName, java.io.InputStream x)
      CallableStatement.setBinaryStream(java.lang.String parameterName, java.io.InputStream x)
      CallableStatement.setCharacterStream(java.lang.String parameterName, java.io.Reader reader)
      CallableStatement.setNCharacterStream(java.lang.String parameterName, java.io.Reader reader)
      CallableStatement.setBlob(java.lang.String parameterName, java.io.InputStream inputStream)
      CallableStatement.setClob(java.lang.String parameterName, java.io.Reader reader)
      CallableStatement.setNClob(java.lang.String parameterName, java.io.Reader reader)
      ResultSet.updateAsciiStream(int columnIndex, java.io.InputStream x)
      ResultSet.updateAsciiStream(java.lang.String columnLabel, java.io.InputStream x)
      ResultSet.updateBinaryStream(int columnIndex, java.io.InputStream x)
      ResultSet.updateBinaryStream(java.lang.String columnLabel, java.io.InputStream x, int length)
      ResultSet.updateCharacterStream(int columnIndex, java.io.Reader x)
      ResultSet.updateCharacterStream(java.lang.String columnLabel, java.io.Reader x)
      ResultSet.updateNCharacterStream(int columnIndex, java.io.Reader x)
      ResultSet.updateNCharacterStream(java.lang.String columnLabel, java.io.Reader x)
      ResultSet.updateBlob(int columnIndex, java.io.InputStream inputStream)
      ResultSet.updateBlob(java.lang.String columnLabel, java.io.InputStream inputStream)
      ResultSet.updateClob(int columnIndex, java.io.Reader reader)
      ResultSet.updateClob(java.lang.String columnLabel, java.io.Reader reader)
      ResultSet.updateNClob(int columnIndex, java.io.Reader reader)
      ResultSet.updateNClob(java.lang.String columnLabel, java.io.Reader reader)

      We should add these new overloads soon so that the build will not break when this methods turn up in a published Mustang build.

      1. derby-1417-9a-blobstream-newapproach.diff
        2 kB
        Kristian Waagan
      2. derby-1417-9a-blobstream-newapproach.stat
        0.2 kB
        Kristian Waagan
      3. derby-1417-8a-enableblobstreaming.diff
        2 kB
        Kristian Waagan
      4. derby-1417-8a-enableblobstreaming.stat
        0.2 kB
        Kristian Waagan
      5. derby-1417-7a-clientborderfix.diff
        12 kB
        Kristian Waagan
      6. derby-1417-7a-clientborderfix.stat
        0.3 kB
        Kristian Waagan
      7. derby-1417-6d-clientimpl.diff
        77 kB
        Kristian Waagan
      8. derby-1417-6c-clientimpl.diff
        77 kB
        Kristian Waagan
      9. derby-1417-6b-clientimpl.diff
        76 kB
        Kristian Waagan
      10. derby-1417-6a-clientimpl.diff
        74 kB
        Kristian Waagan
      11. derby-1417-6a-clientimpl.stat
        1 kB
        Kristian Waagan
      12. derby-1417-5a-brokered.diff
        19 kB
        Kristian Waagan
      13. derby-1417-5a-brokered.stat
        0.3 kB
        Kristian Waagan
      14. derby-1417-4a-disable-psTestsDnc.diff
        2 kB
        Kristian Waagan
      15. derby-1417-3b-embimpl-and-tests.diff
        90 kB
        Kristian Waagan
      16. derby-1417-3b-embimpl-and-tests.stat
        0.5 kB
        Kristian Waagan
      17. derby-1417-3a-embimpl-and-tests.diff
        89 kB
        Kristian Waagan
      18. derby-1417-3a-embimpl-and-tests.stat
        0.5 kB
        Kristian Waagan
      19. derby-1417-2a-rstest-refactor.diff
        35 kB
        Kristian Waagan
      20. derby-1417-1a-notImplemented.diff
        11 kB
        Kristian Waagan
      21. derby-1417-1a-notImplemented.stat
        0.5 kB
        Kristian Waagan
      22. derby-1417-01-castsInTests.diff
        3 kB
        Rick Hillegas

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Corrected the set of overloads.

          Show
          Rick Hillegas added a comment - Corrected the set of overloads.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-1417-01-castsInTests.diff. This patch adds some casts to some jdbc4 tests so that they will continue to compile when the new streaming overloads turn up in a future build of jdk1.6. Thanks to Ole and Narayanan for analyzing the problem and to Kristian for supplying the patch. The patch successfully compiles using mustang build 87 for the JDBC4 support. The jdbc4 tests pass without any new diffs. Touches the following files:

          M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\TestPreparedStatementMethods.java
          M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\CallableStatementTest.java
          M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\ResultSetTest.java

          Committed at subversion revision 414624.

          Show
          Rick Hillegas added a comment - Attaching derby-1417-01-castsInTests.diff. This patch adds some casts to some jdbc4 tests so that they will continue to compile when the new streaming overloads turn up in a future build of jdk1.6. Thanks to Ole and Narayanan for analyzing the problem and to Kristian for supplying the patch. The patch successfully compiles using mustang build 87 for the JDBC4 support. The jdbc4 tests pass without any new diffs. Touches the following files: M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\TestPreparedStatementMethods.java M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\CallableStatementTest.java M java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\ResultSetTest.java Committed at subversion revision 414624.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-1a-notImplemented.diff' adds a number of new lengthless streaming overloads that Derby will not support. All methods added by the patch throws not-implemented exceptions. We don't support them because we either don't support the data type or because we don't yet support named parameters in CallableStatement.
          I feel that, after a review, the patch is safe to apply even though I have not yet submitted any tests for it because:
          a) Only new methods are added.
          b) They all throw not-implemented exceptions.
          c) Mostly JDBC4 specific classes are affected.

          I will add tests, but have to wait until the signatures have made it into Mustang (I do have some tests already, but here I use the specific implementation classes, not the interfaces).

          The following methods have been added, both for the embedded and the client driver:

          [CallableStatement]

          • void setAsciiStream(String parameterName, InputStream x)
          • void setBinaryStream(String parameterName, InputStream x)
          • void setBlob(String parameterName, InputStream inputStream)
          • void setCharacterStream(String parameterName, Reader reader)
          • void setClob(String parameterName, Reader reader)
          • void setNCharacterStream(String parameterName, Reader value)
          • void setNClob(String parameterName, Reader reader)

          [PreparedStatement]

          • void setNCharacterStream(int parameterIndex, Reader value)
          • void setNClob(int parameterIndex, Reader reader)

          [ResultSet]

          • void updateNCharacterStream(int columIndex, Reader x)
          • void updateNCharacterStream(String columnName, Reader value)
          • void updateNClob(int columIndex, Reader reader)
          • void updateNClob(String columName, Reader reader)

          In addition to the new methods added, I added a class comment to EmbedResultSet40.java and moved two methods within that file.

          Note that I have added the not-implemented methods in the JDBC 40 specific classes (where available), while I intend to push the methods we do support to the superclass(es) - ie. CallableStatement.java instead of CallableStatement40.java.
          I plan to implement the remaining methods, and they will be adressed in separate patches.
          I have run the jdbc40 suite without failures.

          This partial patch is ready for review.

          Thanks,

          Show
          Kristian Waagan added a comment - 'derby-1417-1a-notImplemented.diff' adds a number of new lengthless streaming overloads that Derby will not support. All methods added by the patch throws not-implemented exceptions. We don't support them because we either don't support the data type or because we don't yet support named parameters in CallableStatement. I feel that, after a review, the patch is safe to apply even though I have not yet submitted any tests for it because: a) Only new methods are added. b) They all throw not-implemented exceptions. c) Mostly JDBC4 specific classes are affected. I will add tests, but have to wait until the signatures have made it into Mustang (I do have some tests already, but here I use the specific implementation classes, not the interfaces). The following methods have been added, both for the embedded and the client driver: [CallableStatement] void setAsciiStream(String parameterName, InputStream x) void setBinaryStream(String parameterName, InputStream x) void setBlob(String parameterName, InputStream inputStream) void setCharacterStream(String parameterName, Reader reader) void setClob(String parameterName, Reader reader) void setNCharacterStream(String parameterName, Reader value) void setNClob(String parameterName, Reader reader) [PreparedStatement] void setNCharacterStream(int parameterIndex, Reader value) void setNClob(int parameterIndex, Reader reader) [ResultSet] void updateNCharacterStream(int columIndex, Reader x) void updateNCharacterStream(String columnName, Reader value) void updateNClob(int columIndex, Reader reader) void updateNClob(String columName, Reader reader) In addition to the new methods added, I added a class comment to EmbedResultSet40.java and moved two methods within that file. Note that I have added the not-implemented methods in the JDBC 40 specific classes (where available), while I intend to push the methods we do support to the superclass(es) - ie. CallableStatement.java instead of CallableStatement40.java. I plan to implement the remaining methods, and they will be adressed in separate patches. I have run the jdbc40 suite without failures. This partial patch is ready for review. Thanks,
          Hide
          Knut Anders Hatlen added a comment -

          Committed 'derby-1417-1a-notImplemented.diff' into trunk with revision 417753.

          Show
          Knut Anders Hatlen added a comment - Committed 'derby-1417-1a-notImplemented.diff' into trunk with revision 417753.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-2a-rstest-refactor.diff' refactors and rewrites parts of jdbc4/ResultSetTest.junit.
          A few utility methods have been introduced, and a TestSetup-class (anonymous) was used to create the table once. This was done because the previous implementation could cause other tests to fail if one test failed.

          The utility methods follow a pattern that will also be used by a number of tests written for the new lengthless overloads.

          I have run jdbc4/ResultSetTest.junit and the jdbc40 suite (embedded & DerbyNetClient) without failures. No failures before refactoring, no failures afterwards. Please review/commit.

          Show
          Kristian Waagan added a comment - 'derby-1417-2a-rstest-refactor.diff' refactors and rewrites parts of jdbc4/ResultSetTest.junit. A few utility methods have been introduced, and a TestSetup-class (anonymous) was used to create the table once. This was done because the previous implementation could cause other tests to fail if one test failed. The utility methods follow a pattern that will also be used by a number of tests written for the new lengthless overloads. I have run jdbc4/ResultSetTest.junit and the jdbc40 suite (embedded & DerbyNetClient) without failures. No failures before refactoring, no failures afterwards. Please review/commit.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-2a-rstest-refactor.diff' ready for review/commit.

          Show
          Kristian Waagan added a comment - 'derby-1417-2a-rstest-refactor.diff' ready for review/commit.
          Hide
          Knut Anders Hatlen added a comment -

          derby-1417-2a-rstest-refactor.diff looks good. Committed revision 420497.

          Show
          Knut Anders Hatlen added a comment - derby-1417-2a-rstest-refactor.diff looks good. Committed revision 420497.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-3a-embimpl-and-tests.diff' provides tests and implementations for the following methods on the embedded side:
          [ResultSet]
          public void updateAsciiStream(int columnIndex, InputStream x)
          public void updateBinaryStream(int columnIndex, InputStream x)
          public void updateCharacterStream(int columnIndex, Reader x)
          public void updateAsciiStream(String columnName, InputStream x)
          public void updateBinaryStream(String columnName, InputStream x)
          public void updateCharacterStream(String columnName, Reader reader)
          public void updateBlob(int columnIndex, InputStream x)
          public void updateBlob(String columnName, InputStream x)
          public void updateClob(int columnIndex, Reader x)
          public void updateClob(String columnName, Reader x)
          [PreparedStatement]
          public void setBinaryStream(int parameterIndex, InputStream x)
          public void setAsciiStream(int parameterIndex, InputStream x)
          public void setCharacterStream(int parameterIndex, Reader reader)
          public void setClob(int parameterIndex, Reader reader)
          public void setBlob(int parameterIndex, InputStream inputStream)

          IMPORTANT: This patch must be built with Mustang build 91 for the tests to compile!

          Some of the tests are temporarily disabled for the client driver. These will be enabed when the client implementation is submitted.

          I made some changes to ReaderToUTF8Stream, and to the setXXXStreamInteral-methods. I would appreciate if someone had a look at them.

          Derbyall ran cleanly minus the 'dynamic' JDBC 4 tests (VerifySignatures, ClosedObjects, UnsupportedVetter).
          I plan to do some additional testing with large LOBs, and will report back on this. These tests will not run as part of any suite (due to time and memory requirements), but I might submit the code for inclusion anyway.

          To the committers: Please do not commit this before Mustang build 91 is out!
          (must be available at http://download.java.net/jdk6/binaries/)

          Show
          Kristian Waagan added a comment - 'derby-1417-3a-embimpl-and-tests.diff' provides tests and implementations for the following methods on the embedded side: [ResultSet] public void updateAsciiStream(int columnIndex, InputStream x) public void updateBinaryStream(int columnIndex, InputStream x) public void updateCharacterStream(int columnIndex, Reader x) public void updateAsciiStream(String columnName, InputStream x) public void updateBinaryStream(String columnName, InputStream x) public void updateCharacterStream(String columnName, Reader reader) public void updateBlob(int columnIndex, InputStream x) public void updateBlob(String columnName, InputStream x) public void updateClob(int columnIndex, Reader x) public void updateClob(String columnName, Reader x) [PreparedStatement] public void setBinaryStream(int parameterIndex, InputStream x) public void setAsciiStream(int parameterIndex, InputStream x) public void setCharacterStream(int parameterIndex, Reader reader) public void setClob(int parameterIndex, Reader reader) public void setBlob(int parameterIndex, InputStream inputStream) IMPORTANT : This patch must be built with Mustang build 91 for the tests to compile! Some of the tests are temporarily disabled for the client driver. These will be enabed when the client implementation is submitted. I made some changes to ReaderToUTF8Stream, and to the setXXXStreamInteral-methods. I would appreciate if someone had a look at them. Derbyall ran cleanly minus the 'dynamic' JDBC 4 tests (VerifySignatures, ClosedObjects, UnsupportedVetter). I plan to do some additional testing with large LOBs, and will report back on this. These tests will not run as part of any suite (due to time and memory requirements), but I might submit the code for inclusion anyway. To the committers: Please do not commit this before Mustang build 91 is out! (must be available at http://download.java.net/jdk6/binaries/ )
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian,

          I have reviewed your patch. The code changes look good and derbyall
          runs cleanly on Sun JVM 1.5.0. I ran the jdbc40 tests, and they
          complain because some of the methods are missing from
          EmbedCallableStatement40 (they should only throw not supported). Also,
          they report that many methods are missing from the Brokered* classes,
          but I guess you will add them in a later patch?

          When I run 'ant javadoc', I see these warnings:

          EmbedResultSet.java:2957: warning - @param argument "parameterIndex" is not a parameter name.
          EmbedResultSet.java:4778: warning - @param argument "columnLabel" is not a parameter name.
          EmbedResultSet.java:4830: warning - @param argument "x" is not a parameter name.
          EmbedResultSet.java:4888: warning - @param argument "inputStream" is not a parameter name.
          EmbedResultSet.java:4944: warning - @param argument "inputStream" is not a parameter name.
          EmbedResultSet.java:5004: warning - @param argument "reader" is not a parameter name.
          EmbedResultSet.java:5062: warning - @param argument "reader" is not a parameter name.

          Some more comments/questions:

          EmbedResultSet.java:

          • javadocs for for updateAsciiStream(), updateBinaryStream(),
            updateCharacterStream(), updateBlob() and updateClob() say
            "@throws SQLFeatureNotSupportedException if the JDBC driver does
            not support this method". Since Derby does support these methods,
            that sentence could be removed.
          • line exceeding 80 characters in updateCharacterStreamInternal()
          • the update* methods start with a switch on colType. Could the
            switch be replaced with a call to
            DataTypeDescriptor.isJDBCTypeEquivalent() or factored out somehow?

          EmbedPreparedStatement.java:

          • I feel that the names of the new assert*Conditions methods are a
            little confusing. When I read "assert", I first thought they were
            used for asserting certain conditions in debug/sane builds. What
            about renaming them to check*Conditions?
          • javadocs contain "@throws SQLFeatureNotSupportedException" for
            methods that are implemented
          • line exceeding 80 characters in setBlob(int,Blob)
          • assertBlobConditions() and assertClobConditions() have a comment
            about DB2 compliance. Since the behaviour it refers to (only allow
            setBlob() on BLOB columns and setClob() on CLOB columns) seems to
            be exactly as specified by the JDBC spec, I think referring to DB2
            confuses more than it clarifies.

          ReaderToUTF8Stream.java:

          • The constructor can throw an IllegalArgumentException, but it is
            not caught anywhere, so it will propagate up to the application as
            an IllegalArgumentException, not as an SQLException. Since this
            exceptional situation only happens if there is a bug in Derby,
            perhaps SanityManager.ASSERT could be used instead?

          Tests:

          • the new assertEquals() methods could be useful to have in
            BaseTestCase
          • I think it would be good to test that removal of trailing blanks
            in clobs is handled correctly
          Show
          Knut Anders Hatlen added a comment - Hi Kristian, I have reviewed your patch. The code changes look good and derbyall runs cleanly on Sun JVM 1.5.0. I ran the jdbc40 tests, and they complain because some of the methods are missing from EmbedCallableStatement40 (they should only throw not supported). Also, they report that many methods are missing from the Brokered* classes, but I guess you will add them in a later patch? When I run 'ant javadoc', I see these warnings: EmbedResultSet.java:2957: warning - @param argument "parameterIndex" is not a parameter name. EmbedResultSet.java:4778: warning - @param argument "columnLabel" is not a parameter name. EmbedResultSet.java:4830: warning - @param argument "x" is not a parameter name. EmbedResultSet.java:4888: warning - @param argument "inputStream" is not a parameter name. EmbedResultSet.java:4944: warning - @param argument "inputStream" is not a parameter name. EmbedResultSet.java:5004: warning - @param argument "reader" is not a parameter name. EmbedResultSet.java:5062: warning - @param argument "reader" is not a parameter name. Some more comments/questions: EmbedResultSet.java: javadocs for for updateAsciiStream(), updateBinaryStream(), updateCharacterStream(), updateBlob() and updateClob() say "@throws SQLFeatureNotSupportedException if the JDBC driver does not support this method". Since Derby does support these methods, that sentence could be removed. line exceeding 80 characters in updateCharacterStreamInternal() the update* methods start with a switch on colType. Could the switch be replaced with a call to DataTypeDescriptor.isJDBCTypeEquivalent() or factored out somehow? EmbedPreparedStatement.java: I feel that the names of the new assert*Conditions methods are a little confusing. When I read "assert", I first thought they were used for asserting certain conditions in debug/sane builds. What about renaming them to check*Conditions? javadocs contain "@throws SQLFeatureNotSupportedException" for methods that are implemented line exceeding 80 characters in setBlob(int,Blob) assertBlobConditions() and assertClobConditions() have a comment about DB2 compliance. Since the behaviour it refers to (only allow setBlob() on BLOB columns and setClob() on CLOB columns) seems to be exactly as specified by the JDBC spec, I think referring to DB2 confuses more than it clarifies. ReaderToUTF8Stream.java: The constructor can throw an IllegalArgumentException, but it is not caught anywhere, so it will propagate up to the application as an IllegalArgumentException, not as an SQLException. Since this exceptional situation only happens if there is a bug in Derby, perhaps SanityManager.ASSERT could be used instead? Tests: the new assertEquals() methods could be useful to have in BaseTestCase I think it would be good to test that removal of trailing blanks in clobs is handled correctly
          Hide
          Kristian Waagan added a comment -

          Truncation of trailing blanks for length less Clobs will be added as part of DERBY-1473.

          Show
          Kristian Waagan added a comment - Truncation of trailing blanks for length less Clobs will be added as part of DERBY-1473 .
          Hide
          Kristian Waagan added a comment -

          Thanks for the review Knut Anders!
          My replies follow the order of the questions in your comment.

          • I forgot to duplicate the methods from PreparedStatement40 in
            EmbedCallableStatement40 (no inheritance here). I have added them in the
            new patch. I followed the "policy" of keeping unimplemented methods in the
            JDBC40 specific classes. I still get 21 failures in VerifySignatures, but
            all are in the Brokered* classes.
          • Brokered* methods will be added in a follow-up patch (I feel this patch is
            already too big). Since no JDBC4 tests are picking up these missing methods
            except for the dynamic ones (VerifySignatures, UnsupportedVetter,
            ClosedObjects), I think maybe we should run more of our tests with
            XA/pooled connections. Does anyone else feel the same?
          • I have fixed the JavaDoc errors.

          EmbedResultSet.java:

          • Removed "@throws SQLFeatureNotSupported" in JavaDoc.
          • Shortened long line.
          • Yes, the switch can be factored out. I decided to put this on hold, as I'm
            not sure what is the best approach. It makes sense to factor out the
            occurences where only the type is checked, and no other action is taken
            based on the type. This is typical for the ResultSet.updateX methods, but
            not for ResultSet.getXStream methods. Not sure if
            DataTypeDescriptor.isJDBCTypeEquivalent() can be used as it is, for
            instance it does not know anything about Types.BLOB.
            The common mechanism should/could also be used across different classes,
            for instance in both EmbedResultSet and EmbedPreparedStatement. So, where
            should it be placed?
            Feel free to add a Jira to track this.

          EmbedPreparedStatement.java:

          • Ok. Names changed.
          • Removed "@throws SQLFeatureNotSupported" in JavaDoc.
          • Shortened long line.
          • I removed the comments about DB2 compliance (these were already present
            before my patch).

          ReaderToUTF8Stream.java:

          • Was not sure how to handle this. I guess only getting this with debug/sane
            builds is good enough. I replaced the exception with a
            SanityManager.DEBUG block.

          Tests:

          • I added DERBY-1524 for the assertEquals-overloads (a sub-task of 1122).
          • This will be added as part of DERBY-1473. Might have to do something on the
            client side also.

          In addition to the comments from the review, I changed the following (not
          related to my patch):

          • Modified EmbedResultSet.updateAsciiStream(int,InputStream,long) to use
            updateCharacterStreamInternal instead of updateCharacterStream to avoid
            duplicate checks.
          • Removed blank line at the end of EmbedResultSet.java.
          • Corrected spelling error in PreparedStatementTest: Inerted -> Inserted

          I reran suite jdbc4. Only saw 3 expected failures: ClosedObjectTest,
          UnsupportedVetter and VerifySignatures.
          'derby-1417-3b-embimpl-and-tests.diff' is ready for more review and/or commit.

          Show
          Kristian Waagan added a comment - Thanks for the review Knut Anders! My replies follow the order of the questions in your comment. I forgot to duplicate the methods from PreparedStatement40 in EmbedCallableStatement40 (no inheritance here). I have added them in the new patch. I followed the "policy" of keeping unimplemented methods in the JDBC40 specific classes. I still get 21 failures in VerifySignatures, but all are in the Brokered* classes. Brokered* methods will be added in a follow-up patch (I feel this patch is already too big). Since no JDBC4 tests are picking up these missing methods except for the dynamic ones (VerifySignatures, UnsupportedVetter, ClosedObjects), I think maybe we should run more of our tests with XA/pooled connections. Does anyone else feel the same? I have fixed the JavaDoc errors. EmbedResultSet.java: Removed "@throws SQLFeatureNotSupported" in JavaDoc. Shortened long line. Yes, the switch can be factored out. I decided to put this on hold, as I'm not sure what is the best approach. It makes sense to factor out the occurences where only the type is checked, and no other action is taken based on the type. This is typical for the ResultSet.updateX methods, but not for ResultSet.getXStream methods. Not sure if DataTypeDescriptor.isJDBCTypeEquivalent() can be used as it is, for instance it does not know anything about Types.BLOB. The common mechanism should/could also be used across different classes, for instance in both EmbedResultSet and EmbedPreparedStatement. So, where should it be placed? Feel free to add a Jira to track this. EmbedPreparedStatement.java: Ok. Names changed. Removed "@throws SQLFeatureNotSupported" in JavaDoc. Shortened long line. I removed the comments about DB2 compliance (these were already present before my patch). ReaderToUTF8Stream.java: Was not sure how to handle this. I guess only getting this with debug/sane builds is good enough. I replaced the exception with a SanityManager.DEBUG block. Tests: I added DERBY-1524 for the assertEquals-overloads (a sub-task of 1122). This will be added as part of DERBY-1473 . Might have to do something on the client side also. In addition to the comments from the review, I changed the following (not related to my patch): Modified EmbedResultSet.updateAsciiStream(int,InputStream,long) to use updateCharacterStreamInternal instead of updateCharacterStream to avoid duplicate checks. Removed blank line at the end of EmbedResultSet.java. Corrected spelling error in PreparedStatementTest: Inerted -> Inserted I reran suite jdbc4. Only saw 3 expected failures: ClosedObjectTest, UnsupportedVetter and VerifySignatures. 'derby-1417-3b-embimpl-and-tests.diff' is ready for more review and/or commit.
          Hide
          Knut Anders Hatlen added a comment -

          Thank you for addressing my comments, Kristian. I am happy with the
          changes and have committed the 3b patch into trunk with revision
          422995.

          As to your questions, yes, I definitely think it would be a good idea
          to run more of the tests with xa/pooled connections. I believe Anurag
          submitted a patch which made it simpler to run the existing JUnit
          tests with XAConnection. It would be great if we could use that
          functionality to get better test coverage.

          I think the best place for the common type compatibility checks would
          be in DataTypeDescriptor. For instance, isCharacterStreamCompatible()
          and isBinaryStreamCompatible(). I will file a JIRA for it.

          Show
          Knut Anders Hatlen added a comment - Thank you for addressing my comments, Kristian. I am happy with the changes and have committed the 3b patch into trunk with revision 422995. As to your questions, yes, I definitely think it would be a good idea to run more of the tests with xa/pooled connections. I believe Anurag submitted a patch which made it simpler to run the existing JUnit tests with XAConnection. It would be great if we could use that functionality to get better test coverage. I think the best place for the common type compatibility checks would be in DataTypeDescriptor. For instance, isCharacterStreamCompatible() and isBinaryStreamCompatible(). I will file a JIRA for it.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-4a-disable-psTestsDnc.diff' disables 5 tests in jdbc4/PreparedStatementTest for DerbyNetClient.
          This error must have slipped off the table and crawled under the carpet... Sorry for the noise. The tests will be enabled again when the client-side implementation is done.

          Please commit this as soon as possible, to stop the test from failing in the nightly runs. Thanks.

          Show
          Kristian Waagan added a comment - 'derby-1417-4a-disable-psTestsDnc.diff' disables 5 tests in jdbc4/PreparedStatementTest for DerbyNetClient. This error must have slipped off the table and crawled under the carpet... Sorry for the noise. The tests will be enabled again when the client-side implementation is done. Please commit this as soon as possible, to stop the test from failing in the nightly runs. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          Committed the 4a patch into trunk with revision 423068. (I changed one line before committing because the patch disabled one test case that would actually work with the client driver.)

          Show
          Knut Anders Hatlen added a comment - Committed the 4a patch into trunk with revision 423068. (I changed one line before committing because the patch disabled one test case that would actually work with the client driver.)
          Hide
          Kristian Waagan added a comment -

          'derby-1417-5a-brokered.diff' adds the new overloads to the iapi/jdbc/Brokered*-classes.
          The jdbc4 suite now runs without failure under embedded. Note that there will still be failures when running under DerbyNetClient.
          Comments:

          • Note the method duplication in BrokeredCallableStatement40.
          • I have not added JavaDoc for methods that are not implemented.
          • I updated the UnsupportedVetter-test with the new methods we don't support;
          • methods with named parameters in CallableStatement
          • methods for unsupported datatypes in Prepared-/CallableStatement and ResultSet.

          Patch is ready for review.

          Show
          Kristian Waagan added a comment - 'derby-1417-5a-brokered.diff' adds the new overloads to the iapi/jdbc/Brokered*-classes. The jdbc4 suite now runs without failure under embedded. Note that there will still be failures when running under DerbyNetClient. Comments: Note the method duplication in BrokeredCallableStatement40. I have not added JavaDoc for methods that are not implemented. I updated the UnsupportedVetter-test with the new methods we don't support; methods with named parameters in CallableStatement methods for unsupported datatypes in Prepared-/CallableStatement and ResultSet. Patch is ready for review.
          Hide
          Knut Anders Hatlen added a comment -

          Committed the 5a patch into trunk with revision 423807.

          Show
          Knut Anders Hatlen added a comment - Committed the 5a patch into trunk with revision 423807.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-6a-clientimpl.diff' adds the client side implementations of the length less overloads.
          I had to make some changes "here and there" to get this working, and I remind people that this is a somewhat temporarily implementation due to the fact that we do not (yet) support streaming of data with unknown length.
          Therefore, streams used for input are exhausted and materialized in memory before they are sent from the client to the server.

          This is the last patch adding new JDBC 4 methods, but there will be at least one more follow-up patch for the embedded driver (to fix OutOfMemoryError), and DERBY-1473 should be completed as well.

          Comments to the patch:

          • LOBs are materialized when execute* is called (in [BC]lob.length()).
          • I renamed and generalized the existing class UTF32BEEncodedInputStream to reuse code. It now delivers InputStreams with UTF8 or UTF16 encoding and is used to create a stream from a reader.
          • I continued to use the encoding "US-ASCII" over "ISO-8859-1" to be consistent. This will be handled by DERBY-1519.
          • I added ByteArrayCombinerStream (and a test) to reduce memory usage on the client.
          • I enabled tests temporarily disabled for DerbyNetClient.

          I ran suite jdbc4 without failures. Because I'm running a bit short of time, I post the patch before my derbyalls are finished. I will add the results tomorrow.
          Patch is ready for review.

          For the committer: I did a 'svn rename' on UTF32BEEncodedInputStream. I think it should be handled by the diff, but keep an extra eye on it

          Thanks,

          Show
          Kristian Waagan added a comment - 'derby-1417-6a-clientimpl.diff' adds the client side implementations of the length less overloads. I had to make some changes "here and there" to get this working, and I remind people that this is a somewhat temporarily implementation due to the fact that we do not (yet) support streaming of data with unknown length. Therefore, streams used for input are exhausted and materialized in memory before they are sent from the client to the server. This is the last patch adding new JDBC 4 methods, but there will be at least one more follow-up patch for the embedded driver (to fix OutOfMemoryError), and DERBY-1473 should be completed as well. Comments to the patch: LOBs are materialized when execute* is called (in [BC] lob.length()). I renamed and generalized the existing class UTF32BEEncodedInputStream to reuse code. It now delivers InputStreams with UTF8 or UTF16 encoding and is used to create a stream from a reader. I continued to use the encoding "US-ASCII" over "ISO-8859-1" to be consistent. This will be handled by DERBY-1519 . I added ByteArrayCombinerStream (and a test) to reduce memory usage on the client. I enabled tests temporarily disabled for DerbyNetClient. I ran suite jdbc4 without failures. Because I'm running a bit short of time, I post the patch before my derbyalls are finished. I will add the results tomorrow. Patch is ready for review. For the committer: I did a 'svn rename' on UTF32BEEncodedInputStream. I think it should be handled by the diff, but keep an extra eye on it Thanks,
          Hide
          Rick Hillegas added a comment -

          1) The patch tool objects when I try to apply this fix:

          patching file java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java
          patching file java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
          patching file java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ByteArrayCombinerStreamTest.java
          patching file java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ByteArrayCombinerStreamTest_app.properti
          es
          patching file java/testing/org/apache/derbyTesting/functionTests/suites/derbynetclientmats.runall
          can't find file to patch at input line 429
          Perhaps you used the wrong -p or --strip option?
          The text leading up to this was:
          --------------------------

          Index: java/client/org/apache/derby/client/net/EncodedInputStream.java
          ===================================================================
          — java/client/org/apache/derby/client/net/EncodedInputStream.java (revision 424971)
          +++ java/client/org/apache/derby/client/net/EncodedInputStream.java (working copy)
          --------------------------
          File to patch:

          2) In addition, it looks like we're missing the copyright boilerplate in ByteArrayCombinerStreamTest.java and ByteArrayCombinerStream.java.

          Show
          Rick Hillegas added a comment - 1) The patch tool objects when I try to apply this fix: patching file java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/PreparedStatementTest.java patching file java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java patching file java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ByteArrayCombinerStreamTest.java patching file java/testing/org/apache/derbyTesting/functionTests/tests/derbynet/ByteArrayCombinerStreamTest_app.properti es patching file java/testing/org/apache/derbyTesting/functionTests/suites/derbynetclientmats.runall can't find file to patch at input line 429 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- Index: java/client/org/apache/derby/client/net/EncodedInputStream.java =================================================================== — java/client/org/apache/derby/client/net/EncodedInputStream.java (revision 424971) +++ java/client/org/apache/derby/client/net/EncodedInputStream.java (working copy) -------------------------- File to patch: 2) In addition, it looks like we're missing the copyright boilerplate in ByteArrayCombinerStreamTest.java and ByteArrayCombinerStream.java.
          Hide
          Tomohito Nakayama added a comment -

          I notice the problem that buffer size of EncodedInputStream does not suit for all encoding, especially UTF-8.
          UTF-8 needs three bytes for a character which is out of ISO-8859-1.

          This is reason why I limit the encoding to UTF32BE in UTF32BEEncodedInputStream.

          If you intend to share the class to multiple encodins,
          I think you should create subclass of each encodings inherited from EncodedInputStream which contaiins hard-coded size of buffer and encoding identifier.

          Show
          Tomohito Nakayama added a comment - I notice the problem that buffer size of EncodedInputStream does not suit for all encoding, especially UTF-8. UTF-8 needs three bytes for a character which is out of ISO-8859-1. This is reason why I limit the encoding to UTF32BE in UTF32BEEncodedInputStream. If you intend to share the class to multiple encodins, I think you should create subclass of each encodings inherited from EncodedInputStream which contaiins hard-coded size of buffer and encoding identifier.
          Hide
          Kristian Waagan added a comment -

          Hi Tomohito,

          Are you talking about the buffer for the PublicBufferOutputStream (which is a subclass of ByteArrayOutputStream)?
          I agree it might not be optimal, but ByteArrayOutputStream grows the buffer as needed. If we are reading chars outside ISO-8859-1, the ByteArrayOutputStream would have to double its internal buffer once for every 1024 characters we read (from 2048 bytes to 4096 bytes).
          My take on this, is that a separate Jira should be logged for this issue if it is worth optimizing/analyzing.

          Also, I was very confused by the naming of the file UTF32BEEncodedInputStream and its comments. I found that it actually used the encoding UnicodeBigUnmarked, which is the Java name for UTF-16BE. But the name and the comments claimed UTF-32BE was used.

          As for the subclasses, I thought about this, but decided not to do it, since the class is completely internal. If we do it, however, we can make the one for UnicodeBigUnmarked package private, and the UTF8 one public, since the latter one is the only one used outside the client/net package. I also thought about ways to restrict the available encodings, but again, this is an internal class and we should be able to choose only standard encodings (always available in Java). I mentioned this in the class comment. If you feel strongly about this, I can make 3 classes (please let me know).

          Thanks for looking at the patch

          BTW: My derbyalls showed a lot of unrelated errors (I got 18 total with 1.6), so I need time to study the results before I post anything. However, with 1.4 I only got a few known issues.

          Show
          Kristian Waagan added a comment - Hi Tomohito, Are you talking about the buffer for the PublicBufferOutputStream (which is a subclass of ByteArrayOutputStream)? I agree it might not be optimal, but ByteArrayOutputStream grows the buffer as needed. If we are reading chars outside ISO-8859-1, the ByteArrayOutputStream would have to double its internal buffer once for every 1024 characters we read (from 2048 bytes to 4096 bytes). My take on this, is that a separate Jira should be logged for this issue if it is worth optimizing/analyzing. Also, I was very confused by the naming of the file UTF32BEEncodedInputStream and its comments. I found that it actually used the encoding UnicodeBigUnmarked, which is the Java name for UTF-16BE. But the name and the comments claimed UTF-32BE was used. As for the subclasses, I thought about this, but decided not to do it, since the class is completely internal. If we do it, however, we can make the one for UnicodeBigUnmarked package private, and the UTF8 one public, since the latter one is the only one used outside the client/net package. I also thought about ways to restrict the available encodings, but again, this is an internal class and we should be able to choose only standard encodings (always available in Java). I mentioned this in the class comment. If you feel strongly about this, I can make 3 classes (please let me know). Thanks for looking at the patch BTW: My derbyalls showed a lot of unrelated errors (I got 18 total with 1.6), so I need time to study the results before I post anything. However, with 1.4 I only got a few known issues.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-6b-clientimpl.diff' deprecates 6a.
          Changes:
          1) Add missing license to two files.
          2) Added static factory methods to EncodedInputStream for UTF-8- and UTF-16BE-streams.
          Set byte buffer size to 3 and 2 times size of char buffer, respectively.
          'createUTF8Stream' is public, 'createUTF16BEStream' is package private.
          I made class 'EncodedInputStream' public final, and its constructor private.
          3) Updated two classes (Request and Clob) to use the new static methods.

          When applying patch, do a 'svn rename java/client/org/apache/derby/client/net/UTF32BEEncodedInputStream.java java/client/org/apache/derby/client/net/EncodedInputStream.java' first. Just press ENTER (answer no) at the two prompts when running the patch tool.

          Show
          Kristian Waagan added a comment - 'derby-1417-6b-clientimpl.diff' deprecates 6a. Changes: 1) Add missing license to two files. 2) Added static factory methods to EncodedInputStream for UTF-8- and UTF-16BE-streams. Set byte buffer size to 3 and 2 times size of char buffer, respectively. 'createUTF8Stream' is public, 'createUTF16BEStream' is package private. I made class 'EncodedInputStream' public final, and its constructor private. 3) Updated two classes (Request and Clob) to use the new static methods. When applying patch, do a 'svn rename java/client/org/apache/derby/client/net/UTF32BEEncodedInputStream.java java/client/org/apache/derby/client/net/EncodedInputStream.java' first. Just press ENTER (answer no) at the two prompts when running the patch tool.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-6c-clientimpl.diff' deprecates 6b.
          Added both size of the character buffer and the initial size of the byte buffer to the constructor of EncodedInputStream.
          Made the required changes elsewhere due to this addition.

          Suite jdbc40 ran without failures, and all failures seen during derbyall (17 in total) have been verified to be caused by other issues (15 because of DERBY-1578 and 1 because of local environment errors and 1 has already been fixed in trunk).

          The 'svn rename' command must still be run before the patch is applied.

          Show
          Kristian Waagan added a comment - 'derby-1417-6c-clientimpl.diff' deprecates 6b. Added both size of the character buffer and the initial size of the byte buffer to the constructor of EncodedInputStream. Made the required changes elsewhere due to this addition. Suite jdbc40 ran without failures, and all failures seen during derbyall (17 in total) have been verified to be caused by other issues (15 because of DERBY-1578 and 1 because of local environment errors and 1 has already been fixed in trunk). The 'svn rename' command must still be run before the patch is applied.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian,

          I have had a look at your patch, and I have a couple of comments and
          questions:

          1) I believe using Lob.materializeStream() to get the length of a
          Clob is not correct. sqlLength_ is set to the number of bytes
          (UTF-8 encoded), not the number characters. I wrote a small test
          that called ResultSet.updateClob(int,Reader) and getClob() to get
          it back, and Clob.length() returned more than the number of
          characters when there were one or more non-ascii characters.

          2) EncodedInputStream uses "UTF8" and "UnicodeBigUnmarked" as
          encodings. These names are not necessarily understood by all
          JVMs. However, "UTF-8" and "UTF-16BE" (for which UTF8 and
          UnicodeBigUnmarked are aliases) must be supported by all JVMs, so
          it is probably better to use those names.

          3) Clob's constructor checks whether the supplied encoding is
          UnicodeBigUnmarked, but I didn't find that it was called with
          this encoding anywhere in the code. Could that part be removed?

          4) Clob's constructor checks whether the supplied encoding is
          US-ASCII, UTF-8 or UnicodeBigUnmarked, but if it's neither of
          those, no error is signalled. Wouldn't it be appropriate to throw
          some kind of SQLException? If so, SQLState.UNSUPPORTED_ENCODING
          sounds like a good choice.

          5) In ByteArrayCombinerStream, I think it would be good to convert
          the comments which describe the instance variables to
          javadoc. Also, the method description seems to be missing from
          the constructor and read(byte[],int,int).

          6) ByteArrayCombinerStreamTest is a subclass of BaseJDBCTestCase,
          but it's not a JDBC test. Shouldn't it have been a direct
          subclass of BaseTestCase?

          7) (minor) I have noticed in the svn log that there is an ongoing
          clean-up of the JUnit tests. Among other things, setUp and
          tearDown methods have been made protected instead of
          public. Maybe setUp and tearDown in ByteArrayCombinerStreamTest
          should be made protected too?

          8) (very minor) The condition in the while clause in
          ByteArrayCombinerStream.read(byte[],int,int) tests that (read !=
          length). I feel the code would be easier to read if the test was
          (read < length) since that would tell the reader that the read
          variable is increasing in the loop. Also, it wouldn't go into an
          infinite loop if there's an off-by-one error.

          Thanks.

          Show
          Knut Anders Hatlen added a comment - Hi Kristian, I have had a look at your patch, and I have a couple of comments and questions: 1) I believe using Lob.materializeStream() to get the length of a Clob is not correct. sqlLength_ is set to the number of bytes (UTF-8 encoded), not the number characters. I wrote a small test that called ResultSet.updateClob(int,Reader) and getClob() to get it back, and Clob.length() returned more than the number of characters when there were one or more non-ascii characters. 2) EncodedInputStream uses "UTF8" and "UnicodeBigUnmarked" as encodings. These names are not necessarily understood by all JVMs. However, "UTF-8" and "UTF-16BE" (for which UTF8 and UnicodeBigUnmarked are aliases) must be supported by all JVMs, so it is probably better to use those names. 3) Clob's constructor checks whether the supplied encoding is UnicodeBigUnmarked, but I didn't find that it was called with this encoding anywhere in the code. Could that part be removed? 4) Clob's constructor checks whether the supplied encoding is US-ASCII, UTF-8 or UnicodeBigUnmarked, but if it's neither of those, no error is signalled. Wouldn't it be appropriate to throw some kind of SQLException? If so, SQLState.UNSUPPORTED_ENCODING sounds like a good choice. 5) In ByteArrayCombinerStream, I think it would be good to convert the comments which describe the instance variables to javadoc. Also, the method description seems to be missing from the constructor and read(byte[],int,int). 6) ByteArrayCombinerStreamTest is a subclass of BaseJDBCTestCase, but it's not a JDBC test. Shouldn't it have been a direct subclass of BaseTestCase? 7) (minor) I have noticed in the svn log that there is an ongoing clean-up of the JUnit tests. Among other things, setUp and tearDown methods have been made protected instead of public. Maybe setUp and tearDown in ByteArrayCombinerStreamTest should be made protected too? 8) (very minor) The condition in the while clause in ByteArrayCombinerStream.read(byte[],int,int) tests that (read != length). I feel the code would be easier to read if the test was (read < length) since that would tell the reader that the read variable is increasing in the loop. Also, it wouldn't go into an infinite loop if there's an off-by-one error. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          I did some investigation on the existing Clob implementation on the
          client. It might be relevant to the work on this issue. Here's a
          summary of my findings:

          There are three methods which return the length of the Clob:

          • length() which is supposed to return the length in characters
          • getByteLength() which is supposed to return the length in bytes
            for whatever the underlying encoding is
          • getUTF8Length() which is supposed to return the length of the Clob
            if it is UTF-8 encoded

          length() and getByteLength() always return the same value, but what
          they return is not consistent.

          getUTF8Length() only works if the Clob object was created with the
          constructor that takes a string parameter.

          If the Clob was created with the constructor that takes a Reader,
          length() and getByteLength() return the length in characters. These
          clobs are sent UTF-16BE encoded over DRDA, hence their length in bytes
          is actually twice their length in characters. getUTF8Length() will
          throw a NullPointerException (as will position() and getSubString()).

          If the Clob was created with the InputStream constructor, the encoding
          has to be specified explicitly. Three encodings are supported:
          US-ASCII, UTF-8 and UnicodeBigUnmarked (aka UTF-16BE). (In all cases,
          getUTF8Length(), position() and getSubString() will throw a
          NullPointerException.)

          US-ASCII is used when a user calls PreparedStatement.setAsciiStream()
          or ResultSet.updateAsciiStream(). In this case length() and
          getByteLength() both return the length in bytes, which is equal to the
          length in characters.

          UTF-8 is only used internally in NetStatementRequest when sending
          VARCHARs or LONGVARCHARs that are more than 32767/3 characters
          long. In this case length() and getByteLength() both return the number
          of bytes needed to represent the string in UTF-8. These Clob objects
          are never exposed to the user.

          UnicodeBigUnmarked is never passed to the constructor, but if it were,
          length() and getByteLength() would return the length in characters.


          Since the return value of the length() method already is inconsistent,
          I don't think the patch for lengthless overloads actually makes the
          situation much worse. Considering that no other methods work correctly
          on these objects anyway (see for instance DERBY-1599), I think fixing
          that issue could be done as a separate task.

          Show
          Knut Anders Hatlen added a comment - I did some investigation on the existing Clob implementation on the client. It might be relevant to the work on this issue. Here's a summary of my findings: There are three methods which return the length of the Clob: length() which is supposed to return the length in characters getByteLength() which is supposed to return the length in bytes for whatever the underlying encoding is getUTF8Length() which is supposed to return the length of the Clob if it is UTF-8 encoded length() and getByteLength() always return the same value, but what they return is not consistent. getUTF8Length() only works if the Clob object was created with the constructor that takes a string parameter. If the Clob was created with the constructor that takes a Reader, length() and getByteLength() return the length in characters. These clobs are sent UTF-16BE encoded over DRDA, hence their length in bytes is actually twice their length in characters. getUTF8Length() will throw a NullPointerException (as will position() and getSubString()). If the Clob was created with the InputStream constructor, the encoding has to be specified explicitly. Three encodings are supported: US-ASCII, UTF-8 and UnicodeBigUnmarked (aka UTF-16BE). (In all cases, getUTF8Length(), position() and getSubString() will throw a NullPointerException.) US-ASCII is used when a user calls PreparedStatement.setAsciiStream() or ResultSet.updateAsciiStream(). In this case length() and getByteLength() both return the length in bytes, which is equal to the length in characters. UTF-8 is only used internally in NetStatementRequest when sending VARCHARs or LONGVARCHARs that are more than 32767/3 characters long. In this case length() and getByteLength() both return the number of bytes needed to represent the string in UTF-8. These Clob objects are never exposed to the user. UnicodeBigUnmarked is never passed to the constructor, but if it were, length() and getByteLength() would return the length in characters. Since the return value of the length() method already is inconsistent, I don't think the patch for lengthless overloads actually makes the situation much worse. Considering that no other methods work correctly on these objects anyway (see for instance DERBY-1599 ), I think fixing that issue could be done as a separate task.
          Hide
          Kristian Waagan added a comment -

          Thanks for the review Knut Anders.
          I have uploaded revision d of patch 6.
          See my comments and answers below.

          1) You see this bug because you are able to access the Clob-object
          being used for input to the database. This is a shortcut taken in
          my implementation, based on the (faulty) assumption that the
          Clob-object is never passed to the user. Internally, the number of
          bytes are used. See comment at the bottom.

          2) Fixed. The list of supported encodings is found either in the
          package summary for java.lang (JDK < 1.4), or in the JavaDoc for
          java.nio.Charset.

          3) I have removed it from the constructor I added (which was a copy of
          the existing one), but left it as it is in the constructor that was
          already there. I think that change belong in a separate
          cleanup-patch.
          Note that I also removed the block for UTF-8 encoding, as this is
          never used for length less Clobs (encoding is always US-ASCII when
          passing an InputStream - set-/updateAsciiStream). I left the
          encoding argument for now, along with the check. This is handy
          when/if we change to ISO-8859-1. Besides from that, the encoding
          argument could be removed from this constructor
          (Agent,InputStream,String).

          4) Fixed this for the constructor I added. Again I suggest a separate
          cleanup-patch for the other constructor.

          5) Fixed.

          6) Fixed. I also removed an unused import.

          7) Fixed (deleted setUp and tearDown).

          8) Changed.

          As you can see, I have chosen not to address some of your comments on the
          code that is not directly related to my patch. I don't want to mix other
          bug fixes and improvements with my patch for this issue. One of the main
          reasons for this, is that the faulty assumption that a Clob used for
          input is not passed to the user is used for the whole implementation, not
          just mine.

          In my opinion, Clob is ready for a cleanup/rewrite and this should be
          done separately from this issue.

          Thanks,

          Show
          Kristian Waagan added a comment - Thanks for the review Knut Anders. I have uploaded revision d of patch 6. See my comments and answers below. 1) You see this bug because you are able to access the Clob-object being used for input to the database. This is a shortcut taken in my implementation, based on the (faulty) assumption that the Clob-object is never passed to the user. Internally, the number of bytes are used. See comment at the bottom. 2) Fixed. The list of supported encodings is found either in the package summary for java.lang (JDK < 1.4), or in the JavaDoc for java.nio.Charset. 3) I have removed it from the constructor I added (which was a copy of the existing one), but left it as it is in the constructor that was already there. I think that change belong in a separate cleanup-patch. Note that I also removed the block for UTF-8 encoding, as this is never used for length less Clobs (encoding is always US-ASCII when passing an InputStream - set-/updateAsciiStream). I left the encoding argument for now, along with the check. This is handy when/if we change to ISO-8859-1. Besides from that, the encoding argument could be removed from this constructor (Agent,InputStream,String). 4) Fixed this for the constructor I added. Again I suggest a separate cleanup-patch for the other constructor. 5) Fixed. 6) Fixed. I also removed an unused import. 7) Fixed (deleted setUp and tearDown). 8) Changed. As you can see, I have chosen not to address some of your comments on the code that is not directly related to my patch. I don't want to mix other bug fixes and improvements with my patch for this issue. One of the main reasons for this, is that the faulty assumption that a Clob used for input is not passed to the user is used for the whole implementation, not just mine. In my opinion, Clob is ready for a cleanup/rewrite and this should be done separately from this issue. Thanks,
          Hide
          Rick Hillegas added a comment -

          Hi Kristian: Thanks for addressing the concerns raised by Tomohito and Knut Anders. I agree that this patch should not be muddied by an overhaul of our Clob implementation and that you should log a separate JIRA to track the Clob defects identified by this discussion. I have run derbyall under 1.6 and 1.4 against jar files generated from this patch. The results are clean modulo the expected diffs in the 1.6 run. Committed at subversion revision 427112.

          Show
          Rick Hillegas added a comment - Hi Kristian: Thanks for addressing the concerns raised by Tomohito and Knut Anders. I agree that this patch should not be muddied by an overhaul of our Clob implementation and that you should log a separate JIRA to track the Clob defects identified by this discussion. I have run derbyall under 1.6 and 1.4 against jar files generated from this patch. The results are clean modulo the expected diffs in the 1.6 run. Committed at subversion revision 427112.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-7a-clientborderfix.diff' fixes bugs in the client-side implementation. The bugs caused Derby to hang if the size of the data was the same as the internal buffer, and there was also a bug that could cause an ArrayIndexOutOfBoundException.

          Comments:
          1) I have added more tests for ByteArrayCombinerStream, and also done some whitespace changes here.
          2) Added comments to ByteArrayCombinerStream.
          3) Made ByteArrayCombinerStream throw IllegalArgumentException for negative lengths and if there is not enough data for the length specified (detected in constructor). As this is an internal class, I hope this is acceptable. If not, I would like to know how these exeptional situations should be handled.

          I ran tests with Blob sizes of 1K, 32K, 65K, 10M, 100M and 2G with the client. Tests for 100M and 2G fails badly, and Derby hangs due to an OutOfMemoryError on the server. The situation is somewhat improved by the patch 'DERBY-1559.diff', but I still get ugly crashes, like broken communications pipe. I have not looked these problems.

          I ran the jdbc40 suite without failures. I will add more tests soon, but need to coordinate with other work (might add tests one of the several existing tests).

          The patch is ready for review/commit.

          Thanks,

          Show
          Kristian Waagan added a comment - 'derby-1417-7a-clientborderfix.diff' fixes bugs in the client-side implementation. The bugs caused Derby to hang if the size of the data was the same as the internal buffer, and there was also a bug that could cause an ArrayIndexOutOfBoundException. Comments: 1) I have added more tests for ByteArrayCombinerStream, and also done some whitespace changes here. 2) Added comments to ByteArrayCombinerStream. 3) Made ByteArrayCombinerStream throw IllegalArgumentException for negative lengths and if there is not enough data for the length specified (detected in constructor). As this is an internal class, I hope this is acceptable. If not, I would like to know how these exeptional situations should be handled. I ran tests with Blob sizes of 1K, 32K, 65K, 10M, 100M and 2G with the client. Tests for 100M and 2G fails badly, and Derby hangs due to an OutOfMemoryError on the server. The situation is somewhat improved by the patch ' DERBY-1559 .diff', but I still get ugly crashes, like broken communications pipe. I have not looked these problems. I ran the jdbc40 suite without failures. I will add more tests soon, but need to coordinate with other work (might add tests one of the several existing tests). The patch is ready for review/commit. Thanks,
          Hide
          Rick Hillegas added a comment -

          Thanks for these bug fixes, Kristian. My only questions about the IllegalArgumentException would be:

          a) How would a user trip across this?
          b) What would the user see?

          Thanks for including a test case for this exception.

          Show
          Rick Hillegas added a comment - Thanks for these bug fixes, Kristian. My only questions about the IllegalArgumentException would be: a) How would a user trip across this? b) What would the user see? Thanks for including a test case for this exception.
          Hide
          Kristian Waagan added a comment -

          Thanks for looking at the patch, Rick

          I added the test case mostly to demonstrate the expected behavior.

          a) The user should never trip across this at all. If it is thrown, it must be because of a programming error in Derby. Currently, the byte arrays passed in are read from a user/application stream, and the bytes are counted as they are read.

          b) The user would see something ugly... For the non-debug version, replace the linenumbers with "Unknown Source". The error is constructed.
          1) testSetClobLengthless(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)java.lang.IllegalArgumentException: Length cannot be negative: -37
          at org.apache.derby.client.am.ByteArrayCombinerStream.<init>(ByteArrayCombinerStream.java:78)
          at org.apache.derby.client.am.Lob.materializeStream(Lob.java:164)
          at org.apache.derby.client.am.Clob.materializeStream(Clob.java:833)
          at org.apache.derby.client.am.Clob.length(Clob.java:216)
          at org.apache.derby.client.net.NetStatementRequest.computeProtocolTypesAndLengths(NetStatementRequest.java:1232)
          at org.apache.derby.client.net.NetStatementRequest.buildSQLDTAcommandData(NetStatementRequest.java:520)
          at org.apache.derby.client.net.NetStatementRequest.writeExecute(NetStatementRequest.java:139)
          at org.apache.derby.client.net.NetPreparedStatement.writeExecute_(NetPreparedStatement.java:171)
          at org.apache.derby.client.am.PreparedStatement.writeExecute(PreparedStatement.java:1543)
          at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:1789)
          at org.apache.derby.client.am.PreparedStatement.executeX(PreparedStatement.java:1347)
          at org.apache.derby.client.am.PreparedStatement.execute(PreparedStatement.java:1332)
          at org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest.testSetClobLengthless(PreparedStatementTest.java:375)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
          at org.apache.derbyTesting.functionTests.util.BaseTestCase.runBare(Unknown Source)

          I realize this does not look good, but it should not happen. I don't feel like making these two exceptions checked, or just ignore the error conditions (as the previous implementation did).
          Does anyone have opinions?
          Are there guidelines to follow?

          Thanks,

          Show
          Kristian Waagan added a comment - Thanks for looking at the patch, Rick I added the test case mostly to demonstrate the expected behavior. a) The user should never trip across this at all. If it is thrown, it must be because of a programming error in Derby. Currently, the byte arrays passed in are read from a user/application stream, and the bytes are counted as they are read. b) The user would see something ugly... For the non-debug version, replace the linenumbers with "Unknown Source". The error is constructed. 1) testSetClobLengthless(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)java.lang.IllegalArgumentException: Length cannot be negative: -37 at org.apache.derby.client.am.ByteArrayCombinerStream.<init>(ByteArrayCombinerStream.java:78) at org.apache.derby.client.am.Lob.materializeStream(Lob.java:164) at org.apache.derby.client.am.Clob.materializeStream(Clob.java:833) at org.apache.derby.client.am.Clob.length(Clob.java:216) at org.apache.derby.client.net.NetStatementRequest.computeProtocolTypesAndLengths(NetStatementRequest.java:1232) at org.apache.derby.client.net.NetStatementRequest.buildSQLDTAcommandData(NetStatementRequest.java:520) at org.apache.derby.client.net.NetStatementRequest.writeExecute(NetStatementRequest.java:139) at org.apache.derby.client.net.NetPreparedStatement.writeExecute_(NetPreparedStatement.java:171) at org.apache.derby.client.am.PreparedStatement.writeExecute(PreparedStatement.java:1543) at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:1789) at org.apache.derby.client.am.PreparedStatement.executeX(PreparedStatement.java:1347) at org.apache.derby.client.am.PreparedStatement.execute(PreparedStatement.java:1332) at org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest.testSetClobLengthless(PreparedStatementTest.java:375) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at org.apache.derbyTesting.functionTests.util.BaseTestCase.runBare(Unknown Source) I realize this does not look good, but it should not happen. I don't feel like making these two exceptions checked, or just ignore the error conditions (as the previous implementation did). Does anyone have opinions? Are there guidelines to follow? Thanks,
          Hide
          Rick Hillegas added a comment -

          Thanks for the explanation, Kristian. Committed at subversion revision 427969.

          Show
          Rick Hillegas added a comment - Thanks for the explanation, Kristian. Committed at subversion revision 427969.
          Hide
          Kristian Waagan added a comment -

          'derby-1417-8a-enableblobstreaming.diff' enables streaming of BLOBs.
          This patch is small, several related changes are made as part of DERBY-1473. The patch is "kind of" independent. Before DERBY-1473 is committed, it might be possible to get some minor incorrect behavior. There are no existing tests that provoke these situations, they will be added as part of DERBY-1473.

          A committer might choose to hold off the commit until DERBY-1473 is in. Derbyall ran without failures, and the patch only affects tests streaming to BLOB columns.

          Patch ready for review/commit.

          Show
          Kristian Waagan added a comment - 'derby-1417-8a-enableblobstreaming.diff' enables streaming of BLOBs. This patch is small, several related changes are made as part of DERBY-1473 . The patch is "kind of" independent. Before DERBY-1473 is committed, it might be possible to get some minor incorrect behavior. There are no existing tests that provoke these situations, they will be added as part of DERBY-1473 . A committer might choose to hold off the commit until DERBY-1473 is in. Derbyall ran without failures, and the patch only affects tests streaming to BLOB columns. Patch ready for review/commit.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian, I tried to run derbyall with the 8a patch and the patch for DERBY-1473. I see this error:

              • End: TestQueryObject jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:50:02 ***
                          • Diff file jdbc40/jdbc40/PreparedStatementTest.diff
              • Start: PreparedStatementTest jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:51:29 ***
                0 add
                > ................................F.........
                > There was 1 failure:
                > 1) testSetBinaryStreamLengthLessOnBlobTooLong(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.ComparisonFailure: Unexpected SQL state. expected:<...CL30> but was:<...SDA4>
                > FAILURES!!!
                > Tests run: 82, Failures: 1, Errors: 0
                Test Failed.
              • End: PreparedStatementTest jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:51:34 ***
          Show
          Knut Anders Hatlen added a comment - Hi Kristian, I tried to run derbyall with the 8a patch and the patch for DERBY-1473 . I see this error: End: TestQueryObject jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:50:02 *** Diff file jdbc40/jdbc40/PreparedStatementTest.diff Start: PreparedStatementTest jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:51:29 *** 0 add > ................................F......... > There was 1 failure: > 1) testSetBinaryStreamLengthLessOnBlobTooLong(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.ComparisonFailure: Unexpected SQL state. expected:<...CL30> but was:<...SDA4> > FAILURES!!! > Tests run: 82, Failures: 1, Errors: 0 Test Failed. End: PreparedStatementTest jdk1.6.0-rc jdbc40:jdbc40 2006-08-24 00:51:34 ***
          Hide
          Kristian Waagan added a comment -

          Hi Knut Anders,

          Seems like I have forgotten to change the SQL state in the patch for DERBY-1417 (step two). Let me briefly explain why the SQL state changes when patch 8a is applied. Note that the test itself was added in DERBY-1473 (step one).

          The changes in step one added new functionality to the streaming classes. Simply put, they are now able to throw the appropariate exceptions when the stream are not the way they should be (too long, too short, truncation error). These exceptions are specific, but because the stream is passed down into the store, they are wrapped in a generic error message: XSDA4 - An unexpected exception was thrown.

          Before step two was applied, the streams were never passed down to the store, but instead materialized when SQLBlob.setWidth was called. This method has a check, which throws an exception with SQL state XCL30 - An IOException was thrown when reading a ''

          {0}

          '' from an InputStream.

          The state of the error reporting when reading from streams is not optimal. DERBY-1657 are created to track improvements.

          As soon as step one has been committed (DERBY-1473), I will upload a new patch for DERBY-1417 that also changes the SQL state in the test.
          I can't do it right now, because the code I need to change has not yet been committed. Committing DERBY-1473 first should be okay.

          Thanks,

          Show
          Kristian Waagan added a comment - Hi Knut Anders, Seems like I have forgotten to change the SQL state in the patch for DERBY-1417 (step two). Let me briefly explain why the SQL state changes when patch 8a is applied. Note that the test itself was added in DERBY-1473 (step one). The changes in step one added new functionality to the streaming classes. Simply put, they are now able to throw the appropariate exceptions when the stream are not the way they should be (too long, too short, truncation error). These exceptions are specific, but because the stream is passed down into the store, they are wrapped in a generic error message: XSDA4 - An unexpected exception was thrown. Before step two was applied, the streams were never passed down to the store, but instead materialized when SQLBlob.setWidth was called. This method has a check, which throws an exception with SQL state XCL30 - An IOException was thrown when reading a '' {0} '' from an InputStream. The state of the error reporting when reading from streams is not optimal. DERBY-1657 are created to track improvements. As soon as step one has been committed ( DERBY-1473 ), I will upload a new patch for DERBY-1417 that also changes the SQL state in the test. I can't do it right now, because the code I need to change has not yet been committed. Committing DERBY-1473 first should be okay. Thanks,
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian,

          I am not completely comfortable with the 8a patch. It might be the
          case that all callers of SQLBlob.getLength() will work correctly if a
          negative value is returned, but I have a feeling that it will work by
          accident, not by design, and that it might fail later because of
          changes in other parts of the code.

          My two main objections are

          1) SQLBlob.getLength() breaks the contract of
          DataValueDescriptor.getLength() and SQLBinary.getLength() as the
          length in bytes is not returned for parameters set with one of
          the length-less streaming methods.

          2) There will be an inconsistency between the return values from
          SQLBlob.getLength() and SQLClob.getLength(). SQLBlob will return
          a negative value when a length-less method has been used, SQLClob
          will materialize the stream and return a non-negative value.

          I propose a slightly different approach:

          a) Don't make any changes to getLength().

          b) Add a new method (say getLengthIfAvailable()) to the
          DataValueDescriptor interface, with a javadoc similar to

          /**

          • Returns the same value as getLength() if there is a simple and
          • resource-friendly way to find the value. If a call to
          • getLength() would require heavy work (for instance,
          • materializing a stream to get its length), this method is
          • allowed to return a negative value indicating that.
            */

          c) Add a default implementation of the method in DataType. The
          default implementation could just forward calls to the
          getLength() method.

          d) Override the method in SQLBlob (should be equal to the
          getLength() method you added).

          e) Identify all calls to getLength() which would cause a binary
          stream to be materialized, check if they would work correctly
          with a negative value (fix them if they wouldn't), and replace
          the call to getLength() with a call to getLengthIfAvailable().

          I believe this approach is safer. The calls to getLengthIfAvailable()
          make it explicit in the code that a negative value can be returned and
          must be handled. Also, if there is a call to getLength() that has been
          overlooked and that won't work with negative values, it will still
          work as before, materializing the stream and returning the correct
          length.

          Show
          Knut Anders Hatlen added a comment - Hi Kristian, I am not completely comfortable with the 8a patch. It might be the case that all callers of SQLBlob.getLength() will work correctly if a negative value is returned, but I have a feeling that it will work by accident, not by design, and that it might fail later because of changes in other parts of the code. My two main objections are 1) SQLBlob.getLength() breaks the contract of DataValueDescriptor.getLength() and SQLBinary.getLength() as the length in bytes is not returned for parameters set with one of the length-less streaming methods. 2) There will be an inconsistency between the return values from SQLBlob.getLength() and SQLClob.getLength(). SQLBlob will return a negative value when a length-less method has been used, SQLClob will materialize the stream and return a non-negative value. I propose a slightly different approach: a) Don't make any changes to getLength(). b) Add a new method (say getLengthIfAvailable()) to the DataValueDescriptor interface, with a javadoc similar to /** Returns the same value as getLength() if there is a simple and resource-friendly way to find the value. If a call to getLength() would require heavy work (for instance, materializing a stream to get its length), this method is allowed to return a negative value indicating that. */ c) Add a default implementation of the method in DataType. The default implementation could just forward calls to the getLength() method. d) Override the method in SQLBlob (should be equal to the getLength() method you added). e) Identify all calls to getLength() which would cause a binary stream to be materialized, check if they would work correctly with a negative value (fix them if they wouldn't), and replace the call to getLength() with a call to getLengthIfAvailable(). I believe this approach is safer. The calls to getLengthIfAvailable() make it explicit in the code that a negative value can be returned and must be handled. Also, if there is a call to getLength() that has been overlooked and that won't work with negative values, it will still work as before, materializing the stream and returning the correct length.
          Hide
          Kristian Waagan added a comment -

          Knut Anders,

          Your worries make sense. However, I think there is an easier solution. Since the only place that has to be changed to enable blob streaming is SQLBlob.setWidth, I have modified it accordingly. No changes are done to code elsewhere, and if getLength is called the stream will be materialized as before this patch. This can be handled as/if it is detected, and we might reconsider your solution.

          The new, and hopefully last patch, is 'derby-1417-9a-blobstream-newapproach.diff'. It implements a different approach and deprecates revision 8a. I also updated the single test that was failing to an incorrect SQL state in the test (see comment from 24/Aug/06 02:35 AM above).

          I ran derbyall with JDK 1.6, and the only test failing was TestQueryObject (awaiting bugfix in the JDK?).

          Thanks,

          Show
          Kristian Waagan added a comment - Knut Anders, Your worries make sense. However, I think there is an easier solution. Since the only place that has to be changed to enable blob streaming is SQLBlob.setWidth, I have modified it accordingly. No changes are done to code elsewhere, and if getLength is called the stream will be materialized as before this patch. This can be handled as/if it is detected, and we might reconsider your solution. The new, and hopefully last patch, is 'derby-1417-9a-blobstream-newapproach.diff'. It implements a different approach and deprecates revision 8a. I also updated the single test that was failing to an incorrect SQL state in the test (see comment from 24/Aug/06 02:35 AM above). I ran derbyall with JDK 1.6, and the only test failing was TestQueryObject (awaiting bugfix in the JDK?). Thanks,
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Kristian, your latest patch addresses all my concerns. I have verified that inserting a 100MB blob (-Xmx64M) without your fix gives OutOfMemoryError, and that it succeeds with your fix. Derbyall didn't show any regressions. I added a small comment in SQLBlob.setWidth() before committing.

          Committed into trunk with revision 437976.
          Committed into 10.2 with revision 437980.

          Show
          Knut Anders Hatlen added a comment - Thanks Kristian, your latest patch addresses all my concerns. I have verified that inserting a 100MB blob (-Xmx64M) without your fix gives OutOfMemoryError, and that it succeeds with your fix. Derbyall didn't show any regressions. I added a small comment in SQLBlob.setWidth() before committing. Committed into trunk with revision 437976. Committed into 10.2 with revision 437980.
          Hide
          Kristian Waagan added a comment -

          Resolving this. I don't expect to do more on work on this, and plan to close the issue in a few days.

          Show
          Kristian Waagan added a comment - Resolving this. I don't expect to do more on work on this, and plan to close the issue in a few days.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development