[EMAIL PROTECTED] writes:
> Thank you for looking at the patch :)
>
> Knut Anders Hatlen <[EMAIL PROTECTED]> writes:
>
>> Hi Dyre,
>>
>> I had a quick look at the patch, and there was one thing I didn't
>> quite understand in IndexToBaseRowNode.init(). When heapReferencedCols
>> is null, allReferencedCols is also set to null. Intuitively, I would
>> say allReferencedCols should be equal to indexReferencedCols in that
>> case (since A U NULL == A). Or maybe indexReferencedCols is always
>> null when heapReferencedCols is null?
>
> Well, intuitively I would agree with you. The thing is that I have
> tried to duplicate the logic previously found in
> java/engine/org/apache/derby/impl/sql/execute/IndexRowToBaseRowResultSet.java
>
> It seems like there is no handling of the case when
> heapReferencedCols==null but indexReferencedCols!=null. But I don't
> know if this is because it is known not to happen, or if it is an
> oversight.
>
> I just looked at the old code again, and it seems like this situation will
> cause the FormatableBitSet copy constructor to be invoked with a null
> argument. Which should cause an assert in sane mode and an NPE in
> insane mode...
Would it be better to copy this logic in the new code? If I have
understood correctly, we get the same behaviour if we remove the outer
if in init(). Apart from the obvious benefit that we get an error if
this unexpected situation occurs, I also think it would make the code
easier to read.
>> Also, in the same method, is the "return" necessary? If it isn't, I
>> think it would be better to remove it since it could easily be
>> overlooked by someone who adds more code below the if-block later.
>
> That return statement should definitely not be there. Thanks.
I have looked more at the patch. It seems correct to me, and I think
it's a good change. Some minor comments about the changes in
IndexRowToBaseRowResultSet:
* the private variable accessedIndexCols doesn't seem to be used any
more and could be removed
* indexColRefItem (parameter to constructor) is also unused and
could be removed
* it would be good if the call to getCompactRow() in the constructor
had a comment saying that it will set the field compactRow as a
side effect. It could be a little confusing to readers that we
don't care about the return value of a getter.
* couldn't the work System.arraycopy() is doing be done as part of
the for-loop which follows it? Then we wouldn't have to go through
the array twice.
--
Knut Anders
