|
Kathey Marsden made changes - 28/Apr/08 10:33 PM
Sebastian made changes - 28/Apr/08 10:43 PM
Interestingly the code does not correctly restrict group by expression if the expression is not in the select list.
Even with the check in place, I can CREATE FUNCTION R() returns double external name 'java.lang.Math.random' language java parameter style java; ij> select count(*) from test group by r(); 1 ----------- 1 Which I don't think I am supposed to be able to do. The functional test does select r(), count(*) from test group by r() so gets the expected error.
Kathey Marsden made changes - 28/Apr/08 10:50 PM
Yes, I believe that VerifyAggregateExpressionsVisitor visits the
select's "result column list", and for each component of that list, verifies that it is either a valid aggregate function, or a valid grouping column. From memory, I thought that in your example: select count(*) from test group by r() the "r()" expression should have been "generated" into the result column list (look at the places where ResultColumn.markGenerated gets called to see how that process occurs). Generating the group by expression into the select's RCL should have then resulted in the Verify visitor encountering it, so I'm not sure why your example isn't being caught, but maybe this provides some ideas to investigate further. It seems to only add it to the generated columns if it is an instance of ColumnReference.
In GroupByList see ~line 177 if (! matchFound && groupingCol.getColumnExpression() instanceof ColumnReference) { // only add matching columns for column references not // expressions yet. See I am wondering if VerifyAggregateExpressionsVisitor is even the right place to check if the group by expression is a function. It seems to know only about the select columns. Maybe we should be checking elsewhere entirely. Attached is an experimental patch for this issue. I have not run tests or added regression tests yet, but it passes for the repro and disallows group by function as expected I believe. I moved the check to GroupByNode.bindStatement() where I could check the node type of the group by expression. Please let me know if this approach looks ok.
Kathey Marsden made changes - 29/Apr/08 02:36 AM
GroupByNode.bindStatement() definitely seems like a natural
and appropriate location for this sort of check. Thanks for working on this problem, Kathey! Attached is a patch for this issue. derbyall and suites.All ran fine except for some strange network server timeouts that don't seem to be related to this change as I get them without my changes as well.
I saw one file merge on svn update so I will rerun tests, but this patch is ready for review. Moves the check for routine in group by expression from VerifyAggregateExpressionsVistor to GroupByList.bindGroupByColumns. Kathey
Kathey Marsden made changes - 29/Apr/08 11:11 PM
Kathey Marsden made changes - 29/Apr/08 11:11 PM
Kathey Marsden made changes - 02/May/08 11:14 PM
Rick Hillegas made changes - 04/Aug/08 08:24 PM
Kathey Marsden made changes - 19/Nov/08 04:59 PM
Myrna van Lunteren made changes - 04/May/09 06:22 PM
Dag H. Wanvik made changes - 30/Jun/09 03:55 PM
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DERBY-883.http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/VerifyAggregateExpressionsVisitor.java?p2=%2Fdb%2Fderby%2Fcode%2Ftrunk%2Fjava%2Fengine%2Forg%2Fapache%2Fderby%2Fimpl%2Fsql%2Fcompile%2FVerifyAggregateExpressionsVisitor.java&p1=%2Fdb%2Fderby%2Fcode%2Ftrunk%2Fjava%2Fengine%2Forg%2Fapache%2Fderby%2Fimpl%2Fsql%2Fcompile%2FVerifyAggregateExpressionsVisitor.java&r1=437070&r2=437069&view=diff&pathrev=437070
The change I think was intended to prevent group by java function, but disallows java functions with aggregate arguments.
Removing the code when node instanceof JavaToSQLValueNode, corrects the problem but also allows group by function.
Looking for a better way to check for that.