You are viewing a plain text version of this content. The canonical link for it is here.

Posted to derby-dev@db.apache.org by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org> on 2007/01/10 12:08:27 UTC

[jira] Created: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Move column bitset computation to IndexToBaseRowNode
----------------------------------------------------

                 Key: DERBY-2226
                 URL: https://issues.apache.org/jira/browse/DERBY-2226
             Project: Derby
          Issue Type: Improvement
            Reporter: Dyre Tjeldvoll
         Assigned To: Dyre Tjeldvoll
            Priority: Trivial


The constructor for IndexRowToBaseRowResultSet
takes a bitset describing the columns coming from the heap and a
bitset describing the columns coming from the index. But in every
IndexRowToBaseRowResultSet one also has to compute _all_ referenced
columns (union of heap and index bitsets), and frequently also those
columns _only_ coming from the heap (set difference between heap and
index).

But the value of these "set products" seem _only_ to depend on objects
that are fixed at compile time (in IndexToBaseRowNode), so it would be 
cleaner (and possibly more efficient) to compute these products there.


-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] Resolved: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.

     [ https://issues.apache.org/jira/browse/DERBY-2226?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Knut Anders Hatlen resolved DERBY-2226.
---------------------------------------

       Resolution: Fixed
    Fix Version/s: 10.3.0.0

Thank you for addressing my comments, Dyre. Patch v2 looks good. Committed revision 496645.

> Move column bitset computation to IndexToBaseRowNode
> ----------------------------------------------------
>
>                 Key: DERBY-2226
>                 URL: https://issues.apache.org/jira/browse/DERBY-2226
>             Project: Derby
>          Issue Type: Improvement
>            Reporter: Dyre Tjeldvoll
>         Assigned To: Dyre Tjeldvoll
>            Priority: Trivial
>             Fix For: 10.3.0.0
>
>         Attachments: derby-2226.v1.diff, derby-2226.v1.stat, derby-2226.v2.diff, derby-2226.v2.stat
>
>
> The constructor for IndexRowToBaseRowResultSet
> takes a bitset describing the columns coming from the heap and a
> bitset describing the columns coming from the index. But in every
> IndexRowToBaseRowResultSet one also has to compute _all_ referenced
> columns (union of heap and index bitsets), and frequently also those
> columns _only_ coming from the heap (set difference between heap and
> index).
> But the value of these "set products" seem _only_ to depend on objects
> that are fixed at compile time (in IndexToBaseRowNode), so it would be 
> cleaner (and possibly more efficient) to compute these products there.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by Dy...@Sun.COM.

Knut Anders Hatlen <Kn...@Sun.COM> writes:
>>>   * 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?

I think so. At least I have now removed it from the
ActivationClassBuilder that's used in generate(...)

But it cannot be removed from IndexToBaseRowNode since it is used for
other purposes as well.

>
>>>   * 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. :)

I've added a comment that will be part of the next patch...

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

Well, I have changed it now :) 

-- 
dt

However, experience shows that for many people and many applications a
dose of paranoia is reasonable - Bjarne Stroustrup

Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by Knut Anders Hatlen <Kn...@Sun.COM>.

Dyre.Tjeldvoll@Sun.COM writes:

> Hi again, thank you for the comments.
>
> Knut Anders Hatlen <Kn...@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

Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by Dy...@Sun.COM.

Hi again, thank you for the comments.

Knut Anders Hatlen <Kn...@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.

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

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

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

-- 
dt

Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by Knut Anders Hatlen <Kn...@Sun.COM>.

Dyre.Tjeldvoll@Sun.COM writes:

> Thank you for looking at the patch :)
>
> Knut Anders Hatlen <Kn...@Sun.COM> 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

Re: [jira] Commented: (DERBY-2226) Move column bitset computation to IndexToBaseRowNode

Posted by Dy...@Sun.COM.

Thank you for looking at the patch :)

Knut Anders Hatlen <Kn...@Sun.COM> 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...

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

-- 
dt