Commited this patch.
Sending
java\engine\org\apache\derby\impl\sql\compile\OrderByColumn.java
Sending
java\engine\org\apache\derby\impl\sql\compile\ResultColumnList.java
Sending
java\engine\org\apache\derby\impl\sql\compile\TableName.java
Sending
java\engine\org\apache\derby\impl\sql\compile\sqlgrammar.jj
Sending
java\testing\org\apache\derbyTesting\functionTests\master\orderby.out
Sending
java\testing\org\apache\derbyTesting\functionTests\tests\lang\orderby.sql
Transmitting file data ......
Committed revision 169744.
Jack Klebanoff wrote:
I have
attached a patch that fixes Jira bug Derby-127
(http://issues.apache.org/jira/browse/DERBY-127). The problem is with
select statements that use a correlation name in the select list, a
group by clause, and an order by clause that refers to a column by its
database name instead of its correlation name. e.g.
select c1 as x from t where ... group by x order by c1
Derby throws an exception with SQLState 42x04 complaining that it
cannot resolve "c1".
The underlying problem is that the Derby parser transforms the select
into a query tree for the following statement:
select * from (select c1 as x from t where ...) order by c1
The code in class OrderByColumn did not take this into account. I
changed methods pullUpOrderByColumn and bindOrderByColumn to handle
this case specially. pullUpOrderByColumn adds the sort key to the
ResultColumnList if it cannot find it there. It is called before
binding and before select list wildcards ("*") are expanded. I changed
it to pull the sort key into the ResultColumnList of the subselect
generated to handle GROUP BY. I also changed it to remember where it
was added. This simplifies the bindOrderByColumn, which is called after
pullUpOrderByColumn.
I also fixed the handling of table names in class OrderByColumn. It
treated them as strings, which does not work when the schema or table
name is a quoted string containing a period. I changed OrderByColumn to
use class TableName to represent a table name. The
ResultColumnList.getOrderByColumn methods where changed accordingly
Jack Klebanoff.
Index: java/engine/org/apache/derby/impl/sql/compile/TableName.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/TableName.java (revision 168148)
+++ java/engine/org/apache/derby/impl/sql/compile/TableName.java (working copy)
@@ -206,6 +206,9 @@
*/
public boolean equals(TableName otherTableName)
{
+ if( otherTableName == null)
+ return false;
+
String fullTableName = getFullTableName();
if (fullTableName == null)
{
Index: java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj (revision 168148)
+++ java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj (working copy)
@@ -7305,6 +7305,12 @@
*
* RESOLVE - someday we should try to find matching aggregates
* instead of just adding them.
+ *
+ * NOTE: This rewriting of the query tree makes the handling of an ORDER BY
+ * clause difficult. See OrderByColumn.pullUpOrderByColumn. It makes specific
+ * assumptions about the structure of the generated query tree. Do not make
+ * any changes to this transformation without carefully considering the
+ * OrderByColumn pullUpOrderByColumn and bindOrderByColumn methods.
*/
if (havingClause != null)
{
Index: java/engine/org/apache/derby/impl/sql/compile/OrderByColumn.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/OrderByColumn.java (revision 168148)
+++ java/engine/org/apache/derby/impl/sql/compile/OrderByColumn.java (working copy)
@@ -30,6 +30,8 @@
import org.apache.derby.iapi.sql.compile.NodeFactory;
import org.apache.derby.iapi.sql.compile.C_NodeTypes;
+import org.apache.derby.iapi.util.ReuseFactory;
+
/**
* An OrderByColumn is a column in the ORDER BY clause. An OrderByColumn
* can be ordered ascending or descending.
@@ -44,6 +46,12 @@
private ResultColumn resultCol;
private boolean ascending = true;
private ValueNode _expression_;
+ /**
+ * If this sort key is added to the result column list then it is at result column position
+ * 1 + resultColumnList.size() - resultColumnList.getOrderBySelect() + addedColumnOffset
+ * If the sort key is already in the reault colum list then addedColumnOffset < 0.
+ */
+ private int addedColumnOffset = -1;
/**
@@ -161,31 +169,23 @@
}
}else{
- ResultColumnList targetCols = target.getResultColumns();
- ResultColumn col = null;
- int i = 1;
-
- for(i = 1;
- i <= targetCols.size();
- i ++){
-
- col = targetCols.getOrderByColumn(i);
- if(col != null &&
- col.getExpression() == _expression_){
-
- break;
- }
- }
-
- resultCol = col;
- columnPosition = i;
-
+ if( SanityManager.DEBUG)
+ SanityManager.ASSERT( addedColumnOffset >= 0,
+ "Order by _expression_ was not pulled into the result column list");
+ resolveAddedColumn(target);
}
// Verify that the column is orderable
resultCol.verifyOrderable();
}
+ private void resolveAddedColumn(ResultSetNode target)
+ {
+ ResultColumnList targetCols = target.getResultColumns();
+ columnPosition = targetCols.size() - targetCols.getOrderBySelect() + addedColumnOffset + 1;
+ resultCol = targetCols.getResultColumn( columnPosition);
+ }
+
/**
* Pull up this orderby column if it doesn't appear in the resultset
*
@@ -195,15 +195,42 @@
public void pullUpOrderByColumn(ResultSetNode target)
throws StandardException
{
- if(_expression_ instanceof ColumnReference){
+ ResultColumnList targetCols = target.getResultColumns();
+ // If the target is generated for a select node then we must also pull the order by column
+ // into the select list of the subquery.
+ if((target instanceof SelectNode) && ((SelectNode) target).getGeneratedForGroupbyClause())
+ {
+ if( SanityManager.DEBUG)
+ SanityManager.ASSERT( target.getFromList().size() == 1
+ && (target.getFromList().elementAt(0) instanceof FromSubquery)
+ && targetCols.size() == 1
+ && targetCols.getResultColumn(1) instanceof AllResultColumn,
+ "Unexpected structure of selectNode generated for a group by clause");
+
+ ResultSetNode subquery = ((FromSubquery) target.getFromList().elementAt(0)).getSubquery();
+ pullUpOrderByColumn( subquery);
+ if( resultCol == null) // The order by column is referenced by number
+ return;
+
+ // ResultCol is in the subquery's ResultColumnList. We have to transform this OrderByColumn
+ // so that it refers to the column added to the subquery. We assume that the select list
+ // in the top level target is a (generated) AllResultColumn node, so the this order by _expression_
+ // does not have to be pulled into the the top level ResultColumnList. Just change this
+ // OrderByColumn to be a reference to the added column. We cannot use an integer column
+ // number because the subquery can have a '*' in its select list, causing the column
+ // number to change when the '*' is expanded.
+ resultCol = null;
+ targetCols.copyOrderBySelect( subquery.getResultColumns());
+ return;
+ }
+
+ if(_expression_ instanceof ColumnReference){
+
ColumnReference cr = (ColumnReference) _expression_;
- ResultColumnList targetCols = target.getResultColumns();
resultCol = targetCols.getOrderByColumn(cr.getColumnName(),
- cr.tableName != null ?
- cr.tableName.getFullTableName():
- null);
+ cr.getTableNameNode());
if(resultCol == null){
resultCol = (ResultColumn) getNodeFactory().getNode(C_NodeTypes.RESULT_COLUMN,
@@ -211,16 +238,17 @@
cr,
getContextManager());
targetCols.addResultColumn(resultCol);
+ addedColumnOffset = targetCols.getOrderBySelect();
targetCols.incOrderBySelect();
}
}else if(!isReferedColByNum(_expression_)){
- ResultColumnList targetCols = target.getResultColumns();
resultCol = (ResultColumn) getNodeFactory().getNode(C_NodeTypes.RESULT_COLUMN,
null,
_expression_,
getContextManager());
targetCols.addResultColumn(resultCol);
+ addedColumnOffset = targetCols.getOrderBySelect();
targetCols.incOrderBySelect();
}
}
@@ -284,7 +312,7 @@
}
- private static ResultColumn resolveColumnReference(ResultSetNode target,
+ private ResultColumn resolveColumnReference(ResultSetNode target,
ColumnReference cr)
throws StandardException{
@@ -336,8 +364,15 @@
ResultColumnList targetCols = target.getResultColumns();
resultCol = targetCols.getOrderByColumn(cr.getColumnName(),
- cr.getTableName(),
+ cr.getTableNameNode(),
sourceTableNumber);
+ /* Search targetCols before using addedColumnOffset because select list wildcards, '*',
+ * are expanded after pullUpOrderByColumn is called. A simple column reference in the
+ * order by clause may be found in the user specified select list now even though it was
+ * not found when pullUpOrderByColumn was called.
+ */
+ if( resultCol == null && addedColumnOffset >= 0)
+ resolveAddedColumn(target);
if (resultCol == null || resultCol.isNameGenerated()){
String errString = cr.columnName;
Index: java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java
===================================================================
--- java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java (revision 168148)
+++ java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java (working copy)
@@ -354,14 +354,14 @@
* columnName and ensure that there is only one match.
*
* @param columnName The ResultColumn to get from the list
- * @param exposedName The correlation name on the OrderByColumn, if any
+ * @param tableName The table name on the OrderByColumn, if any
* @param tableNumber The tableNumber corresponding to the FromTable with the
- * exposed name of exposedName, if exposedName != null.
+ * exposed name of tableName, if tableName != null.
*
* @return the column that matches that name.
* @exception StandardException thrown on duplicate
*/
- public ResultColumn getOrderByColumn(String columnName, String exposedName, int tableNumber)
+ public ResultColumn getOrderByColumn(String columnName, TableName tableName, int tableNumber)
throws StandardException
{
int size = size();
@@ -378,26 +378,15 @@
* o The RC is not qualified, but its _expression_ is a ColumnReference
* from the same table (as determined by the tableNumbers).
*/
- if (exposedName != null)
+ if (tableName != null)
{
- String rcTableName = resultColumn.getTableName();
-
- if (rcTableName == null)
- {
- ValueNode rcExpr = resultColumn.getExpression();
- if (! (rcExpr instanceof ColumnReference))
- {
+ ValueNode rcExpr = resultColumn.getExpression();
+ if (! (rcExpr instanceof ColumnReference))
continue;
- }
- else if (tableNumber != ((ColumnReference) rcExpr).getTableNumber())
- {
- continue;
- }
- }
- else if (! exposedName.equals(resultColumn.getTableName()))
- {
- continue;
- }
+
+ ColumnReference cr = (ColumnReference) rcExpr;
+ if( (! tableName.equals( cr.getTableNameNode())) && tableNumber != cr.getTableNumber())
+ continue;
}
/* We finally got past the qualifiers, now see if the column
@@ -430,12 +419,12 @@
* columnName and ensure that there is only one match before the bind process.
*
* @param columnName The ResultColumn to get from the list
- * @param exposedName The correlation name on the OrderByColumn, if any
+ * @param tableName The table name on the OrderByColumn, if any
*
* @return the column that matches that name.
* @exception StandardException thrown on duplicate
*/
- public ResultColumn getOrderByColumn(String columnName, String exposedName)
+ public ResultColumn getOrderByColumn(String columnName, TableName tableName)
throws StandardException
{
int size = size();
@@ -449,20 +438,16 @@
// exposedName will not be null and "*" will not have an _expression_
// or tablename.
// We may be checking on "ORDER BY T.A" against "SELECT T.B, T.A".
- if (exposedName != null)
+ if (tableName != null)
{
ValueNode rcExpr = resultColumn.getExpression();
- if (rcExpr == null || resultColumn.getTableName() == null)
- {
- continue;
- }
- else
- {
- if (! (rcExpr instanceof ColumnReference) || ! exposedName.equals(resultColumn.getTableName()))
- {
- continue;
- }
- }
+ if (rcExpr == null || ! (rcExpr instanceof ColumnReference))
+ {
+ continue;
+ }
+ ColumnReference cr = (ColumnReference) rcExpr;
+ if( ! tableName.equals( cr.getTableNameNode()))
+ continue;
}
/* We finally got past the qualifiers, now see if the column
@@ -3925,4 +3910,9 @@
{
return orderBySelect;
}
+
+ public void copyOrderBySelect( ResultColumnList src)
+ {
+ orderBySelect = src.orderBySelect;
+ }
}
Index: java/testing/org/apache/derbyTesting/functionTests/tests/lang/orderby.sql
===================================================================
--- java/testing/org/apache/derbyTesting/functionTests/tests/lang/orderby.sql (revision 168148)
+++ java/testing/org/apache/derbyTesting/functionTests/tests/lang/orderby.sql (working copy)
@@ -320,6 +320,10 @@
create table bug2769(c1 int, c2 int);
insert into bug2769 values (1, 1), (1, 2), (3, 2), (3, 3);
select a.c1, sum(a.c1) from bug2769 a group by a.c1 order by a.c1;
+select bug2769.c1 as x, sum(bug2769.c1) as y from bug2769 group by bug2769.c1 order by bug2769.c1;
+select bug2769.c1 as x, sum(bug2769.c1) as y from bug2769 group by bug2769.c1 order by x;
+select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order by c1 + c2;
+select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order by -(c1 + c2);
rollback;
-- reset autocommit
Index: java/testing/org/apache/derbyTesting/functionTests/master/orderby.out
===================================================================
--- java/testing/org/apache/derbyTesting/functionTests/master/orderby.out (revision 168148)
+++ java/testing/org/apache/derbyTesting/functionTests/master/orderby.out (working copy)
@@ -759,6 +759,30 @@
-----------------------
1 |2
3 |6
+ij> select bug2769.c1 as x, sum(bug2769.c1) as y from bug2769 group by bug2769.c1 order by bug2769.c1;
+X |Y
+-----------------------
+1 |2
+3 |6
+ij> select bug2769.c1 as x, sum(bug2769.c1) as y from bug2769 group by bug2769.c1 order by x;
+X |Y
+-----------------------
+1 |2
+3 |6
+ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order by c1 + c2;
+X |Y
+-----------------------
+1 |1
+1 |2
+3 |2
+3 |3
+ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order by -(c1 + c2);
+X |Y
+-----------------------
+3 |3
+3 |2
+1 |2
+1 |1
ij> rollback;
ij> -- reset autocommit
autocommit on;
|
