Derby
  1. Derby
  2. DERBY-2730

Implement not implemented Embedded methods Blob.getBinaryStream(long pos, long length) and Clob. getCharacterStream(long pos, long length)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: JDBC
    • 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)

      The implementation on the Network Client is already done as part of Derby-2444

      1. GetCharacterStreamImpl_v2.stat
        0.4 kB
        V.Narayanan
      2. GetCharacterStreamImpl_v2.diff
        11 kB
        V.Narayanan
      3. GetCharacterStreamImpl_v1.stat
        0.2 kB
        V.Narayanan
      4. GetCharacterStreamImpl_v1.diff
        9 kB
        V.Narayanan
      5. GetBinaryStreamImpl_v2.stat
        0.2 kB
        V.Narayanan
      6. GetBinaryStreamImpl_v2.diff
        8 kB
        V.Narayanan
      7. GetBinaryStreamImpl_v1.stat
        0.2 kB
        V.Narayanan
      8. GetBinaryStreamImpl_v1.diff
        8 kB
        V.Narayanan
      9. GetBinaryStreamImpl_v1_NOT_FOR_COMMIT.stat
        0.4 kB
        V.Narayanan
      10. GetBinaryStreamImpl_v1_NOT_FOR_COMMIT.diff
        24 kB
        V.Narayanan

        Issue Links

          Activity

          Hide
          V.Narayanan added a comment -

          All patches submitted and committed. Indicated change going into another jira.
          Hence dlosing this issue!

          Show
          V.Narayanan added a comment - All patches submitted and committed. Indicated change going into another jira. Hence dlosing this issue!
          Hide
          V.Narayanan added a comment -

          I am OK with this kristian.

          Show
          V.Narayanan added a comment - I am OK with this kristian.
          Hide
          Kristian Waagan added a comment -

          Unless I get wild protests, I will change the following line in the ClobUpdateableReader constructor as below:
          + maxPos = Math.min(clob.length(), pos + len);
          to be simply
          maxPos = pos + len;

          The reason is that the clob.length() call can be very expensive. At this point, we might have already exhausted the stream two times. Doing it even once more is unnecessary in this case:

          • First of all the check is already done in EmbedClob.getCharacterStream(long,long). This must be done to comply with the JDBC spec.
          • Second, even if the former test was removed, ClobUpdateableReader would behave properly if maxPos turns out to be set to a value larger then the actual size of the Clob. The underlying stream would simply return -1, which is exactly what the maxPos variable is used for anyway.

          The change will go in with another change in a yet-to-be-created-Jira (I will create a link to it).

          Show
          Kristian Waagan added a comment - Unless I get wild protests, I will change the following line in the ClobUpdateableReader constructor as below: + maxPos = Math.min(clob.length(), pos + len); to be simply maxPos = pos + len; The reason is that the clob.length() call can be very expensive. At this point, we might have already exhausted the stream two times. Doing it even once more is unnecessary in this case: First of all the check is already done in EmbedClob.getCharacterStream(long,long). This must be done to comply with the JDBC spec. Second, even if the former test was removed, ClobUpdateableReader would behave properly if maxPos turns out to be set to a value larger then the actual size of the Clob. The underlying stream would simply return -1, which is exactly what the maxPos variable is used for anyway. The change will go in with another change in a yet-to-be-created-Jira (I will create a link to it).
          Hide
          V.Narayanan added a comment -

          Thank you for the commit Bernt !!

          Show
          V.Narayanan added a comment - Thank you for the commit Bernt !!
          Hide
          Bernt M. Johnsen added a comment -

          Patch ok.
          Committed revision 547203.

          Show
          Bernt M. Johnsen added a comment - Patch ok. Committed revision 547203.
          Hide
          V.Narayanan added a comment -

          Thank you for the review on the patch.

          I found a clue on how to handle this from already exisiting
          usage of the other Updatable stream constructors.

          I removed the try catch in the updatable streams and
          moved them into the corresponding Lob classes where
          they were being used. Here I called the Util.setStreamFailure
          passing the IOException as parameter.

          I ran the jdbc4/BlobTest, jdbc4/ClobTest and jdbcapi/BlobClob4BlobTest
          and saw no failures.

          I have started a junit All run and shall revert back with results.

          Show
          V.Narayanan added a comment - Thank you for the review on the patch. I found a clue on how to handle this from already exisiting usage of the other Updatable stream constructors. I removed the try catch in the updatable streams and moved them into the corresponding Lob classes where they were being used. Here I called the Util.setStreamFailure passing the IOException as parameter. I ran the jdbc4/BlobTest, jdbc4/ClobTest and jdbcapi/BlobClob4BlobTest and saw no failures. I have started a junit All run and shall revert back with results.
          Hide
          V.Narayanan added a comment -

          I will fix this Bernt! Thanx for the review.

          Show
          V.Narayanan added a comment - I will fix this Bernt! Thanx for the review.
          Hide
          Bernt M. Johnsen added a comment -

          ClobUpdateableReader (EmbedClob clob, long pos, long len) throws an SQLException but the javadoc has @throws IOException. Same goes for UpdateableBlobStream (i missed it in the previous review).

          Also: in both constructors (ClobUpdateableReader and UpdateableBlobStream), you do
          SQLException sqle = new SQLException();
          sqle.initCause(ioe);
          throw sqle;
          I.e.: an SQLException with no message and no sql state is thrown. I think it should have both.

          Show
          Bernt M. Johnsen added a comment - ClobUpdateableReader (EmbedClob clob, long pos, long len) throws an SQLException but the javadoc has @throws IOException. Same goes for UpdateableBlobStream (i missed it in the previous review). Also: in both constructors (ClobUpdateableReader and UpdateableBlobStream), you do SQLException sqle = new SQLException(); sqle.initCause(ioe); throw sqle; I.e.: an SQLException with no message and no sql state is thrown. I think it should have both.
          Hide
          V.Narayanan added a comment -

          I ran junit All on this patch and found no failures.

          Show
          V.Narayanan added a comment - I ran junit All on this patch and found no failures.
          Hide
          V.Narayanan added a comment -

          Pls find the implementation of Clob.getCharacterStream(long, long). The logic
          followed is very similar to the getBinaryStream(long, long) implementation.

          I am running junit All on this patch and shall revert back with the test results.

          Show
          V.Narayanan added a comment - Pls find the implementation of Clob.getCharacterStream(long, long). The logic followed is very similar to the getBinaryStream(long, long) implementation. I am running junit All on this patch and shall revert back with the test results.
          Hide
          V.Narayanan added a comment -

          Thanks a ton for the commit Bernt !!

          Show
          V.Narayanan added a comment - Thanks a ton for the commit Bernt !!
          Hide
          Bernt M. Johnsen added a comment -

          getBinaryStream ok.
          Committed revision 546838.

          Show
          Bernt M. Johnsen added a comment - getBinaryStream ok. Committed revision 546838.
          Hide
          V.Narayanan added a comment -

          I have run tests on GetBinaryStreamImpl_v2.diff and saw no failures. If everything is OK I request
          for this patch to be considered for a commit.

          Show
          V.Narayanan added a comment - I have run tests on GetBinaryStreamImpl_v2.diff and saw no failures. If everything is OK I request for this patch to be considered for a commit.
          Hide
          V.Narayanan added a comment -

          The getCharacterStream(long, long) implementation is blocked by
          the changes in Derby-2712.

          Pls note that this is only for the getCharacterStream implementation.

          Show
          V.Narayanan added a comment - The getCharacterStream(long, long) implementation is blocked by the changes in Derby-2712. Pls note that this is only for the getCharacterStream implementation.
          Hide
          V.Narayanan added a comment -

          I have made the following changes in the follow-up

          1) I have initialized maxPos to -1 in the constructor that
          does not accept length as a parameter.
          2) I have updated the javadoc to say the following

          • Construct an <code>UpdateableBlobStream<code> using the
          • <code>InputStream</code> received as parameter. The initial
          • position in the stream is set to <code>pos</code> and the
          • stream is restricted to a length of <code>len</code>.

          Pls do mention if this is OK

          I ran BlobClob4BlobTest without failures and have started a junit All run.

          Sorry about the delayed test runs. I shall post the results as soon as the test
          completes.

          Show
          V.Narayanan added a comment - I have made the following changes in the follow-up 1) I have initialized maxPos to -1 in the constructor that does not accept length as a parameter. 2) I have updated the javadoc to say the following Construct an <code>UpdateableBlobStream<code> using the <code>InputStream</code> received as parameter. The initial position in the stream is set to <code>pos</code> and the stream is restricted to a length of <code>len</code>. Pls do mention if this is OK I ran BlobClob4BlobTest without failures and have started a junit All run. Sorry about the delayed test runs. I shall post the results as soon as the test completes.
          Hide
          V.Narayanan added a comment -

          I had to initialize maxPos to -1 in the constructor of UpdateableBlobStream
          that does not accept length as parameter. I was planning to produce a
          follow-up
          just now. I will correct this issue along with the maxPos one. Thank you
          for
          taking a look at this.

          Show
          V.Narayanan added a comment - I had to initialize maxPos to -1 in the constructor of UpdateableBlobStream that does not accept length as parameter. I was planning to produce a follow-up just now. I will correct this issue along with the maxPos one. Thank you for taking a look at this.
          Hide
          Bernt M. Johnsen added a comment -

          Awkward first sentence in javadoc for new UpdateableBlobStream constructor. The rest is ok.

          Show
          Bernt M. Johnsen added a comment - Awkward first sentence in javadoc for new UpdateableBlobStream constructor. The rest is ok.
          Hide
          V.Narayanan added a comment -

          I just found out that there was a problem in the machine I was running
          tests. But I will anyway run
          tests once more today to re-affirm the that there are no problems in the
          implementation.

          Show
          V.Narayanan added a comment - I just found out that there was a problem in the machine I was running tests. But I will anyway run tests once more today to re-affirm the that there are no problems in the implementation.
          Hide
          V.Narayanan added a comment -

          The tests I had started on Saturday did not complete, I have started a
          new test run today. The tests seemed to be
          hanging and from the output of junit.textui.TestRunner I was not able to
          affirm which test was hanging and also
          if it was due to my change.

          I will run tests again and revert back with the results today. I
          Apologize for this unexpected delay.

          Show
          V.Narayanan added a comment - The tests I had started on Saturday did not complete, I have started a new test run today. The tests seemed to be hanging and from the output of junit.textui.TestRunner I was not able to affirm which test was hanging and also if it was due to my change. I will run tests again and revert back with the results today. I Apologize for this unexpected delay.
          Hide
          V.Narayanan added a comment -

          Derby-2711 has been committed. I have removed these changes from
          the patch previously marked as not for commit.

          I ran the modified BlobTest and it ran without failures.

          I have started a junit All run and shall revert back with
          the results.

          Pls consider this patch for reviews and comments

          Show
          V.Narayanan added a comment - Derby-2711 has been committed. I have removed these changes from the patch previously marked as not for commit. I ran the modified BlobTest and it ran without failures. I have started a junit All run and shall revert back with the results. Pls consider this patch for reviews and comments
          Hide
          V.Narayanan added a comment -

          Thanks again for the comments, I shall make the changes as soon as Derby-2711 is committed and submit the patch.

          Show
          V.Narayanan added a comment - Thanks again for the comments, I shall make the changes as soon as Derby-2711 is committed and submit the patch.
          Hide
          Bernt M. Johnsen added a comment -

          I saw no white-space or layout issues. By "clean patch" in my comment, I meant a stand-alone patch independent of other non-committed changes.

          Show
          Bernt M. Johnsen added a comment - I saw no white-space or layout issues. By "clean patch" in my comment, I meant a stand-alone patch independent of other non-committed changes.
          Hide
          V.Narayanan added a comment -

          Thank you for the reviews and comments.

          > 1) Looks fair enough, although I will not consider the review done until after DERBY-2711 is committed and a clean patch submitted.

          The Derby-2711 changes will not exist when I submit a follow-up patch once Derby-2711
          is committed.

          You have mentioned clean patch, so I just wanted to ensure that you did
          feel unhappy with the alignment, non-presence of whit-space diffs and other general
          standard requirements of clean code.

          Pls do mention if the code is falling in quality in any of these and
          I will correct them duly.

          > 2) There's no explanation why Clob.getCharacterStream isn't a part of this patch.

          I am sorry I should have mentioned this in the comments.

          Derby-2712 seems to be making changes to the wrapper in the same way
          Derby-2711 does. I could therefore not pull-out a patch as I have done in the
          case of Derby-2711 and make changes in way of a review patch since any
          changes to current classes would have had a conflict from the patch for Derby-2712.

          Can someone familiar with Derby-2712 pls tell me if the assumption I have made in the
          above comment is correct?

          Show
          V.Narayanan added a comment - Thank you for the reviews and comments. > 1) Looks fair enough, although I will not consider the review done until after DERBY-2711 is committed and a clean patch submitted. The Derby-2711 changes will not exist when I submit a follow-up patch once Derby-2711 is committed. You have mentioned clean patch, so I just wanted to ensure that you did feel unhappy with the alignment, non-presence of whit-space diffs and other general standard requirements of clean code. Pls do mention if the code is falling in quality in any of these and I will correct them duly. > 2) There's no explanation why Clob.getCharacterStream isn't a part of this patch. I am sorry I should have mentioned this in the comments. Derby-2712 seems to be making changes to the wrapper in the same way Derby-2711 does. I could therefore not pull-out a patch as I have done in the case of Derby-2711 and make changes in way of a review patch since any changes to current classes would have had a conflict from the patch for Derby-2712. Can someone familiar with Derby-2712 pls tell me if the assumption I have made in the above comment is correct?
          Hide
          Bernt M. Johnsen added a comment -

          1) Looks fair enough, although I will not consider the review done until after DERBY-2711 is committed and a clean patch submitted.
          2) There's no explanation why Clob.getCharacterStream isn't a part of this patch.

          Show
          Bernt M. Johnsen added a comment - 1) Looks fair enough, although I will not consider the review done until after DERBY-2711 is committed and a clean patch submitted. 2) There's no explanation why Clob.getCharacterStream isn't a part of this patch.
          Hide
          V.Narayanan added a comment -

          I have used the patch attached to the issue Derby-2711 to implement this patch. This patch
          hence contains the changes introduced by Derby-2711 also.

          Pls note that this patch is not for a commit.

          M java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java

          Added implementation for the getBinaryStream(long position, long length) method
          that wraps the stream returned from the getBinaryStream method inside the
          UpdateableBlobStream, using the constructor that accepts a position
          and length as parameter, to return a subset of the Blob value.

          A java/engine/org/apache/derby/impl/jdbc/UpdateableBlobStream.java

          Added a constructor that accepts position and length as parameter. Modified
          the read methods to use the restriction on the position and length of the
          stream.

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/BlobUpdateableStreamTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/_Suite.java

          The other changes are related to Derby-2711

          Pls consider this patch for reviews and comments

          Show
          V.Narayanan added a comment - I have used the patch attached to the issue Derby-2711 to implement this patch. This patch hence contains the changes introduced by Derby-2711 also. Pls note that this patch is not for a commit. M java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java Added implementation for the getBinaryStream(long position, long length) method that wraps the stream returned from the getBinaryStream method inside the UpdateableBlobStream, using the constructor that accepts a position and length as parameter, to return a subset of the Blob value. A java/engine/org/apache/derby/impl/jdbc/UpdateableBlobStream.java Added a constructor that accepts position and length as parameter. Modified the read methods to use the restriction on the position and length of the stream. M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobTest.java A java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/BlobUpdateableStreamTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/_Suite.java The other changes are related to Derby-2711 Pls consider this patch for reviews and comments
          Hide
          V.Narayanan added a comment -

          DERBY-2711 introduces a wrapper class UpdateableBlobStream which shall be used
          to return a subset of the underlying Blob object to the user.

          Show
          V.Narayanan added a comment - DERBY-2711 introduces a wrapper class UpdateableBlobStream which shall be used to return a subset of the underlying Blob object to the user.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development