Restructure code for Blob/Clob length in client to prepare for locator implementation

• Key: DERBY-2540
• Type: Sub-task
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Øystein Grøvlen
• Reporter: Øystein Grøvlen
• Component/s: Network Client
• Fix Version/s: 10.3.1.4

Description

In order to prepare for the locator-based implementation, I want to restructure the code for getting the length of LOBs.
Currently, the LOB class has a protected field sqlLength_ that will contain the length of the LOB, if known. Currently, it is always known as long as the LOB has been materialized. However, when locators are introduced, the length may have to be fetched from the server the first time it is needed. With the current implementation, where sqlLength_ is accessed directly by many classes in the package, it will become very difficult to keep track of whether one needs to fetch the length from the server or not.

I will change the implementation to make Lob.sqlLength_ private and add access methods to get and set its value. (A good thing in itself IMHO). If the length is unknown, the LOB will be materialized to get the length.

Comments

[Knut Anders Hatlen]

This sounds like a good idea! I don't know if it's relevant for this issue, but I think the sqlLength_ variable sometimes holds the byte count and sometimes holds the character count for a CLOB. I don't remember the all the details, but I once summarized what I found in this JIRA comment: https://issues.apache.org/jira/browse/DERBY-1417#action_12424055

[Øystein Grøvlen]

Knut Anders, thanks for the reference to your summary of length
issues. For this patch I do not intend to change any behavior here.

However, I will propose to remove Clob.getByteLength() and its
associated field. The method is not used by any Derby code and it is,
as far as I can tell, not working correctly. In your summary, you
write that getByteLength() gives the same result as length(). This is
initially true, but if the length of the Clob is changed (e.g., by
truncate()), getByteLength() will still return the original value.
In addition, I do not understand how this associated comment applies to
the current implementation:

    // this method is primarily for mixed clob length calculations.
    // it was introduced to prevent recursion in the actual char
    // length calculation

Given all this, I think it is better to just remove it.

[Øystein Grøvlen]

The attached patch contains the following modifications:

M java/client/org/apache/derby/client/am/Lob.java
   * Made sqlLength_ and lengthObtained_ private
   * Moved logic from Clob.length() and Blob.length() to sqlLength()
     for checking whether length is already obtained and if not,
     obtain the length by materializing the stream.
   * checkForClosedConnection() was moved to Blob.length() since Clob
     does not currently have the same behavior with respect to closed
     connections, and I do not want to change the behavior with this
     patch. That will come in later patches. Also, it should not be
     necessary to this check for closed connections every time the
     length is needed, only when this is a user-initiated operation.
   * Added method setSqlLength() to be used to update the length.
   * Added abstract method materializeStream() so that subclasses can
     do the necessary setup before calling the existing
     materializeStream().

M java/client/org/apache/derby/client/am/Blob.java
   * Replaced all operations on sqlLength_ with calls to the
     corresponding mehtods in Lob.
   * Replace calls to this.length() with calls to Lob.sqlLength()
     since length() is primarily a function to be called by users, and
     contains code that is not necessary for internal usage.
   * Removed some try-catch blocks that is no longer needed since
     length() is not used.
   * Implemted the new materializeStream() method.

M java/client/org/apache/derby/client/am/Clob.java
   * Same changes as described for Blob.
   * Remove lengthInBytes_ and getByteLength() as discussed earlier.

M java/client/org/apache/derby/client/am/BlobOutputStream.java
M java/client/org/apache/derby/client/am/ClobOutputStream.java
M java/client/org/apache/derby/client/am/ClobWriter.java
   * Replaced all operations on sqlLength_ with calls to the
     corresponding mehtods in Lob.
   * Refactored common code into a private method to be used by the
     other methods. This way, I did not have to repeat the same
     changes over and over again.

[Øystein Grøvlen]

Forgot to mention that I have run the Junit All suite with no new failures.
I am currently running derbyall. I do not except any new failures since I ran it successfuly earlier on an almost identical version of the patch.

[Dag H. Wanvik]

I'll have a look at this one.

[Dag H. Wanvik]

I think this patch is a good incremental cleanup. I ran derbyall and
suites.All with (0,2) failures respectively, none related.
Just some minor nits:

1) Lob#sqlLength: The method throws SqlException if Layer B streaming
   is used. The javadoc is not clear on this point ("unless Layer B
   streaming is used"). Would be good to move this "unless"-comment to the
   @throws tag.

2) Lob#materializeStream: Javadoc says "Method to be implemented by
   subclasses to do the necessary setup before calling
   #materializedStream(InputStream, String)". It seems neither
   Blob.java nor Clob.java does any set-up *before* calling
   #materializedStream(InputStream, String). Maybe it would be better
   to say just "Method to be implemented by subclasses, which in
   addition to calling #materializedStream(InputStream, String) will
   do any setup specific to that subclass".

   It also lacks a @throws SqlException tag.

3) Blob#materializeStream: A @throw SqlException is missing

4) Clob#materializeStream: A @throw SqlException is missing

5) Clob#length: It seems this method will no longer be checking for
   closed connection; is this correct? Maybe you can
   explain why this change is ok, it seems from the comments this
   is intended.

6) BlobOutputStream#writeX: It seems an arraycopy could be used for
   the second part of the copy operation as well (not introduced by
   this patch, though, only refactored, but I thought I'd mention it).

[Øystein Grøvlen]

Dag, thanks for the review. I have addressed your comments below, and
I will upload a new patch soon,

> Dag H. Wanvik commented on DERBY-2540:
> --------------------------------------
>
> I think this patch is a good incremental cleanup. I ran derbyall and
> suites.All with (0,2) failures respectively, none related.
> Just some minor nits:
>
> 1) Lob#sqlLength: The method throws SqlException if Layer B streaming
> is used. The javadoc is not clear on this point ("unless Layer B
> streaming is used"). Would be good to move this "unless"-comment to the
> @throws tag.

Good point. Will change the comment.

>
> 2) Lob#materializeStream: Javadoc says "Method to be implemented by
> subclasses to do the necessary setup before calling
> #materializedStream(InputStream, String)". It seems neither
> Blob.java nor Clob.java does any set-up *before* calling
> #materializedStream(InputStream, String). Maybe it would be better
> to say just "Method to be implemented by subclasses, which in
> addition to calling #materializedStream(InputStream, String) will
> do any setup specific to that subclass".

What it actually does it to call it with the right parameters and
assign the result to the right stream. I will update the javadoc to
reflect that.

>
> It also lacks a @throws SqlException tag.
>

Will fix.

>
> 3) Blob#materializeStream: A @throw SqlException is missing
>

Will fix.

> 4) Clob#materializeStream: A @throw SqlException is missing

Will fix.

>
> 5) Clob#length: It seems this method will no longer be checking for
> closed connection; is this correct? Maybe you can
> explain why this change is ok, it seems from the comments this
> is intended.

Clob#length earlier only checked for closed connections if the length
was not already obtained (i.e., the Clob had been materialized). I
could have checked for closed connections in Lob#sqlLength, but the
problem is Blob and Clob currently behave differently. Blob always
checks, Clob only if length needs to be obtained. Hence, if I put the
check in Lob#sqlLength, I would have had to change tests. I wanted to
avoid that since the behavior will soon change again when locators are
introduced. (I guess this means that getting the length of a not
materialized Clob after the connection is closed is not currently
tested.)

>
> 6) BlobOutputStream#writeX: It seems an arraycopy could be used for
> the second part of the copy operation as well (not introduced by
> this patch, though, only refactored, but I thought I'd mention it).
>

I agree that arraycopy seems a better choice, but I do not think I
want to change this code as part of this patch. Also, I do not think
BlobOutputStream will be in much use going forward since new Stream
classes will be made for locator based Blobs.

[Øystein Grøvlen]

Attached patch, derby-2540_2.diff, addresses review comments.

[Dag H. Wanvik]

Thanks, Øystein!
I ran the regression tests over again cleanly with JDK1.6.

Committed at svn 530070.