db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bryan Pendleton <bpendle...@amberpoint.com>
Subject Re: [jira] Commented: (DERBY-796) jdbc 4.0 specific Blob and Clob method support
Date Tue, 14 Feb 2006 21:11:10 GMT
Thanks for calling my attention to the diff; I am glad I read it
because I hadn't really been paying attention to this thread.

Regarding what I think to be the basic concepts of this patch:
  - using a factory to encapsulate the JDBC 4.0-specific class creation
  - implementing the get/set Blob/Clob methods in the JDBC 4.0
    classes as appropriate
I think that I understand the overall changes and they seem
reasonable to me.

I had two substantive comments on the patch:

1) It would be good to have a comment explaining why this
    change was made:
-        if (offset_ > (blob_.binaryString_.length - blob_.dataOffset_)) {
+        if ((offset_-1) > (blob_.binaryString_.length - blob_.dataOffset_)) {
It appears to be just an off-by-one error in the previous code, but
I couldn't from trivial inspection understand why it needed to be changed,
what test(s) covered this case, etc.

2) This change kind of scared me:
--- java/client/org/apache/derby/client/am/Blob.java	(revision 377622)
+++ java/client/org/apache/derby/client/am/Blob.java	(working copy)
@@ -267,7 +267,10 @@

      public int setBytesX(long pos, byte[] bytes, int offset, int len) throws SqlException
          int length = 0;
-        if ((int) pos <= 0) {
+        boolean insertIntoEmpty = false;
+	if(pos==1 && binaryString_.length==0)
+	    insertIntoEmpty = true;
+        if ((int) pos <= 0 || ((!insertIntoEmpty) && (pos > binaryString_.length
- dataOffset_))) {
              throw new SqlException(agent_.logWriter_,
                  new MessageId(SQLState.BLOB_BAD_POSITION), new Long(pos));

It seemed like the essence of this change was something like:

   "It's usually an error to pass a 'pos' value greater than the
   length of the blob, but if the blob is currently empty then
   there is a special case where the caller passes 1, not 0, as
   you might expect."

But I couldn't figure out what the change was doing and why it was
there, particularly since the change to the "if" statement seemed
to duplicate the very next "if" statement in 'setBytesX()'.

Could you please expand on why this change to setBytes was necessary?

I also had a couple very small additional comments on this patch,
which you could feel free to ignore.

1) The change to
has a bunch of probably unnecessary whitespace diffs (removal of
blank lines). I'm glad you added the class-level comment, though.

2) java/client/org/apache/derby/jdbc/ClientDataSource.java seemed
to be changed more than, strictly speaking, it needed to be, but
I think it's just a style thing. The diff seemed to introduce the
local variable "con" unnecessarily.

3) I wasn't clear on the various parts of the diff that read:

-        catch ( SqlException se )
+        catch (SqlException se)

This seemed to happen several times, and didn't seem to add
much value to the diff, in my opinion. Similarly, changes like

-    throws SQLException {
+                throws SQLException {


-    public void setSQLXML(String parameterName, SQLXML xmlObject) throws SQLException {
+    public void setSQLXML(String parameterName, SQLXML xmlObject)
+           throws SQLException {

seemed pointless, but harmless.



View raw message