Army,
Thanks for the feeback. I appreciate it. You've
identified atleast one (perhaps 2) bugs with my code
changes.
On the point of 80 chars per line, I can't help but
ask: does a tab count as 4 chars or 8?
The conventional wisdom from an earlier discussion was
that you set your editor to treat a tab as 4 chars
which I did and the offending lines that you point to
were well within the 80 char margin on my editor.
m
--- Army <[EMAIL PROTECTED]> wrote:
> 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?
>
>
=== message truncated ===
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
