db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Manish Khettry <manish_khet...@yahoo.com>
Subject Re: [jira] Updated: (DERBY-883) Enhance GROUP BY clause to support expressions instead of just column references.
Date Tue, 25 Jul 2006 04:59:10 GMT
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 <qozinx@gmail.com> 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 

Mime
View raw message