[
https://issues.apache.org/jira/browse/DERBY-2998?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12572260#action_12572260
]
A B commented on DERBY-2998:
----------------------------
Hi Thomas, thanks for your continued work on this.
> Sorry about that :|
Not a problem :)
I haven't had a chance to look at the failing queries that you refer to in
test7, but I did take a look at patch 15. Some comments/questions after my
first run through are below.
1) There are a lot of places with logic similar to the following:
ResultColumn rc = (ResultColumn) resultColumns.elementAt(index);
if (rc.getExpression() instanceof WindowFunctionColumnNode)
{
...
}
Since WindowFunctionColumnNode extends ResultColumn, I wonder if it would be
worth it to add a new "isWindowColumn()" method (or something like that) to
ResultColumn.java and let that method contain all of the necessary logic for
finding a WindowFunctionColumnNode. If that was done then the above type of
"if" expression becomes simply:
ResultColumn rc = (ResultColumn) resultColumns.elementAt(index);
if (rc.isWindowColumn())
{
...
}
which makes it easier to extend the check for "window columns" in the future if
needed. For example, would it ever be necessary to walk further down the
ResultColumn's expression chain to look for a RowNumber column? That is, could
we ever have:
ResultColumn
-> VirtualColumnNode
-> ResultColumn
-> ...
-> RowNumberColumnNode
If so, then the "isWindowColumn()" as defined in ResultColumn.java could
include the necessary logic to walk the chain, which would simplify the calling
code. See, for example, comments #5 and #6 below.
2)
In WindowNode.bind() there is the following:
/* At this stage there is hopefully a PRN below us as the fromList */
return this;
It might be good to add an ASSERT here to make sure that the 'hopefully'
condition is satisified...
3)
Many of the fields in WindowNode (and other new classes added by this patch)
are "public", but some of them are never referenced outside of WindowNode.
Unless a "public" declaration is functionally necessary, it seems like it would
be safer to make them private and then supply the necessary getter/setter
methods where needed, to avoid accidental modification.
4)
In WindowNode.java there is a field "level" which is initialized to -1 and then
is set from SelectNode.java. But as far as I can tell there are no comments
explaining what that field does. I think I was able to figure it out from the
code, but it'd be nice to have explanatory comments within WindowNode itself.
And per comment #3 above, I think it'd be nice to use a setter method in
SelectNode instead of direct assignment to the public field.
5)
In ColumnReference.java there are two different kinds of checks to see if the
ColumnReference is pointing to a WindowColumnNode. The first is via the method
"pointsToWindowFunction()", which does an instanceof check on the source
ResultColumn's expression and returns true if it is a WindowFunctionColumnNode.
The second is in the categorize() method, where the logic only evaluates to
true if the source ResultColumn's expression is a *VirtualColumnNode* whose
source ResultColumn's expression is a WindowFunctionColumnNode. So the checks
do not appear to be equivalent. Is that intentional? If so, then explanatory
comments might be good here. Also, I believe this is a good example of the
kind of code that might benefit from an "isWindowColumn()" function on
ResultColumn, per comment #1 above.
6)
In ResultColumnList.java, there is the following:
/* Window function columns are added by the WindowResultSet */
if (sourceExpr instanceof WindowFunctionColumnNode ||
(sourceExpr instanceof VirtualColumnNode &&
(sourceExpr.getSourceResultColumn().getExpression()
instanceof WindowFunctionCumnNode)))
{
continue;
}
See comment #1 above for an alternate (cleaner?) approach...
7)
The result set statistics node for WindowResultSet does not currently print out
the name of the result set...(I don't think). This is pretty minor, but I was
briefly confused when I saw the RealWindowResultSetStatistics class and yet the
query plans weren't showing anything "window" releated.
8)
If I'm understanding correctly, the execution-time logic for WindowResultSet
operates in a fashion similar to ProjectRestrictResultSet. That is, it
iterates through all of its source's rows and, for each one, it applies a
restriction. If the row fails the restriction, then the iteration moves on to
the next source row, and so on. This agrees with the following comments,
pulled from the getNextRowCore() method of WindowResultSet:
/*
* Loop until we get a row from the source that qualifies, or there are
* no more rows to qualify. For each iteration fetch a row from the
* source, and evaluate against the restriction if any.
*/
If this is correct, then I'm not entirely sure we'll get the performance boost
that one might perhaps expect from a query like:
SELECT * FROM (
SELECT row_number() over () as r, t.* FROM T
) AS tmp WHERE r <= 3;
I made a similar comment on Februrary 5th that was based on materialization of
the source result set, and that comment ended up being wrong because I
misunderstood materialization. But in looking at this query again, I think
something might still be slightly sub-optimal here.
If we take the approach of "loop until we get a row that qualifies", then we
will loop through _all_ of the rows in table T, even though only the first 3
satisfy the restriction. In the end we will correctly return the first 3 rows
(and only the first 3 rows) but not before we've read all of the others from
disk and applied the "<= 3" restriction to each one. This can be observed by
either a) adding debug printouts to WindowResultSet.getNextRowCore() for every
call to "source.getNextRowCore()", or b) looking at the query plan. For the
above query, the plan shows the following for the top-most
ProjectRestrictResultSet:
******* Project-Restrict ResultSet (1):
Number of opens = 1
Rows seen = 1280
Rows filtered = 1277
restriction = true
So we actually read all 1280 rows from disk, then filtered 1277 of them out so
that, in the end, we only returned 3 rows. Thus from a performance perspective
the ROW_NUMBER() function does not appear to improve things. Is that a known
limitation of the current development (which, for the record, would be
perfectly acceptable)?
Having said that, there is absolutely _NO_ need for you to boost performance
before committing the changes :) Incremental development is good and it's
great to have the functionality working. Performance improvements can always
be addressed later, if you (or anyone else) so chooses. I'm just raising the
issue to make sure that I understand how things are working.
9)
For the query mentioned in comment #8 above, the query plan shows:
ProjectRestrictResultSet (sees ALL rows, restricts down to 3)
-> ProjectRestrictResultSet (sees ALL rows)
-> WindowResultSet (sees ALL rows)
I intuitively expected the restriction to appear at the level of the
WindowResultSet, as opposed to appearing at the level of the top-most PRN. The
plan as shown above makes it look like the restriction is not being enforced by
the Window ResultSet, but is instead being enforced by the ProjectRestrict. Is
that correct? Or is this just a case where the query plan fails to capture
what is really happening with the WindowResultSet? I'm guessing the latter,
but am not sure...
> Add support for ROW_NUMBER() window function
> --------------------------------------------
>
> Key: DERBY-2998
> URL: https://issues.apache.org/jira/browse/DERBY-2998
> Project: Derby
> Issue Type: Sub-task
> Components: SQL
> Reporter: Thomas Nielsen
> Assignee: Thomas Nielsen
> Priority: Minor
> Attachments: d2998-10.diff, d2998-10.stat, d2998-11.diff,
> d2998-12.diff, d2998-12.stat, d2998-13.diff, d2998-13.stat, d2998-14.diff,
> d2998-14.stat, d2998-15.diff, d2998-15.stat, d2998-4.diff, d2998-4.stat,
> d2998-5.diff, d2998-5.stat, d2998-6.diff, d2998-6.stat, d2998-7.diff,
> d2998-7.stat, d2998-8.diff, d2998-8.stat, d2998-9-derby.log, d2998-9.diff,
> d2998-9.stat, d2998-doc-1.diff, d2998-doc-1.stat, d2998-test.diff,
> d2998-test.stat, d2998-test2.diff, d2998-test2.stat, d2998-test3.diff,
> d2998-test3.stat, d2998-test4.diff, d2998-test4.stat, d2998-test6.diff,
> d2998-test7.diff
>
>
> As part of implementing the overall OLAP Operations features of SQL
> (DERBY-581), implement the ROW_NUMBER() window function.
> More information about this feature is available at
> http://wiki.apache.org/db-derby/OLAPRowNumber
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
