Derby
  1. Derby
  2. DERBY-2444

Implement not implemented methods Blob.getBinaryStream(long pos, long length) and Clob. getCharacterStream(long pos, long length) in the Network Client

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: Network Client
    • Labels:
      None

      Description

      The following methods were introduced in the java.sql.Clob and java.sql.Blob interface as part of JDBC 4.0 and need to be implemented.

      Clob
      ------

      getCharacterStream(long pos, long length)

      Blob
      ------

      getBinaryStream(long pos, long length)

      1. ClobBlobNotImplemented_v1.diff
        13 kB
        V.Narayanan
      2. ClobBlobNotImplemented_v1.stat
        0.4 kB
        V.Narayanan
      3. ClobBlobNotImplemented_v2.diff
        13 kB
        V.Narayanan
      4. ClobBlobNotImplemented_v2.stat
        0.4 kB
        V.Narayanan
      5. ClobBlobNotImplemented_v3.diff
        14 kB
        V.Narayanan
      6. ClobBlobNotImplemented_v3.stat
        0.5 kB
        V.Narayanan
      7. ClobBlobNotImplemented_v4.diff
        22 kB
        V.Narayanan
      8. ClobBlobNotImplemented_v4.stat
        0.6 kB
        V.Narayanan
      9. ClobBlobNotImplemented_v5.diff
        2 kB
        V.Narayanan
      10. ClobBlobNotImplemented_v5.stat
        0.3 kB
        V.Narayanan

        Activity

        Hide
        V.Narayanan added a comment -

        Add the tests to the jdbc4 suite

        M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
        M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java

        Added the methods to the Blob and Clob implementations in the am interface

        M java/client/org/apache/derby/client/am/Blob.java
        M java/client/org/apache/derby/client/am/Clob.java

        Tests Run
        ---------------

        ran derbyall and junit and no failures were there.

        Show
        V.Narayanan added a comment - Add the tests to the jdbc4 suite M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ClobTest.java Added the methods to the Blob and Clob implementations in the am interface M java/client/org/apache/derby/client/am/Blob.java M java/client/org/apache/derby/client/am/Clob.java Tests Run --------------- ran derbyall and junit and no failures were there.
        Hide
        V.Narayanan added a comment -

        I tried applying this patch to a freshworkspace. It did not show an conflicts with existing files while applying. I had also run the tests as indicated while submitting this patch. I request for this patch to be considered by a interested person for reviews, comments and if everything is OK a commit too.

        Show
        V.Narayanan added a comment - I tried applying this patch to a freshworkspace. It did not show an conflicts with existing files while applying. I had also run the tests as indicated while submitting this patch. I request for this patch to be considered by a interested person for reviews, comments and if everything is OK a commit too.
        Hide
        Knut Anders Hatlen added a comment -

        The patch looks good. Some minor comments/questions:
        1) Blob.getBinaryStream(long,long) creates a copy of the internal byte array and wraps it in a ByteArrayInputStream. Would it be better to use the ByteArrayInputStream constructor which takes three arguments (array, offset and length) and pass in a direct reference to binaryString_? This could reduce the memory footprint (except in the case where the Blob could be garbage collected before the stream). To achieve the same thing for Clob.getCharacterStream(), I think we would have to implement something like the LimitReader class in the engine code, but that's not required for this issue. Just throwing out some thoughts...
        2) Blob.getBinaryStream and Clob.getCharacterStream have almost identical code for checking position and length. Would it be possible to move that code into a common method in the base class (Lob)?
        3) The test code uses five blanks for indentation (should be four).
        4) The javadoc comments for getCharacterStream and getBinaryStream exceed 80 characters per line (as do a couple of lines in the methods' bodies).

        Show
        Knut Anders Hatlen added a comment - The patch looks good. Some minor comments/questions: 1) Blob.getBinaryStream(long,long) creates a copy of the internal byte array and wraps it in a ByteArrayInputStream. Would it be better to use the ByteArrayInputStream constructor which takes three arguments (array, offset and length) and pass in a direct reference to binaryString_? This could reduce the memory footprint (except in the case where the Blob could be garbage collected before the stream). To achieve the same thing for Clob.getCharacterStream(), I think we would have to implement something like the LimitReader class in the engine code, but that's not required for this issue. Just throwing out some thoughts... 2) Blob.getBinaryStream and Clob.getCharacterStream have almost identical code for checking position and length. Would it be possible to move that code into a common method in the base class (Lob)? 3) The test code uses five blanks for indentation (should be four). 4) The javadoc comments for getCharacterStream and getBinaryStream exceed 80 characters per line (as do a couple of lines in the methods' bodies).
        Hide
        V.Narayanan added a comment -

        Thank you for you comments. The patch addresses the issues you have pointed.

        Show
        V.Narayanan added a comment - Thank you for you comments. The patch addresses the issues you have pointed.
        Hide
        V.Narayanan added a comment -

        I ran ClobTest and BlobTest and both of them passed.

        Show
        V.Narayanan added a comment - I ran ClobTest and BlobTest and both of them passed.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for the updated patch, Narayanan!

        I looked at the javadoc for Blob.getBinaryStream(long,long) at http://java.sun.com/javase/6/docs/api/. It says

        Throws:
        SQLException - if pos is less than 1 or if pos is greater than the number of bytes in the Blob or if pos + length is greater than the number of bytes in the Blob

        (The wording is similar for Clob.getCharacterStream(long,long).)

        I don't think that matches the current checks. For instance, Lob.checkPosition() throws exception if (pos > length() + 1) whereas the it should throw exception if (pos > length()) . And checkLength() only checks whether length is negative. It would probably be good to add tests for these boundary cases in BlobTest/ClobTest as well.

        I don't think this part of getBinaryStream() is correct:
        + InputStream retVal = new java.io.ByteArrayInputStream
        + (binaryString_, (int)(pos), (int)length);

        The pos argument starts on 1, whereas the offset argument to ByteArrayInputStream starts on 0. The tests in BlobTest passed, though, which I found a bit puzzling. It seems there is a dataOffset_ variable in Blob which, by accident, was one when running the tests, so the bug wasn't discovered. I believe pos should be replaced with (dataOffset_ + pos - 1).

        Show
        Knut Anders Hatlen added a comment - Thanks for the updated patch, Narayanan! I looked at the javadoc for Blob.getBinaryStream(long,long) at http://java.sun.com/javase/6/docs/api/ . It says Throws: SQLException - if pos is less than 1 or if pos is greater than the number of bytes in the Blob or if pos + length is greater than the number of bytes in the Blob (The wording is similar for Clob.getCharacterStream(long,long).) I don't think that matches the current checks. For instance, Lob.checkPosition() throws exception if (pos > length() + 1) whereas the it should throw exception if (pos > length()) . And checkLength() only checks whether length is negative. It would probably be good to add tests for these boundary cases in BlobTest/ClobTest as well. I don't think this part of getBinaryStream() is correct: + InputStream retVal = new java.io.ByteArrayInputStream + (binaryString_, (int)(pos), (int)length); The pos argument starts on 1, whereas the offset argument to ByteArrayInputStream starts on 0. The tests in BlobTest passed, though, which I found a bit puzzling. It seems there is a dataOffset_ variable in Blob which, by accident, was one when running the tests, so the bug wasn't discovered. I believe pos should be replaced with (dataOffset_ + pos - 1).
        Hide
        V.Narayanan added a comment -

        Thank you for the comments. Yes you are correct. I should have looked deeper into this. I am sorry, will fix this.

        There was a message on the comment notification email from JIRA that said you could reply to this email to place comments and they will appear in your issue. I did that and the comments did not come. Guess I made a mistake somewhere.

        Show
        V.Narayanan added a comment - Thank you for the comments. Yes you are correct. I should have looked deeper into this. I am sorry, will fix this. There was a message on the comment notification email from JIRA that said you could reply to this email to place comments and they will appear in your issue. I did that and the comments did not come. Guess I made a mistake somewhere.
        Hide
        Knut Anders Hatlen added a comment -

        The comment is added to JIRA if you reply to jira@apache.org, but the
        mailing list software rewrites the reply-to field and so that your
        reply went directly to derby-dev. So it wasn't your fault!

        This mail was sent to jira@apache.org, so it will hopefully end up in
        JIRA.


        Knut Anders

        Show
        Knut Anders Hatlen added a comment - The comment is added to JIRA if you reply to jira@apache.org, but the mailing list software rewrites the reply-to field and so that your reply went directly to derby-dev. So it wasn't your fault! This mail was sent to jira@apache.org, so it will hopefully end up in JIRA. – Knut Anders
        Hide
        V.Narayanan added a comment -
        • I have checked for the cases pointed out as part of the
          comments to this issue.
        • I removed the previous two methods checkPosition
          and checkLength from Lob class and added one
          method checkPosAndLength that checks for the
          position and length input parameters.
        • To throw an exception in the case when pos+length>length
          of LOB I created a new SQLState.
        • I have to add the tests for the cases when the exception are
          thrown. I will do this in a subsequent submission.

        Pls consider this patch for reviews and comments.

        Show
        V.Narayanan added a comment - I have checked for the cases pointed out as part of the comments to this issue. I removed the previous two methods checkPosition and checkLength from Lob class and added one method checkPosAndLength that checks for the position and length input parameters. To throw an exception in the case when pos+length>length of LOB I created a new SQLState. I have to add the tests for the cases when the exception are thrown. I will do this in a subsequent submission. Pls consider this patch for reviews and comments.
        Hide
        V.Narayanan added a comment -

        I haven't yet run tests on v3. I will post the test results tommorrow along with the tests that need to be run for the cases mentioned in the comments.

        Show
        V.Narayanan added a comment - I haven't yet run tests on v3. I will post the test results tommorrow along with the tests that need to be run for the cases mentioned in the comments.
        Hide
        V.Narayanan added a comment -

        I have addressed the comments pointed out in this issue

        The following can be highlighted with respect to this patch

        • pos has been replaced with (dataOffset_ + pos - 1) in the getBinaryStream(long, long)
          method implementation in am/Blob.
        • added a checkPosAndLength method in the Lob class that verifies that pos and length
          of getCharacterStream(long, long) and getBinaryStream(long, long) satisfy the following

        a) pos <= 0
        b) pos > (length of LOB)
        c) length < 0
        d) pos > (length of LOB)

        • Added tests to jdbc4/BlobTest and jdbc4/ClobTest that check for these conditions

        I have run derbyall and junit all and have the following failures that I can find in the tinderbox also.

        In my junit run
        --------------------

        1) testMiscExpressions(org.apache.derbyTesting.functionTests.tests.lang.GroupByExpressionTest)java.sql.SQLSyntaxErrorException: The SELECT list of a grouped query contains at least one invalid expression. If a SELECT list has a GROUP BY, the list may only contain valid grouping expressions and valid aggregate expressions.

        There was 1 failure:
        1) SecurityPolicyReloadingTest( )junit.framework.AssertionFailedError: Policy file changed. Should not be able to read the property.

        In my derbyall run
        -------------------------
        derbyall/derbyall.fail:lang/grantRevokeDDL.sql

        I request for this patch to be considered for reviews and comments and if everything is ok a commit too.

        Show
        V.Narayanan added a comment - I have addressed the comments pointed out in this issue The following can be highlighted with respect to this patch pos has been replaced with (dataOffset_ + pos - 1) in the getBinaryStream(long, long) method implementation in am/Blob. added a checkPosAndLength method in the Lob class that verifies that pos and length of getCharacterStream(long, long) and getBinaryStream(long, long) satisfy the following a) pos <= 0 b) pos > (length of LOB) c) length < 0 d) pos > (length of LOB) Added tests to jdbc4/BlobTest and jdbc4/ClobTest that check for these conditions I have run derbyall and junit all and have the following failures that I can find in the tinderbox also. In my junit run -------------------- 1) testMiscExpressions(org.apache.derbyTesting.functionTests.tests.lang.GroupByExpressionTest)java.sql.SQLSyntaxErrorException: The SELECT list of a grouped query contains at least one invalid expression. If a SELECT list has a GROUP BY, the list may only contain valid grouping expressions and valid aggregate expressions. There was 1 failure: 1) SecurityPolicyReloadingTest( )junit.framework.AssertionFailedError: Policy file changed. Should not be able to read the property. In my derbyall run ------------------------- derbyall/derbyall.fail:lang/grantRevokeDDL.sql I request for this patch to be considered for reviews and comments and if everything is ok a commit too.
        Hide
        V.Narayanan added a comment -

        The last check should have been pos+length > (length of LOB) in the above comment. Sorry for the typo.

        Show
        V.Narayanan added a comment - The last check should have been pos+length > (length of LOB) in the above comment. Sorry for the typo.
        Hide
        V.Narayanan added a comment -

        Also it should not have been "satisfy the following" but rather "do not violate the following". Sorry for the multiple comments.

        Show
        V.Narayanan added a comment - Also it should not have been "satisfy the following" but rather "do not violate the following". Sorry for the multiple comments.
        Hide
        Knut Anders Hatlen added a comment -

        Thank you for addressing my comments, Narayanan! I think the patch looks good, and I will commit it when my test run has completed.

        One tiny nit (if you want to fix it, it is OK to do it as a follow-up). I think the check for (pos > this.length()) in Lob.checkPosAndLength() is redundant. Either (length < 0) or ((pos + length) > this.length()) must be true if (pos > this.length()), so an exception is thrown regardless of that check.

        Oh, and one more... There is a slight possibility that (pos+length) overflows and goes negative. For instance, a valid position combined with a length of Long.MAX_VALUE will not be detected as an error by checkPosAndLength(), I think. Changing the last check to (length > this.length() - pos) would eliminate the possibility for an overflow.

        Show
        Knut Anders Hatlen added a comment - Thank you for addressing my comments, Narayanan! I think the patch looks good, and I will commit it when my test run has completed. One tiny nit (if you want to fix it, it is OK to do it as a follow-up). I think the check for (pos > this.length()) in Lob.checkPosAndLength() is redundant. Either (length < 0) or ((pos + length) > this.length()) must be true if (pos > this.length()), so an exception is thrown regardless of that check. Oh, and one more... There is a slight possibility that (pos+length) overflows and goes negative. For instance, a valid position combined with a length of Long.MAX_VALUE will not be detected as an error by checkPosAndLength(), I think. Changing the last check to (length > this.length() - pos) would eliminate the possibility for an overflow.
        Hide
        Knut Anders Hatlen added a comment -

        Committed ClobBlobNotImplemented_v4.diff with revision 528546.

        Show
        Knut Anders Hatlen added a comment - Committed ClobBlobNotImplemented_v4.diff with revision 528546.
        Hide
        V.Narayanan added a comment -

        Thank you for the commit Knut. I will submit a follow-up patch for the issue you have pointed.

        Show
        V.Narayanan added a comment - Thank you for the commit Knut. I will submit a follow-up patch for the issue you have pointed.
        Hide
        V.Narayanan added a comment -

        Addressed all the issues ppointed out. Also made changes to the test since the SQLState after the change in the caes where we test whether pos > length would be different. Changed the tests to test for the new SQLState.

        Since the new methods are tested only in jdbc4/BlobTest and jdbc4/ClobTest I have just run these two tests.

        I have however started the junit all run to not miss any failures that might occur. Will revert back with the results as soon as the test finishes.

        Pls consider this patch for reviews and comments.

        Show
        V.Narayanan added a comment - Addressed all the issues ppointed out. Also made changes to the test since the SQLState after the change in the caes where we test whether pos > length would be different. Changed the tests to test for the new SQLState. Since the new methods are tested only in jdbc4/BlobTest and jdbc4/ClobTest I have just run these two tests. I have however started the junit all run to not miss any failures that might occur. Will revert back with the results as soon as the test finishes. Pls consider this patch for reviews and comments.
        Hide
        V.Narayanan added a comment -

        The junit All suite ran fine. There was just one failure which is already being seen in the tinderbox runs.

        1) testDSReference(org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest)java.lang.NullPointerException
        at org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest.assertDataSourceReference(DataSourceReferenceTest.java:190)
        at org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest.testDSReference(DataSourceReferenceTest.java:143)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:88)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
        at junit.extensions.TestSetup.run(TestSetup.java:23)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
        at junit.extensions.TestSetup.run(TestSetup.java:23)
        at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
        at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
        at junit.extensions.TestSetup.run(TestSetup.java:23)

        I request for this patch to be considered for a commit.

        Show
        V.Narayanan added a comment - The junit All suite ran fine. There was just one failure which is already being seen in the tinderbox runs. 1) testDSReference(org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest)java.lang.NullPointerException at org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest.assertDataSourceReference(DataSourceReferenceTest.java:190) at org.apache.derbyTesting.functionTests.tests.jdbcapi.DataSourceReferenceTest.testDSReference(DataSourceReferenceTest.java:143) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:88) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) I request for this patch to be considered for a commit.
        Hide
        Knut Anders Hatlen added a comment -

        Thank you for addressing my comments Narayanan! I will take a look at the patch.

        Show
        Knut Anders Hatlen added a comment - Thank you for addressing my comments Narayanan! I will take a look at the patch.
        Hide
        Knut Anders Hatlen added a comment -

        Committed v5 with revision 529987.

        Show
        Knut Anders Hatlen added a comment - Committed v5 with revision 529987.

          People

          • Assignee:
            V.Narayanan
            Reporter:
            V.Narayanan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development