db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jack Klebanoff <klebanoff-de...@sbcglobal.net>
Subject Re: About improvement of DERBY-134
Date Tue, 15 Mar 2005 01:51:59 GMT
The new patch looks much better. However, I found two problems, one
serious and the other minor.

The serious problem is that INTERSECT no longer works. The
lang/intersect.sql test (part of the derbylang suite) fails. The problem
is in the
org.apache.derby.impl.sql.compile.IntersectOrExceptNode.pushOrderingDown
method. It attempts to create OrderByColumns by calling
nf.getNode( C_NodeTypes.ORDER_BY_COLUMN,
ReuseFactory.getInteger( intermediateOrderByColumns[i] + 1),
cm)
This used to work. Now OrderByColumn.init throws a ClassCastException
because it expects to be passed a ValueNode, not an Integer.

IntersectOrExceptNode.pushOrderingDown has to be changed to pass a
ValueNode. I think that
nf.getNode( C_NodeTypes.ORDER_BY_COLUMN,
nf.getNode( C_NodeTypes.INT_CONSTANT_NODE,
ReuseFactory.getInteger( intermediateOrderByColumns[i] + 1),
cm),
cm)
works.

The minor problem is that the javadoc for OrderByColumn.init( Object
expression) documents a "dummy" parameter that no longer exists.

Jack Klebanoff

TomohitoNakayama wrote:

> Hello.
>
> I have finished coding and testing in orderby.sql.
> I'm not sure test is enough.
>
> Would you please review it ?
>
> Best regards.
>
> /*
>
> Tomohito Nakayama
> tomoihto@rose.zero.ad.jp
> tomonaka@basil.ocn.ne.jp
>
> Naka
> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>
> */
> ----- Original Message ----- From: "Satheesh Bandaram"
> <satheesh@sourcery.org>
> To: "Derby Development" <derby-dev@db.apache.org>
> Sent: Saturday, March 12, 2005 6:59 AM
> Subject: Re: About improvement of DERBY-134
>
>
>> Hi Tomohito Nakayama,
>>
>> Just wanted to check how you are progressing on the patch update,
>> following comments by myself and Jack. I do think you are working on an
>> important enhancement that not only yourself but other developpers have
>> expressed interest in. I strongly encourage you to continue working on
>> this and post any questions or comments you might have. You are pretty
>> close to addressing all issues.
>>
>> I am willing to help, if you need any, to continue taking this further.
>>
>> Satheesh
>>
>> TomohitoNakayama wrote:
>>
>>> Hello.
>>> Thanks for your reviewing.
>>>
>>> About 1:
>>> Handling any sortKey as expression is better structure.
>>> A little challenging but worth for it.
>>> I will try.
>>>
>>> About 2:
>>> Uh oh.
>>> Bug about starting value of element indexing in ResultColumnList ....
>>> Test of comma separated lists of ORDER BY expressions in orderby.sql
>>> was needed.....
>>>
>>> About 3:
>>> I see.
>>> It seems that it is certainly needed to add test case .
>>>
>>> I will continue this issue.
>>> Best regards.
>>>
>>> /*
>>>
>>> Tomohito Nakayama
>>> tomoihto@rose.zero.ad.jp
>>> tomonaka@basil.ocn.ne.jp
>>>
>>> Naka
>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>
>>> */
>>> ----- Original Message ----- From: "Jack Klebanoff"
>>> <klebanoff-derby@sbcglobal.net>
>>> To: "Derby Development" <derby-dev@db.apache.org>
>>> Sent: Sunday, February 20, 2005 8:37 AM
>>> Subject: 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@rose.zero.ad.jp
>>>>> tomonaka@basil.ocn.ne.jp
>>>>>
>>>>> Naka
>>>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>>>
>>>>> */
>>>>> ----- Original Message ----- From: "TomohitoNakayama"
>>>>> <tomonaka@basil.ocn.ne.jp>
>>>>> To: "Derby Development" <derby-dev@db.apache.org>
>>>>> Sent: Sunday, February 13, 2005 4:09 PM
>>>>> Subject: Re: About improvement of DERBY-134
>>>>>
>>>>>
>>>>>> Sorry.
>>>>>> Mistaken.
>>>>>>
>>>>>> LOOKAHEAD()....
>>>>>>
>>>>>> /*
>>>>>>
>>>>>> Tomohito Nakayama
>>>>>> tomoihto@rose.zero.ad.jp
>>>>>> tomonaka@basil.ocn.ne.jp
>>>>>>
>>>>>> Naka
>>>>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>>>>
>>>>>> */
>>>>>> ----- Original Message ----- From: "TomohitoNakayama"
>>>>>> <tomonaka@basil.ocn.ne.jp>
>>>>>> To: "Derby Development" <derby-dev@db.apache.org>
>>>>>> 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@rose.zero.ad.jp
>>>>>>> tomonaka@basil.ocn.ne.jp
>>>>>>>
>>>>>>> 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@rose.zero.ad.jp
>>>>>>> tomonaka@basil.ocn.ne.jp
>>>>>>>
>>>>>>> Naka
>>>>>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>>>>>
>>>>>>> */
>>>>>>> ----- Original Message ----- From: "TomohitoNakayama"
>>>>>>> <tomonaka@basil.ocn.ne.jp>
>>>>>>> To: "Derby Development" <derby-dev@db.apache.org>
>>>>>>> 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@rose.zero.ad.jp
>>>>>>> tomonaka@basil.ocn.ne.jp
>>>>>>>
>>>>>>> Naka
>>>>>>> http://www5.ocn.ne.jp/~tomohito/TopPage.html
>>>>>>>
>>>>>>> */
>>>>>>> ----- Original Message ----- From: "TomohitoNakayama"
>>>>>>> <tomonaka@basil.ocn.ne.jp>
>>>>>>> To: <derby-dev@db.apache.org>
>>>>>>> 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@rose.zero.ad.jp
>>>>>>> tomonaka@basil.ocn.ne.jp
>>>>>>>
>>>>>>> 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
>>>>
>>>>
>>>>


Mime
View raw message