Re: About improvement of DERBY-134

TomohitoNakayama wrote:

> Hello.
>
> I have put some LOOKAHEAD to sqlgrammer.jj and
> add some test pattern to orderby.sql.
>
> Would someone review patch please ?
>
> Best regards.
>
> /*
>
> Tomohito Nakayama
> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>
> Naka
> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>
> */
> ----- Original Message ----- From: "TomohitoNakayama"
> <tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx>
> To: "Derby Development" <derby-dev-PvNy6fhA98DNLxjTenLetw@xxxxxxxxxxxxxxxx>
> Sent: Sunday, February 13, 2005 4:09 PM
> Subject: Re: About improvement of DERBY-134
>
>
>> Sorry.
>> Mistaken.
>>
>> LOOKAHEAD()....
>>
>> /*
>>
>> Tomohito Nakayama
>> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
>> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>>
>> Naka
>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>
>> */
>> ----- Original Message ----- From: "TomohitoNakayama"
>> <tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx>
>> To: "Derby Development" <derby-dev-PvNy6fhA98DNLxjTenLetw@xxxxxxxxxxxxxxxx>
>> Sent: Sunday, February 13, 2005 3:42 PM
>> Subject: Re: About improvement of DERBY-134
>>
>>
>>> Hello.
>>>
>>> Thank's for your reviewing.
>>> Grammer ambiguity is very critical problem ....
>>>
>>> I will try to put LOOKUP() and consider about testing..
>>>
>>> #World is not simple as I wish to be.....
>>>
>>> Best regards.
>>>
>>> /*
>>>
>>> Tomohito Nakayama
>>> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
>>> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>>>
>>> Naka
>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>> ----- Original Message ----- From: Satheesh Bandaram
>>> To: Derby Development
>>> Sent: Saturday, February 12, 2005 4:10 AM
>>> Subject: Re: About improvement of DERBY-134
>>>
>>>
>>> I think the patch is a good start. But more work needs to be done.
>>> Based on a quick review, some of the items to be completed are:
>>> (there may be more)
>>>
>>> Grammar ambiguity. SortKey() has grammar ambiguity the way the patch
>>> is written. Since orderby expression and orderby column can both
>>> start with an identifier, this causes ambiguity. Need to rewrite or
>>> add lookup to avoid this.
>>> Current patch doesn't seem to support all expressions, Ex: select i
>>> from t1 order by i/2. So, needs more work.
>>> Add more test cases and test outputs to show changed behavior. You
>>> could add test cases to orderby.sql test that is already part of
>>> functionTests/tests/lang.
>>> I do encourage you to continue work on this ...
>>>
>>> Satheesh
>>>
>>> TomohitoNakayama wrote:
>>>
>>> I tried to solve DERBY-134.
>>> Patch is attached to this mail.
>>>
>>>
>>> /*
>>>
>>> Tomohito Nakayama
>>> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
>>> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>>>
>>> Naka
>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>> ----- Original Message ----- From: "TomohitoNakayama"
>>> <tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx>
>>> To: "Derby Development" <derby-dev-PvNy6fhA98DNLxjTenLetw@xxxxxxxxxxxxxxxx>
>>> Sent: Wednesday, February 09, 2005 5:33 PM
>>> Subject: Re: About improvement of DERBY-134
>>>
>>>
>>>
>>> Woops.
>>> Mistaken.
>>>
>>>
>>> That's "DERBY-124 Sorted string columns are sorted in a case
>>> sensitive way"
>>>
>>>
>>>
>>> That's "DERBY-134 Sorted string columns are sorted in a case
>>> sensitive way"
>>>
>>> /*
>>>
>>> Tomohito Nakayama
>>> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
>>> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>>>
>>> Naka
>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>> ----- Original Message ----- From: "TomohitoNakayama"
>>> <tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx>
>>> To: <derby-dev-PvNy6fhA98DNLxjTenLetw@xxxxxxxxxxxxxxxx>
>>> Sent: Wednesday, February 09, 2005 4:35 PM
>>> Subject: About improvement of DERBY-134
>>>
>>>
>>>
>>> Hello.
>>> My name is Naka.
>>> I'm very newbie in derby community.
>>>
>>> I'm now seeing report for derby in ASF Jira.
>>> And found a interesting issue.
>>>
>>> That's "DERBY-124 Sorted string columns are sorted in a case
>>> sensitive way"
>>>
>>> This issue seems to mean that we can't use complex item in order
>>> clause.
>>> #That title was difficult to understand a bit ....
>>>
>>> Solving this isn't useful?
>>> Especially when we manipulate DBMS by hand.
>>>
>>> What I think we need to do is as next:
>>>
>>> 1) Allow additiveExpression() in sortKey() in "sqlgrammer.jj". 2)
>>> Make OrderByColumn class to support additiveExpression.
>>>
>>> Best regards.
>>>
>>> /*
>>>
>>> Tomohito Nakayama
>>> tomoihto-3vbGXx/L6hogx5LJoh7VlA@xxxxxxxxxxxxxxxx
>>> tomonaka-w3tBFbkRwq9dvEbG9P4ZtA@xxxxxxxxxxxxxxxx
>>>
>>> Naka
>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>>
I have worked on Derby/Cloudscape for a few years and have even fixed
one or two ORDER BY bugs in the past. I have reviewed your patch. It is
close, but I have some problems with it.

1. sqlgrammar.jj. I think that creating a new method,
isNonReservedKeyword() to determine whether a token is a non-reserved
keyword or not, is a maintenance problem. Whenever we add a new
non-reserved keyword we must add it to the list of tokens, to
nonReservedKeyword(), and now to isNonReservedKeyword(). Having to add
it in two places is difficult enough to discover or remember. If we need
isNonReservedKeyword then we should find a way of combining
nonReservedKeyword and isNonReservedKeyword so that only one of them
keeps the list of non-reserved key words.

It is not necessary for the parser to recognize 3 cases of ORDER BY sort
key type. A column name is just one kind of <expression>. If the parser
treats it as an expression we should still get the right ordering. I
think that it would better if the parser did so. The OrderByColumn class
can special case a simple column reference expression, as an
optimization. This considerably simplifies parsing sort keys.

The only sort key type that has to be handled specially is that of an
integer constant. That specifies one of the select list columns as the
sort key. This case can be recognized in the parser, as is done in the
patch, or it can be recognized in OrderByColumn. In this alternative the
parser always creates OrderByColumn nodes with the sort key given by an
expression (a ValueNode). At bind time OrderByColumn can determine
whether the sort key expression is an integer constant, and if so treat
it as a column position.

The two alternatives differ in the way that they treat constant integer
expressions like "ORDER BY 2-1". The patch orders the rows by the
constant 1, which is not usefull. With the patch "ORDER BY 2-1 ASC" and
"ORDER BY 2-1 DESC" produce the same ordering. If OrderByColumn treated
an integer constant sort key expression as a result column position then
"ORDER BY 2-1" would cause the rows to be ordered on the first result
column, which I think is more usefull.

2. OrderByColumn. I think that there is a mistake in the patch to the
bindOrderByColumn method of class OrderByColumn.

The new code is
}else if(expression != null){

ResultColumn col = null;
int i = 0;

for(i = 0;
i < targetCols.size();
i ++){
col = targetCols.getOrderByColumn(i);
if(col != null &&
col.getExpression() == expression){
break;
}
}

Method ResultColumnList.getOrderByColumn( int) uses 1 indexing. The
patch assumes 0 indexing. So the loop really should be "for( i = 1; i <=
targetCols.size(); i++)".

(Java likes 0 indexing while SQL likes 1 indexing. So some parts of the
Derby code use 0 indexing while others use 1 indexing. The resulting
confusion has caught most of us at one time or another).

The result is that when the sort key is an expression
OrderByColumn.pullUpOrderByColumn adds it to the end of the target list,
but OrderByColumn.bindOrderByColumn doesn't find it.
OrderByColumn.bindOrderByColumn tests whether the second last column in
the target list is orderable. This is usually not right. Consider the
following SQL:

create table tblob( id int, b blob(1000));
select id,b from tblob order by abs(id);
select b,id from tblob order by abs(id);

The first SELECT raises the error "ERROR X0X67: Columns of type 'BLOB'
may not be used in CREATE INDEX, ORDER BY, GROUP BY, UNION, INTERSECT,
EXCEPT or DISTINCT, because comparisons are not supported for that
type". The second SELECT executes properly.

3. Testing. I would like to see some additional tests: the failing case
above; ORDER BY expressions combined with ASC and DESC, to ensure that
the compiler handles ASC and DESC after a sort key, and comma separated
lists of ORDER BY expressions.

Jack