db-derby-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bpendle...@apache.org
Subject svn commit: r1292134 - in /db/derby/code/trunk/java: engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
Date Wed, 22 Feb 2012 06:05:56 GMT
Author: bpendleton
Date: Wed Feb 22 06:05:55 2012
New Revision: 1292134

URL: http://svn.apache.org/viewvc?rev=1292134&view=rev
Log:
DERBY-5584: distinct grouped aggregates can return wrong results

This change addresses a problem that can arise when a GroupedAggregateResultSet
that contains a distinct aggregate is processed multiple times in the
same query execution.

The problem involves this section of GroupedAggregateResultSet:

                        /*
                        ** If there was a distinct aggregate, then that column
                        ** was automatically included as the last column in
                        ** the sort ordering. But we don't want it to be part
                        ** of the ordering anymore, because we aren't grouping
                        ** by that column, we just sorted it so that distinct
                        ** aggregation would see the values in order.
                        */

The solution that was implemented in GroupedAggregateResultSet prior to this
change was assuming that the result set was only opened and read once; it
physically removed the last column from the ordering columns as a side effect
of processing the result set.

However, during a query plan such as this cartesian product, the GROUP BY
subquery is created, then opened/read/closed, opened/read/closed, etc.,
once per row of the other side of the cartesian product.

In that case, we can't physically remove the last column each time, because
then the second and subsequent times that we read the result set, we are
sorting on the wrong columns and we produce the wrong results.

The solution is to have a better way of handling that extra invisible column,
so that we can consider it sometimes, and ignore it other times,
without doing something as destructive as physically removing it from the
ordering array, which is what we do now. 

Note that Derby has had a limitation that there be at most one DISTINCT
aggregate in a query for a long time, probably ever since it was written.
See, for example, this link from the 10.2 docs:
http://db.apache.org/derby/docs/10.2/ref/rrefsqlj32693.html

        Only one DISTINCT aggregate expression per SelectExpression is allowed.
        For example, the following query is not valid:

            SELECT AVG (DISTINCT flying_time), SUM (DISTINCT miles) FROM Flights

The GroupedAggregateResultSet implementation is aware of this limit, and knows
that there is at most one distinct aggregate in the result set. This change
neither increases that dependency nor lessens it; I'm just noting it here
for the record.



Modified:
    db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java
    db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java

Modified: db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java?rev=1292134&r1=1292133&r2=1292134&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java
(original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/GroupedAggregateResultSet.java
Wed Feb 22 06:05:55 2012
@@ -88,6 +88,7 @@ class GroupedAggregateResultSet extends 
 	private ExecIndexRow sortTemplateRow;
 	public	boolean	hasDistinctAggregate;	// true if distinct aggregate
 	public	boolean isInSortedOrder;				// true if source results in sorted order
+	private	int numDistinctAggs = 0;
 	private int maxRowSize;
 
 	// set in open and not modified thereafter
@@ -237,7 +238,7 @@ class GroupedAggregateResultSet extends 
 		else if (!resultsComplete)
 		{
 			if (rollup)
-				resultRows = new ExecIndexRow[order.length+1];
+				resultRows = new ExecIndexRow[numGCols()+1];
 			else
 				resultRows = new ExecIndexRow[1];
 			if (aggInfoList.hasDistinct())
@@ -324,33 +325,26 @@ class GroupedAggregateResultSet extends 
 			** by that column, we just sorted it so that distinct
 			** aggregation would see the values in order.
 			*/
-			int numDistinctAggs = 0;
-			for (int i = 0; i < aggregates.length; i++)
-			{
-				AggregatorInfo aInfo = (AggregatorInfo)
-					aggInfoList.elementAt(i);
-				if (aInfo.isDistinct())
-					numDistinctAggs++;
-			}
 			// Although it seems like N aggs could have been
 			// added at the end, in fact only one has been
 			// FIXME -- need to get GroupByNode to handle this
 			// correctly, but that requires understanding
 			// scalar distinct aggregates.
 			numDistinctAggs = 1;
-			if (order.length > numDistinctAggs)
-			{
-				ColumnOrdering[] newOrder = new ColumnOrdering[
-					order.length - numDistinctAggs];
-				System.arraycopy(order, 0, newOrder, 0,
-					order.length-numDistinctAggs);
-				order = newOrder;
-			}
 		}
 		return tc.openSortScan(genericSortId,
 			activation.getResultSetHoldability());
 	}
 
+	/**
+	 * Return the number of grouping columns.
+	 *
+	 * Since some additional sort columns may have been included
+	 * in the sort for DISTINCT aggregates, this function is
+	 * used to ignore those columns when computing the grouped
+	 * results.
+	 */
+	private int numGCols() { return order.length - numDistinctAggs; }
 
 	/**
 	 * Return the next row.  
@@ -409,7 +403,7 @@ class GroupedAggregateResultSet extends 
 			{
 				boolean sameGroup = (rollup ?
 				    r <= distinguisherCol :
-				    distinguisherCol == order.length);
+				    distinguisherCol == numGCols());
 				if (sameGroup)
 				{
 					/* Same group - initialize the new
@@ -486,7 +480,7 @@ class GroupedAggregateResultSet extends 
 	private int sameGroupingValues(ExecRow currRow, ExecRow newRow)
 		throws StandardException
 	{
-		for (int index = 0; index < order.length; index++)
+		for (int index = 0; index < numGCols(); index++)
 		{
 			DataValueDescriptor currOrderable = currRow.getColumn(order[index].getColumnId() + 1);
 			DataValueDescriptor newOrderable = newRow.getColumn(order[index].getColumnId() + 1);
@@ -495,7 +489,7 @@ class GroupedAggregateResultSet extends 
 				return index;
 			}
 		}
-		return order.length;
+		return numGCols();
 	}
 
 	/**
@@ -650,7 +644,7 @@ class GroupedAggregateResultSet extends 
 		int numRolledUpCols = resultRows.length - resultNum - 1;
 		for (int i = 0; i < numRolledUpCols; i++)
 		{
-			int rolledUpColIdx = order.length - 1 - i;
+			int rolledUpColIdx = numGCols() - 1 - i;
 			DataValueDescriptor rolledUpColumn =
 				row.getColumn(order[rolledUpColIdx].getColumnId() + 1);
 			rolledUpColumn.setToNull();

Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java?rev=1292134&r1=1292133&r2=1292134&view=diff
==============================================================================
--- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
(original)
+++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
Wed Feb 22 06:05:55 2012
@@ -2611,4 +2611,262 @@ public class GroupByTest extends BaseJDB
 
             rollback();
     }
+
+
+    /**
+     * DISTINCT aggregates in result sets which are opened multiple times.
+     * DERBY-5584.
+     * @throws SQLException
+     */
+    public void testDerby5584()
+	throws SQLException
+    {
+        setAutoCommit(false);
+        Statement s = createStatement();
+        ResultSet rs;
+
+        s.executeUpdate(
+		"CREATE TABLE TEST_5 (" +
+		"       profile_id INTEGER NOT NULL," +
+		"       group_ref INTEGER NOT NULL," +
+		"       matched_count INTEGER NOT NULL )"); 
+
+        s.executeUpdate(
+		"CREATE TABLE TEST_6 ( " +
+		"       profile_id INTEGER NOT NULL, " +
+		"       group_ref INTEGER NOT NULL, " +
+		"       matched_count INTEGER NOT NULL )"); 
+
+        s.executeUpdate( "insert into test_5 values (1, 10000, 1)" ); 
+        s.executeUpdate( "insert into test_5 values (2, 10000, 2)" ); 
+
+        s.executeUpdate( "insert into test_6 values (1, 10000, 1)" ); 
+        s.executeUpdate( "insert into test_6 values (2, 10000, 2)" );
+
+        rs = s.executeQuery( "SELECT ps1.group_ref," +
+		"COUNT(DISTINCT ps1.matched_count) AS matched_count" +
+		" FROM test_5 ps1 " +
+		" GROUP BY ps1.group_ref, ps1.profile_id" );
+        JDBC.assertFullResultSet(rs, new String[][] {
+                {"10000", "1"},
+                {"10000", "1"}
+	});
+
+        rs = s.executeQuery( "SELECT ps1.group_ref," +
+		"COUNT(ps1.matched_count) AS matched_count" +
+		" FROM test_5 ps1 " +
+		" GROUP BY ps1.group_ref, ps1.profile_id" );
+        JDBC.assertFullResultSet(rs, new String[][] {
+                {"10000", "1"},
+                {"10000", "1"}
+	});
+
+	String cartProdWithDISTINCTsubqueries = " SELECT *" +
+		" FROM " +
+		" (SELECT ps1.group_ref, ps1.profile_id, " +
+		"         COUNT(DISTINCT ps1.matched_count) AS matched_count " +
+		"  FROM test_5 ps1" +
+		"  GROUP BY ps1.group_ref, ps1.profile_id " +
+		" ) a, " +
+		" (SELECT ps2.group_ref, ps2.profile_id, " +
+		"         COUNT( DISTINCT ps2.matched_count) AS matched_count" +
+		"  FROM test_6 ps2" +
+		"  GROUP BY ps2.group_ref, ps2.profile_id " +
+		") b ";
+
+	String cartProdWithSubqueries = " SELECT * " +
+		" FROM " +
+		" (SELECT ps1.group_ref, ps1.profile_id, " +
+		"         COUNT(ps1.matched_count) AS matched_count " +
+		"  FROM test_5 ps1 " +
+		"  GROUP BY ps1.group_ref, ps1.profile_id " +
+		") a, " +
+		" (SELECT ps2.group_ref, ps2.profile_id, " +
+		"         COUNT( ps2.matched_count) AS matched_count " +
+		"  FROM test_6 ps2 " +
+		"  GROUP BY ps2.group_ref, ps2.profile_id " +
+		") b ";
+
+	String cartProdWithOrderBySubqueries = "SELECT * " +
+		" FROM " +
+		" (SELECT ps1.group_ref, ps1.profile_id " +
+		"  FROM test_5 ps1 ORDER BY profile_id fetch first 3 rows only) a, " +
+		" (SELECT ps2.group_ref, ps2.profile_id " +
+		"  FROM test_6 ps2 ORDER BY PROFILE_ID fetch first 2 rows only) b "; 
+
+
+	rs = s.executeQuery( cartProdWithDISTINCTsubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithSubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"}
+	});
+
+        s.executeUpdate( "insert into test_5 values (3, 10000, 3)" ); 
+
+	rs = s.executeQuery( cartProdWithDISTINCTsubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithSubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "1"}
+	});
+
+        s.executeUpdate( "insert into test_5 values (4, 10000, 4) "); 
+        s.executeUpdate( "insert into test_6 values (3, 10000, 3) "); 
+
+	// NOTE: At this point,
+	//   test_5 contains:		test_6 contains:
+	//	1, 10000, 1			1, 10000, 1
+	//	2, 10000, 2			2, 10000, 2
+	//	3, 10000, 3			3, 10000, 3
+	//	4, 10000, 4
+
+	rs = s.executeQuery( cartProdWithDISTINCTsubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "1"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "1"},
+		{"10000", "4", "1", "10000", "2", "1"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithSubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "1"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "1"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "1"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "1"},
+		{"10000", "4", "1", "10000", "2", "1"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+        s.executeUpdate( "insert into test_6 values (2, 10000, 1) "); 
+
+	rs = s.executeQuery( cartProdWithDISTINCTsubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "2"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "2"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "2"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "1"},
+		{"10000", "4", "1", "10000", "2", "2"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithSubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "2"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "2"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "2"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "1"},
+		{"10000", "4", "1", "10000", "2", "2"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+	// Now introduce some duplicate values so that the DISTINCT
+	// aggregates have some work to do
+
+
+        s.executeUpdate( "insert into test_6 values (1, 10000, 1) "); 
+        s.executeUpdate( "insert into test_6 values (2, 10000, 2) "); 
+
+	// NOTE: At this point,
+	//   test_5 contains:		test_6 contains:
+	//	1, 10000, 1			1, 10000, 1 (2 vals, 1 distinct)
+	//	2, 10000, 2			1, 10000, 1 
+	//	3, 10000, 3			2, 10000, 1 (3 vals, 2 distinct)
+	//	4, 10000, 4			2, 10000, 2
+	//	 				2, 10000, 2
+	//	 				3, 10000, 2 (1 val, 1 distinct)
+
+	rs = s.executeQuery( cartProdWithDISTINCTsubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "1"},
+		{"10000", "1", "1", "10000", "2", "2"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "1"},
+		{"10000", "2", "1", "10000", "2", "2"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "1"},
+		{"10000", "3", "1", "10000", "2", "2"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "1"},
+		{"10000", "4", "1", "10000", "2", "2"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithSubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "1", "10000", "1", "2"},
+		{"10000", "1", "1", "10000", "2", "3"},
+		{"10000", "1", "1", "10000", "3", "1"},
+		{"10000", "2", "1", "10000", "1", "2"},
+		{"10000", "2", "1", "10000", "2", "3"},
+		{"10000", "2", "1", "10000", "3", "1"},
+		{"10000", "3", "1", "10000", "1", "2"},
+		{"10000", "3", "1", "10000", "2", "3"},
+		{"10000", "3", "1", "10000", "3", "1"},
+		{"10000", "4", "1", "10000", "1", "2"},
+		{"10000", "4", "1", "10000", "2", "3"},
+		{"10000", "4", "1", "10000", "3", "1"}
+	});
+
+	rs = s.executeQuery( cartProdWithOrderBySubqueries );
+        JDBC.assertFullResultSet(rs, new String[][] {
+		{"10000", "1", "10000", "1"},
+		{"10000", "1", "10000", "1"},
+		{"10000", "2", "10000", "1"},
+		{"10000", "2", "10000", "1"},
+		{"10000", "3", "10000", "1"},
+		{"10000", "3", "10000", "1"}
+	});
+
+            rollback();
+    }
 }



Mime
View raw message