Hi Manish,
I have to be honest and say that I haven't taken the time to fully understand
the "guts" of this patch--namely, the part where you rewrite the group-by tree
in GroupByNode.addAggregateColumns(). But I did do some review of the overall
patch and I have the following comments. If you can address these I think the
resultant patch will be a lot easier to read (and will also be smaller :) so
that I or anyone else reviewing can focus more on the "guts" of what this patch
is really doing...
------------------------------------------------------------------
0) Patch doesn't apply cleanly with latest codeline--there appear to be
conflicts with the JUnit test changes and also one conflict in sqlgrammar.jj. I
manually resolved these conflicts in my local codeline so that I could run some
tests/view the code, but I think it'd be appropriate to post an updated patch
(sync'd with the latest codeline) to ease the review process for others.
1) TernarnyOpNode.isEquivalent(): I think we need to check "receiver"
equivalence as well, in addition to left and right operands. For example, I did
the following and I received an error as expected:
ij> create table s1 (vc varchar(20), vc2 varchar(10));
0 rows inserted/updated/deleted
ij> insert into s1 values ('allo', 'bye'), ('inigo', 'montoya'),
('six','fingers');
3 rows inserted/updated/deleted
ij> select substr(vc, 3, 4) from s1 group by vc2;
ERROR 42Y30: The SELECT list of a grouped query contains at least one invalid
expression. If a SELECT list has a GROUP BY, the list may only contain valid
grouping expressions and valid aggregate expressions.
I then I did the following, expecting to see the same error--but this actually
works:
ij> select substr(vc, 3, 4) from s1 group by substr(vc2, 3, 4);
1
----
igo
lo
x
3 rows selected
I think this is because TernaryOperatorNode.isEquivalent() doesn't compare
"receiver" and thus thinks that vc and vc2 are the same, hence allowing the
group by. Is this supposed to work or is it supposed to throw an error? My
understanding is that it should fail, but I could of course be wrong...
2) Aren't we losing information with this change?
In VerifyAggregateExpressionsVisitor.java:
- throw
StandardException.newException(SQLState.LANG_INVALID_COL_REF_GROUPED_SELECT_LIST,
cr.getSQLColumnName());
+ throw
StandardException.newException(SQLState.LANG_INVALID_GROUPED_SELECT_LIST);
Before your changes we see the column reference name:
ij> select substr(vc, 3, 4) from s1 group by vc2;
ERROR 42Y36: Column reference 'VC' is invalid. For a SELECT list with a GROUP
BY, the list may only contain grouping columns and valid aggregate expressions.
But after the patch is applied, we lose the name of the invalid column
reference:
ij> select substr(vc, 3, 4) from s1 group by vc2;
ERROR 42Y30: The SELECT list of a grouped query contains at least one invalid
expression. If a SELECT list has a GROUP BY, the list may only contain valid
grouping expressions and valid aggregate expressions.
Generally it seems like a good idea to provide the user with as much information
as possible for error conditions like this. Is it possible to preserve the
result column name in cases like this (i.e. cases where we do actually know what
the invalid column reference is)?
3) In UnaryOperatorNode.java the check for equivalence of the operand has three
parts to it:
+ return (operator.equals(other.operator)
+ || (operand == other.operand)
+ || ((operand != null) && operand.isEquivalent(other)));
But for other classes--such as BinaryOperatorNode.java--the check is only a
single call to "isEquivalent" for each operand:
+ return methodName.equals(other.methodName)
+ && leftOperand.isEquivalent(other.leftOperand)
+ && rightOperand.isEquivalent(other.rightOperand);
Can you explain why the operand for UnaryOperatorNode has three checks
(.equals(), "==", and .isEquivalent()) while the operands for BinaryOperatorNode
only have a single check (.isEquivalent())? Some comments to describe the
difference might be useful in these classes.
3) I think there are several places in this patch where you have comments
explaining "what" by not explaining "why"--and I think the latter would be
helpful. For example:
In VerifyAggregateExpressionsVisitor.java:
Why do we disallow expressions involving native java computation in the
following code?
+ } else if (node instanceof JavaToSQLValueNode)
+ {
+ // disallow any expression which involves native java computation.
+ throw StandardException.newException( (groupByList == null) ?
+ SQLState.LANG_INVALID_NON_GROUPED_SELECT_LIST :
+ SQLState.LANG_INVALID_GROUPED_SELECT_LIST);
Why do we not visit children under subqueries in the following code?
/**
- * Don't visit children under an aggregate
+ * Don't visit children under an aggregate, subquery or any node which
+ * is equivalent to any of the group by expressions.
In SelectNode.java:
Why do we need to add a call to preprocess in the following code?
+ if (groupByList != null)
+ {
+ groupByList.preprocess(numTables, fromList, whereSubquerys,
wherePredicates);
+ }
+
For this one you can probably just use the comments that you already included in
your patch description, namely: "This is needed because expressions can get
rewritten in the select list but not in the grouping list causing problems when
the result set is generated." It'd be great if this explanation was included in
the actual code.
In GroupByNode.java:
Why do we always say that the source is not in sorted order if we have an
expression instead of a ColumnReference?
- crs[index] = gc.getColumnReference();
+ if (gc.getColumnExpression() instanceof ColumnReference)
+ {
+ crs[index] = (ColumnReference)gc.getColumnExpression();
+ }
+ else
+ {
+ isInSortedOrder = false;
+ break;
+ }
Also, in the comment you posted to DERBY-883 with the patch you explained how
the rewrite is different now:
o the rewrite logic is a bit different now. Earlier, we would change
each unaggregate ColumnReference in the select list and point it to the
GroupByColumn. Now we replace each group by expression that we find
in the projection list with a virtual column node that effectively points
to a result column in the result set doing the group by. In addition
the original routine which did the rewrite is now split up into two separate
and smaller routines: addUnAggColumns and addAggregateColumns.
It would be great if this explanation could be added to the relevant parts in
the code and/or if the comments for that section could be updated to match the
new behavior. This will make it easier for people to understand what is going
on when they read the code several years from now.
In GroupByList.java:
Why don't we need to add a ResultColumn/ColumnReference pair to SelectNode's RCL
when the grouping column is an expression (as opposed to a ColumnReference), as
in the following code?
@@ -185,7 +174,8 @@
/* If no match found in the SELECT list, then add a matching
* ResultColumn/ColumnReference pair to the SelectNode's RCL.
*/
- if (! matchFound)
+ if (! matchFound &&
+ groupingCol.getColumnExpression() instanceof ColumnReference)
{
ResultColumn newRC;
These are just some examples; I think there are others too. If you can look
again at the patch and try to add comments explaining *why* we do or do not need
to do things like this, that would be a huge help to those reading the code now
and in the future. These don't have to be extensive--even just a line or two
can be very helpful.
4) In GroupByColumn and GroupByList you have deleted a bunch of methods that
apparently are unused. While this is a useful thing to do, I don't know if we
want to do that as part of the DERBY-883 patch--it makes the diff bigger and
doesn't add to the patch functionally. Instead you could file a new Jira entry
and attach a separate patch that removes the unnecessary methods to that Jira
issue, to be reviewed/committed separately. Or you could create a follow-up
patch for this issue that just separates out the changes for removing these methods.
5) Extraneous diff in QueryTreeNode.java? Are any of these changes actually
required or are they just there for debugging? If they are an intentional part
of the patch then a) comments explaining these changes would be helpful, and b)
it might be good to actually delete the commented out lines altogether, instead
of leaving them in to clutter the code.
6) New methods could use javadoc explaining in more detail what they do:
GroupByNode.addUnAggColumns()
GroupByNode.addAggregateColumns()
7) There are a couple of places where you have commented lines out instead of
deleting them. If there's a reason you left these in, then comments explaining
that would be great; otherwise I think it's cleaner to just delete the lines
altogether. For example:
In GroupByColumn.java:
@@ -48,7 +48,8 @@
*/
public void init(Object colRef)
{
- this.colRef = (ColumnReference) colRef;
+ //this.colRef = (ColumnReference) colRef;
+ this.columnExpression = (ValueNode)colRef;
}
/**
8) I think the diff for master/subquery.out has some stuff from a failed merge
in it:
Index: java/testing/org/apache/derbyTesting/functionTests/master/subquery.out
===================================================================
--- java/testing/org/apache/derbyTesting/functionTests/master/subquery.out
(revision 422486)
+++ java/testing/org/apache/derbyTesting/functionTests/master/subquery.out
(working copy)
@@ -1016,8 +1016,14 @@
Number of rows visited=9
Scan type=heap
start position:
+<<<<<<< .mine
null stop position:
+null
+ scan qualifiers:
+=======
+null stop position:
null scan qualifiers:
+>>>>>>> .r422486
None
next qualifiers:
None
**** Nit-picking: Changes that you don't *have* to do but that might help in
cleaning up the patch.
A. Unnecessary white-space changes?
In SelectNode.java:
- VerifyAggregateExpressionsVisitor visitor =
- new
VerifyAggregateExpressionsVisitor(groupByList);
+ VerifyAggregateExpressionsVisitor visitor =
+ new
VerifyAggregateExpressionsVisitor(groupByList);
resultColumns.accept(visitor);
- }
-
+ }
+
+
+
In GroupByNode.java:
-
C_NodeTypes.RESULT_COLUMN_LIST,
-
getContextManager()),
- ((FromTable)
childResult).getTableNumber());
+
C_NodeTypes.RESULT_COLUMN_LIST,
+ getContextManager()),
+ ((FromTable) childResult).getTableNumber());
...
- ** For each aggregate
- */
+ ** For each aggregate
+ */
...
-
+
/*
- ** AGG RESULT: Set the aggregate result to null in the
- ** bottom project restrict.
- */
+ ** AGG RESULT: Set the aggregate result to null in the
+ ** bottom project restrict.
+ */
... etc.
There are quite a few white-space changes in this patch that make it hard to
figure out what has actually changed.
B. Lines longer than 80 chars:
In SelectNode.java:
@@ -965,6 +956,11 @@
wherePredicates);
}
+ if (groupByList != null)
+ {
+ groupByList.preprocess(numTables, fromList,
whereSubquerys, wherePredicates);
+ }
+
In VerifyAggregateExpressionsVisitor.java:
+ return ((node instanceof AggregateNode) ||
+ (node instanceof SubqueryNode) ||
+ (node instanceof ValueNode &&
+ groupByList != null
+ &&
groupByList.findGroupingColumn((ValueNode)node) != null));
I know, this whole 80 character thing is a bit picky--but that seems to be the
rule adopted by most developers in the Derby code, so I'm just bringing it up...
------------------------------------------------------------------
I know that's a lot of feedback and very little of it has to do with the actual
correctness of the code--so my apologies if this is a bit annoying. But I do
think that some additional comments and an effort to reduce the extra diffs will
make this a cleaner patch and will help developers in the long and short-term
who might find themselves reading this code.
Functionally, this is a great patch--an excellent feature enhancement for Derby.
Thank you so much for taking the time to figure out all of the details--and
for writing such extensive tests, as well. I hope this feedback doesn't
discourage you from continuing to work on this or other Derby changes: I'm just
posting feedback based on my own opinions. If you disagree with anything I've
written here, please feel free to speak up!
Thanks again for all of the work on this,
Army
