db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Satheesh Bandaram <sathe...@Sourcery.Org>
Subject Re: [PATCH] Derby-127
Date Thu, 12 May 2005 19:34:17 GMT
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
  <title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Commited this patch.<br>
<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\engine\org\apache\derby\impl\sql\compile\OrderByColumn.java<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\engine\org\apache\derby\impl\sql\compile\ResultColumnList.java<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\engine\org\apache\derby\impl\sql\compile\TableName.java<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\engine\org\apache\derby\impl\sql\compile\sqlgrammar.jj<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\testing\org\apache\derbyTesting\functionTests\master\orderby.out<br>
Sending&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
java\testing\org\apache\derbyTesting\functionTests\tests\lang\orderby.sql<br>
Transmitting file data ......<br>
Committed revision 169744.<br>
<br>
Jack Klebanoff wrote:
<blockquote cite="mid427A4BF4.3020808@sbcglobal.net" type="cite">I have
attached a patch that fixes Jira bug Derby-127
(<a class="moz-txt-link-freetext" href="http://issues.apache.org/jira/browse/DERBY-127">http://issues.apache.org/jira/browse/DERBY-127</a>).
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.
  <br>
&nbsp;select c1 as x from t where ... group by x order by c1
  <br>
Derby throws an exception with SQLState 42x04 complaining that it
cannot resolve "c1".
  <br>
  <br>
The underlying problem is that the Derby parser transforms the select
into a query tree for the following statement:
  <br>
&nbsp;select * from (select c1 as x from t where ...) order by c1
  <br>
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.
  <br>
  <br>
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
  <br>
  <br>
Jack Klebanoff.
  <br>
  <pre wrap="">
<hr size="4" width="90%">
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 &lt;
0.
+     */
+    private int addedColumnOffset = -1;
 
 
     	/**
@@ -161,31 +169,23 @@
 			}
 
 		}else{
-			ResultColumnList targetCols = target.getResultColumns();
-			ResultColumn col = null;
-			int i = 1;
-			
-			for(i = 1;
-			    i &lt;= targetCols.size();
-			    i  ++){
-				
-				col = targetCols.getOrderByColumn(i);
-				if(col != null &amp;&amp; 
-				   col.getExpression() == expression){
-					
-					break;
-				}
-			}
-			
-			resultCol = col;
-			columnPosition = i;
-		    
+            if( SanityManager.DEBUG)
+                SanityManager.ASSERT( addedColumnOffset &gt;= 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) &amp;&amp; ((SelectNode) target).getGeneratedForGroupbyClause())
+        {
+            if( SanityManager.DEBUG)
+                SanityManager.ASSERT( target.getFromList().size() == 1
+                                      &amp;&amp; (target.getFromList().elementAt(0)
instanceof FromSubquery)
+                                      &amp;&amp; targetCols.size() == 1
+                                      &amp;&amp; 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 &amp;&amp; addedColumnOffset &gt;= 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())) &amp;&amp; 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&gt; 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&gt; 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&gt; 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&gt; 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&gt; rollback;
 ij&gt; -- reset autocommit
 autocommit on;
  </pre>
</blockquote>
</body>
</html>


Mime
View raw message