db-derby-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bpendle...@apache.org
Subject svn commit: r632221 - in /db/derby/code/trunk/java: engine/org/apache/derby/impl/sql/compile/ testing/org/apache/derbyTesting/functionTests/tests/lang/
Date Fri, 29 Feb 2008 04:24:18 GMT
Author: bpendleton
Date: Thu Feb 28 20:24:17 2008
New Revision: 632221

URL: http://svn.apache.org/viewvc?rev=632221&view=rev
Log:
DERBY-3094: Grouping by expressions causes NullPointerException

This change modifies GroupByNode.addUnAggColumns to process the expressions
in the GROUP BY list in descending order of complexity, as measured by the
number of column references in the GROUP BY list element. This ensures
that when we are matching up expressions in the SELECT list with expressions
in the GROUP BY list, that we choose the best match, and don't erroneously
match a sub-expression.

Modified:
    db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/GroupByNode.java
    db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/SubstituteExpressionVisitor.java
    db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByExpressionTest.java

Modified: db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/GroupByNode.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/GroupByNode.java?rev=632221&r1=632220&r2=632221&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/GroupByNode.java (original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/GroupByNode.java Thu
Feb 28 20:24:17 2008
@@ -23,6 +23,9 @@
 
 import java.util.Iterator;
 import java.util.Vector;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.Collections;
 
 import org.apache.derby.catalog.IndexDescriptor;
 import org.apache.derby.iapi.error.StandardException;
@@ -343,6 +346,10 @@
 		ResultColumnList bottomRCL  = childResult.getResultColumns();
 		ResultColumnList groupByRCL = resultColumns;
 
+		ArrayList referencesToSubstitute = new ArrayList();
+		ArrayList havingRefsToSubstitute = null;
+		if (havingClause != null)
+			havingRefsToSubstitute = new ArrayList();
 		int sz = groupingList.size();
 		for (int i = 0; i < sz; i++) 
 		{
@@ -385,12 +392,27 @@
 			// in the projection list with a virtual column node
 			// that effectively points to a result column 
 			// in the result set doing the group by
-			SubstituteExpressionVisitor se = 
-				new SubstituteExpressionVisitor(
-						gbc.getColumnExpression(),
-						vc,
+			//
+			// Note that we don't perform the replacements
+			// immediately, but instead we accumulate them
+			// until the end of the loop. This allows us to
+			// sort the expressions and process them in
+			// descending order of complexity, necessary
+			// because a compound expression may contain a
+			// reference to a simple grouped column, but in
+			// such a case we want to process the expression
+			// as an expression, not as individual column
+			// references. E.g., if the statement was:
+			//   SELECT ... GROUP BY C1, C1 * (C2 / 100), C3
+			// then we don't want the replacement of the
+			// simple column reference C1 to affect the
+			// compound expression C1 * (C2 / 100). DERBY-3094.
+			//
+			ValueNode vn = gbc.getColumnExpression();
+			SubstituteExpressionVisitor vis =
+				new SubstituteExpressionVisitor(vn, vc,
 						AggregateNode.class);
-			parent.getResultColumns().accept(se);
+			referencesToSubstitute.add(vis);
 			
 			// Since we always need a PR node on top of the GB 
 			// node to perform projection we can use it to perform 
@@ -414,15 +436,26 @@
 			// GBN (RCL) -> (C1, SUM(C2), <input>, <aggregator>, MAX(C3), <input>,
<aggregator>
 			//              |
 			//       FBT (C1, C2)
-			if (havingClause != null) {
+			if (havingClause != null)
+			{
 				SubstituteExpressionVisitor havingSE =
-					new SubstituteExpressionVisitor(
-							gbc.getColumnExpression(),
-							vc, null);
-				havingClause.accept(havingSE);
+					new SubstituteExpressionVisitor(vn,vc,null);
+				havingRefsToSubstitute.add(havingSE);
 			}
 			gbc.setColumnPosition(bottomRCL.size());
 		}
+		Comparator sorter = new ExpressionSorter();
+		Collections.sort(referencesToSubstitute,sorter);
+		for (int r = 0; r < referencesToSubstitute.size(); r++)
+			parent.getResultColumns().accept(
+				(SubstituteExpressionVisitor)referencesToSubstitute.get(r));
+		if (havingRefsToSubstitute != null)
+		{
+			Collections.sort(havingRefsToSubstitute,sorter);
+			for (int r = 0; r < havingRefsToSubstitute.size(); r++)
+				havingClause.accept(
+					(SubstituteExpressionVisitor)havingRefsToSubstitute.get(r));
+}
 	}
 
 	/**
@@ -1220,6 +1253,48 @@
 						singleInputRowOptimization = true;
 					}
 				}
+			}
+		}
+	}
+
+	/**
+	 * Comparator class for GROUP BY expression substitution.
+	 *
+	 * This class enables the sorting of a collection of
+	 * SubstituteExpressionVisitor instances. We sort the visitors
+	 * during the tree manipulation processing in order to process
+	 * expressions of higher complexity prior to expressions of
+	 * lower complexity. Processing the expressions in this order ensures
+	 * that we choose the best match for an expression, and thus avoids
+	 * problems where we substitute a sub-expression instead of the
+	 * full expression. For example, if the statement is:
+	 *   ... GROUP BY a+b, a, a*(a+b), a+b+c
+	 * we'll process those expressions in the order: a*(a+b),
+	 * a+b+c, a+b, then a.
+	 */
+	private static final class ExpressionSorter implements Comparator
+	{
+		public int compare(Object o1, Object o2)
+		{
+			try {
+				ValueNode v1 = ((SubstituteExpressionVisitor)o1).getSource();
+				ValueNode v2 = ((SubstituteExpressionVisitor)o2).getSource();
+				int refCount1, refCount2;
+				CollectNodesVisitor vis = new CollectNodesVisitor(
+				ColumnReference.class);
+				v1.accept(vis);
+				refCount1 = vis.getList().size();
+				vis = new CollectNodesVisitor(ColumnReference.class);
+				v2.accept(vis);
+				refCount2 = vis.getList().size();
+				// The ValueNode with the larger number of refs
+				// should compare lower. That way we are sorting
+				// the expressions in descending order of complexity.
+				return refCount2 - refCount1;
+			}
+			catch (StandardException e)
+			{
+				throw new RuntimeException(e);
 			}
 		}
 	}

Modified: db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/SubstituteExpressionVisitor.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/SubstituteExpressionVisitor.java?rev=632221&r1=632220&r2=632221&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/SubstituteExpressionVisitor.java
(original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/compile/SubstituteExpressionVisitor.java
Thu Feb 28 20:24:17 2008
@@ -43,6 +43,14 @@
 		skipOverClass = skipThisClass;
 	}
 
+	/**
+	 * used by GroupByNode to process expressions by complexity level.
+	 */
+	public ValueNode getSource()
+	{
+		return source;
+	}
+
 	public Visitable visit(Visitable node) throws StandardException 
 	{
 		if (!(node instanceof ValueNode))

Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByExpressionTest.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByExpressionTest.java?rev=632221&r1=632220&r2=632221&view=diff
==============================================================================
--- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByExpressionTest.java
(original)
+++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByExpressionTest.java
Thu Feb 28 20:24:17 2008
@@ -122,6 +122,127 @@
                         {9,13,2}});
     }
     
+    /**
+     * queries which combine compound expressions and simple column refs.
+     */
+    public void testDerby3094Expressions() throws Exception
+    {
+        verifyQueryResults(
+                "Q1",
+                "select c1+c2, sum(c3) from test group by c1+c2, c1",
+                new int[][] {
+                        {11, 100}, 
+                        {12, 100},  // c1=1, c2=11
+                        {12, 100},  // c1=2, c2=10
+                        {13, 202}});
+        verifyQueryResults(
+                "Q2",
+                "select c1+c2, sum(c3) from test group by c1, c1+c2",
+                new int[][] {
+                        {11, 100}, 
+                        {12, 100},  // c1=1, c2=11
+                        {12, 100},  // c1=2, c2=10
+                        {13, 202}});
+        verifyQueryResults(
+                "Q3",
+                "select c1, c1+c2 from test group by c1, c1+c2",
+                new int[][] {
+                        {1, 11}, 
+                        {1, 12},
+                        {2, 12},
+                        {2, 13}});
+        verifyQueryResults(
+                "Q4",
+                "select c1+c2, sum(c3) from test group by c1+c2",
+                new int[][] {
+                        {11, 100}, 
+                        {12, 200},
+                        {13, 202}});
+        verifyQueryResults(
+                "Q5",
+                "select c1,c2,c1+c2,sum(c3) from test group by c1,c2,c1+c2",
+                new int[][] {
+                        {1, 10, 11, 100},
+                        {1, 11, 12, 100},
+                        {2, 10, 12, 100},
+                        {2, 11, 13, 202}});
+        verifyQueryResults(
+                "Q6",
+                "select c1,c2,sum(c3) from test group by c2, c1",
+                new int[][] {
+                        {1, 10, 100},
+                        {2, 10, 100},
+                        {1, 11, 100},
+                        {2, 11, 202}});
+        verifyQueryResults(
+                "Q7",
+                "select c1 as c2, sum(c3) from test group by c1,c2",
+                new int[][] {
+                        {1, 100},
+                        {1, 100},
+                        {2, 100},
+                        {2, 202}});
+        verifyQueryResults(
+                "Q8",
+                "select c1 as c2, sum(c3) from test group by c1",
+                new int[][] {
+                        {1, 200},
+                        {2, 302}});
+        verifyQueryResults(
+                "Q9",
+            "select c1+c2, sum(c3) from test group by c1+c2 having c1+c2 > 11",
+                new int[][] {
+                        {12, 200},
+                        {13, 202}});
+        verifyQueryResults(
+                "Q10",
+            "select c1+c2, sum(c3) from test " +
+                     "group by c1, c1+c2 having c1+c2 > 11",
+                new int[][] {
+                        {12, 100},
+                        {12, 100},
+                        {13, 202}});
+        verifyQueryResults(
+                "Q11",
+                "select c1*((c1+c2)/2), count(*) from test " +
+                " group by (c1+c2),  c1*((c1+c2)/2)",
+                new int[][] {
+                        {5, 1},
+                        {6, 1},
+                        {12, 1},
+                        {12, 2}});
+        verifyQueryResults(
+                "Q12",
+                "select c1, c1+c2, (c1+c2)+c3, count(*) from test " +
+                " group by c1, c1+c2, (c1+c2)+c3",
+                new int[][] {
+                        {1, 11, 111, 1},
+                        {1, 12, 112, 1},
+                        {2, 12, 112, 1},
+                        {2, 13, 114, 2}});
+        verifyQueryResults(
+                "Q13",
+                "select (c1+c2)+c3, count(*) from test " +
+                " group by c3, c1+c2",
+                new int[][] {
+                        {111, 1},
+                        {112, 2},
+                        {114, 2}});
+        assertCompileError(
+                "42Y30", "select c1+c2, sum(c3) from test group by c1");
+        assertCompileError(
+                "42Y30", "select c1,c2, sum(c3) from test group by c1+c2,c1");
+        assertCompileError(
+                "42Y30", "select c1+c2, sum(c3) from test group by 1");
+        assertCompileError(
+            "42X04", "select c1+c2 as expr, sum(c3) from test group by expr");
+        assertCompileError(
+            "42X04", "select c1 as c1a, c2, sum(c3) from test group by c1a,c2");
+        assertCompileError(
+                "42Y30", "select c1 as c2, sum(c3) from test group by c2");
+        assertCompileError(
+                "42Y30", "select c1+(c2+c3), sum(c3) from test group by c3, (c1+c2)");
+    }
     
     public void testSubSelect() throws Exception
     {



Mime
View raw message