Thanks for reviewing the patch, Dag!
> Hi Knut, thanks for this cleanup! I hope this can go in along with
DERBY-5749 into 10.9 (both concern truncation diagnostics).
If it's completed in time for 10.9, perhaps the warnings should be
mentioned in the
DERBY-5749 release note? Might be easier to explain
the two together in one release note than in two separate notes.
> - If value is lob, the call to first getLength, then to getValue, can
> cause us the risk of havin to read a lob twice. If we call getValue
> first, we can save the call to getLength, no?
> Or, in all of SQLBit, SQLVarBit and SQLBlob, the actual width is
> actually available at the call site, so it could be passed in
> instead of doing another getLength? But maybe the lob is already in
> memory, so LOB reading cannot take place here.. if so, a comment
> that the getLength should be cheap would be nice.
I think the getLength() calls would typically be cheap because, as you
observed, it has already been called before truncate(), and length is
in most cases cached. The exception seems to be if the SQLBlob wraps a
java.sql.Blob instance, in which case Blob.length() will be called
both times, and there's no guarantee that the Blob class in question
(which could be a user-defined class) is caching the length.
I'll change the signature of truncate() to take the original length as
a parameter, as you suggested, to avoid the extra getLength() calls.
> - I think we could use the less specialized class ObjectOutputStream
> instead of FormatIdOutputStream here for "cs". The write method
> isn't overriden by FormatIdOutputStream, though, but it made me wary
> at first, lest extra bytes were written. writeUTF needs
> ObjectOutputStream, not FormatIdOutputStream.
That was my first thought, too. However, ObjectOutputStream inserts
some extra bytes at block boundaries (typically, after flush() has
been called, or when some internal block size limit is exceeded),
which made it a bit tricky to count the number of bytes. It might be
possible to compensate for this with some extra logic, but I think
that would just complicate the code for little added benefit.
It's easy to get confused with all the different base-classes and
interfaces with similar names here:
- writeUTF() takes an ObjectOutput, not an ObjectOutputStream
- FormatIdOutputStream extends DataOutputStream, not
- DataOutputStream does not implement the ObjectOutput interface,
and can therefore not be passed to writeUTF()
- FormatIdOutputStream, which is a specialization of
DataOutputStream, does implement the ObjectOutput interface
So of the three candidates
- ObjectOutputStream can be passed to writeUTF(), but gives the
wrong number of bytes
- DataOutputStream gives the right number of bytes, but cannot be
passed to writeUTF()
- FormatIdOutputStream gives the right number of bytes, and it can
be passed to writeUTF()
> - /**
> * Check the results for the queries in testDataTruncation().
> private void checkDataTruncationResult(Statement s, String sql)
> Might be good to document the expected row behavior: 1, 0 and 2
> warnings for rows 1,2 and three respectively, and 3, 3 and 4 in
> actual dataSize in the Javadoc.
Will copy that info into the javadoc.
> - checkDataTruncationResult(s,
> "values (cast('abc' as clob(2)), cast('de' as varchar(2)))," +
> " (cast('fg' as clob(2)), cast('hi' as varchar(2)))," +
> " (cast('jkl' as clob(2)), cast('mnop' as varchar(2)))");
> Why not clob(2) in column 2 on these rows? Copy/paste error?
Yes, the intention was to test CLOB(2) in both columns. Will fix.
> - DRDAConnThread
> Is the code path
> private int getSqlCode(int severity)
> if (severity == CodePoint.SVRCOD_WARNING) // warning
> return 100; //CLI likes it
> still relevant now that you test "instanceof SQLWarning"?
I don't think this code is reachable anymore now that we test for
instanceof SQLWarning. SQL code 100 is interpreted as end-of-data by
the client, so I don't think that code is correct in the first place.
(The end-of-data warnings are generated/sent via another code path
from DRDAConnThread.doneData(), and should still end up with SQL code
100 on the client, so I don't expect this change to cause any kind of
Although it's now unreachable code, I'd prefer to address that in a
separate issue, as I think the entire getSqlCode() method needs to be
fixed. Currently, it returns SQL code -1 for all exceptions, which
makes getErrorCode() return different values on client and embedded.
DERBY-5773 for this.