db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Knut Anders Hatlen <Knut.Hat...@Sun.COM>
Subject Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode
Date Fri, 12 Jan 2007 15:25:10 GMT
Dyre.Tjeldvoll@Sun.COM writes:

> Hi again, thank you for the comments.
>
> Knut Anders Hatlen <Knut.Hatlen@Sun.COM> writes:
>
>> 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.
>
> I could have sworn that I added that outer if-else at some point to
> avoid a "may not have been initialized" warning. However, I can now
> remove it without seeing that warning, so I'll do that.

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
>
> True.

A related question: Could we also remove the index cols object from
the list of saved objects?

>>   * 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.
>
> Hmm, yes maybe, but this is really an issue with the
> implementation of getCompactRow() in BasicNoPutResultSetImpl. 
> compactRow is both returned and stored in a
> variable accessible to derived classes. And derived classes seem to be
> the primary users of this method. So I guess I would rather see
> BasicNoPutResultSetImpl fixed... but that did not seem to fit in the
> scope of this patch.

I agree that this is an issue with the getCompactRow() implementation
and that it's outside the scope of this patch to fix it. But I think
it was easier to understand the old code since it explicitly set
compactRow. Just adding a very short comment would increase the
readability in my opinion:

    // sets the value of compactRow
    getCompactRow(...);

It could be only me, but when I see a getter called like a void
method, I immediately start wondering whether something is wrong. A
simple comment would assure me that everything is alright, and I
wouldn't have to spend time digging into getCompactRow() to find out
what's going on. :)

>>   * 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.
>
> Yes it could. I think I did it that way because the profiler showed that
> manual copy was expensive. I was hoping that arraycopy + writing nulls
> would be cheaper, but it is admittedly less intuitive. 

Well, I wouldn't say it is less intuitive. Your approach is probably
easier to read than if all the logic were inside the for-loop. Since
using standard library functions for such operations is a good thing,
and you also looked at it in a profiler, I have no objections to the
approach.

-- 
Knut Anders

Mime
View raw message