PHOENX-3451 Incorrect determination of preservation of order for an aggregate query leads to incorrect query results (chenglei)
Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/3f76e118 Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/3f76e118 Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/3f76e118 Branch: refs/heads/calcite Commit: 3f76e11802bcdc90b12f12e90494c1ebdcfb90fc Parents: 0d138cf Author: James Taylor <jamestay...@apache.org> Authored: Wed Nov 16 10:40:31 2016 -0800 Committer: James Taylor <jamestay...@apache.org> Committed: Wed Nov 16 21:45:50 2016 -0800 ---------------------------------------------------------------------- .../apache/phoenix/end2end/GroupByCaseIT.java | 51 +++++++++- .../phoenix/compile/OrderPreservingTracker.java | 100 ++++++++++++++++-- .../phoenix/compile/QueryCompilerTest.java | 101 +++++++++++++++++++ 3 files changed, 245 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/3f76e118/phoenix-core/src/it/java/org/apache/phoenix/end2end/GroupByCaseIT.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/GroupByCaseIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/GroupByCaseIT.java index 31cd9c4..bec7337 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/GroupByCaseIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/GroupByCaseIT.java @@ -33,7 +33,9 @@ import java.sql.Statement; import java.util.List; import java.util.Properties; +import org.apache.phoenix.compile.QueryPlan; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; +import org.apache.phoenix.jdbc.PhoenixStatement; import org.apache.phoenix.query.KeyRange; import org.apache.phoenix.schema.AmbiguousColumnException; import org.apache.phoenix.schema.types.PChar; @@ -589,6 +591,54 @@ public class GroupByCaseIT extends ParallelStatsDisabledIT { } @Test + public void testGroupByOrderByDescBug3451() throws Exception { + Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + try (Connection conn = DriverManager.getConnection(getUrl(), props)) { + String tableName=generateUniqueName(); + String sql="CREATE TABLE " + tableName + " (\n" + + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + + " CONTAINER_ID CHAR(15) NOT NULL,\n" + + " ENTITY_ID CHAR(15) NOT NULL,\n" + + " SCORE DOUBLE,\n" + + " CONSTRAINT TEST_PK PRIMARY KEY (\n" + + " ORGANIZATION_ID,\n" + + " CONTAINER_ID,\n" + + " ENTITY_ID\n" + + " )\n" + + " )"; + conn.createStatement().execute(sql); + String indexName=generateUniqueName(); + conn.createStatement().execute("CREATE INDEX " + indexName + " ON " + tableName + "(ORGANIZATION_ID,CONTAINER_ID, SCORE DESC, ENTITY_ID DESC)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container2','entityId6',1.1)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container1','entityId5',1.2)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container2','entityId4',1.3)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container1','entityId5',1.2)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container1','entityId3',1.4)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container3','entityId7',1.35)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('org2','container3','entityId8',1.45)"); + conn.commit(); + String query = "SELECT DISTINCT entity_id, score\n" + + " FROM " + tableName + "\n" + + " WHERE organization_id = 'org2'\n" + + " AND container_id IN ( 'container1','container2','container3' )\n" + + " ORDER BY score DESC\n" + + " LIMIT 2"; + Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery(query); + QueryPlan plan = stmt.unwrap(PhoenixStatement.class).getQueryPlan(); + assertEquals(indexName, plan.getContext().getCurrentTable().getTable().getName().getString()); + assertFalse(plan.getOrderBy().getOrderByExpressions().isEmpty()); + assertTrue(rs.next()); + assertEquals("entityId8", rs.getString(1)); + assertEquals(1.45, rs.getDouble(2),0.001); + assertTrue(rs.next()); + assertEquals("entityId3", rs.getString(1)); + assertEquals(1.4, rs.getDouble(2),0.001); + assertFalse(rs.next()); + } + } + + @Test public void testGroupByDescColumnWithNullsLastBug3452() throws Exception { Connection conn=null; @@ -598,7 +648,6 @@ public class GroupByCaseIT extends ParallelStatsDisabledIT { conn = DriverManager.getConnection(getUrl(), props); String tableName=generateUniqueName(); - conn.createStatement().execute("DROP TABLE if exists "+tableName); String sql="CREATE TABLE "+tableName+" ( "+ "ORGANIZATION_ID VARCHAR,"+ "CONTAINER_ID VARCHAR,"+ http://git-wip-us.apache.org/repos/asf/phoenix/blob/3f76e118/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java index 3aa6f06..e9603d7 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java @@ -17,12 +17,15 @@ import java.util.List; import org.apache.phoenix.compile.GroupByCompiler.GroupBy; import org.apache.phoenix.execute.TupleProjector; import org.apache.phoenix.expression.CoerceExpression; +import org.apache.phoenix.expression.Determinism; import org.apache.phoenix.expression.Expression; +import org.apache.phoenix.expression.LiteralExpression; import org.apache.phoenix.expression.ProjectedColumnExpression; import org.apache.phoenix.expression.RowKeyColumnExpression; import org.apache.phoenix.expression.RowValueConstructorExpression; import org.apache.phoenix.expression.function.FunctionExpression.OrderPreserving; import org.apache.phoenix.expression.function.ScalarFunction; +import org.apache.phoenix.expression.visitor.StatelessTraverseAllExpressionVisitor; import org.apache.phoenix.expression.visitor.StatelessTraverseNoExpressionVisitor; import org.apache.phoenix.schema.PTable; import org.apache.phoenix.schema.SortOrder; @@ -71,6 +74,7 @@ public class OrderPreservingTracker { private final Ordering ordering; private final int pkPositionOffset; private final List<Info> orderPreservingInfos; + private final TupleProjector projector; private boolean isOrderPreserving = true; private Boolean isReverse = null; private int orderPreservingColumnCount = 0; @@ -81,21 +85,23 @@ public class OrderPreservingTracker { public OrderPreservingTracker(StatementContext context, GroupBy groupBy, Ordering ordering, int nNodes, TupleProjector projector) { this.context = context; - int pkPositionOffset = 0; - PTable table = context.getResolver().getTables().get(0).getTable(); - isOrderPreserving = table.rowKeyOrderOptimizable(); - if (groupBy.isEmpty()) { // FIXME: would the below table have any of these set in the case of a GROUP BY? + if (groupBy.isEmpty()) { + PTable table = context.getResolver().getTables().get(0).getTable(); + this.isOrderPreserving = table.rowKeyOrderOptimizable(); boolean isSalted = table.getBucketNum() != null; boolean isMultiTenant = context.getConnection().getTenantId() != null && table.isMultiTenant(); boolean isSharedViewIndex = table.getViewIndexId() != null; // TODO: util for this offset, as it's computed in numerous places - pkPositionOffset = (isSalted ? 1 : 0) + (isMultiTenant ? 1 : 0) + (isSharedViewIndex ? 1 : 0); + this.pkPositionOffset = (isSalted ? 1 : 0) + (isMultiTenant ? 1 : 0) + (isSharedViewIndex ? 1 : 0); + } else { + this.isOrderPreserving = true; + this.pkPositionOffset = 0; } - this.pkPositionOffset = pkPositionOffset; this.groupBy = groupBy; this.visitor = new TrackOrderPreservingExpressionVisitor(projector); this.orderPreservingInfos = Lists.newArrayListWithExpectedSize(nNodes); this.ordering = ordering; + this.projector = projector; } public void track(Expression node) { @@ -195,6 +201,31 @@ public class OrderPreservingTracker { private boolean hasEqualityConstraints(int startPos, int endPos) { ScanRanges ranges = context.getScanRanges(); + // If a GROUP BY is being done, then the rows are ordered according to the GROUP BY key, + // not by the original row key order of the table (see PHOENIX-3451). + // We check each GROUP BY expression to see if it only references columns that are + // matched by equality constraints, in which case the expression itself would be constant. + // FIXME: this only recognizes row key columns that are held constant, not all columns. + // FIXME: we should optimize out any GROUP BY or ORDER BY expression which is deemed to + // be held constant based on the WHERE clause. + if (!groupBy.isEmpty()) { + for (int pos = startPos; pos < endPos; pos++) { + IsConstantVisitor visitor = new IsConstantVisitor(this.projector, ranges); + List<Expression> groupByExpressions = groupBy.getExpressions(); + if (pos >= groupByExpressions.size()) { // sanity check - shouldn't be necessary + return false; + } + Expression groupByExpression = groupByExpressions.get(pos); + if ( groupByExpression.getDeterminism().ordinal() > Determinism.PER_STATEMENT.ordinal() ) { + return false; + } + Boolean isConstant = groupByExpression.accept(visitor); + if (!Boolean.TRUE.equals(isConstant)) { + return false; + } + } + return true; + } for (int pos = startPos; pos < endPos; pos++) { if (!ranges.hasEqualityConstraint(pos)) { return false; @@ -207,6 +238,63 @@ public class OrderPreservingTracker { return Boolean.TRUE.equals(isReverse); } + /** + * + * Determines if an expression is held constant. Only works for columns in the PK currently, + * as we only track whether PK columns are held constant. + * + */ + private static class IsConstantVisitor extends StatelessTraverseAllExpressionVisitor<Boolean> { + private final TupleProjector projector; + private final ScanRanges scanRanges; + + public IsConstantVisitor(TupleProjector projector, ScanRanges scanRanges) { + this.projector = projector; + this.scanRanges = scanRanges; + } + + @Override + public Boolean defaultReturn(Expression node, List<Boolean> returnValues) { + if (node.getDeterminism().ordinal() > Determinism.PER_STATEMENT.ordinal() || + returnValues.size() < node.getChildren().size()) { + return Boolean.FALSE; + } + for (Boolean returnValue : returnValues) { + if (!returnValue) { + return Boolean.FALSE; + } + } + return Boolean.TRUE; + } + + @Override + public Boolean visit(RowKeyColumnExpression node) { + return scanRanges.hasEqualityConstraint(node.getPosition()); + } + + @Override + public Boolean visit(LiteralExpression node) { + return Boolean.TRUE; + } + + @Override + public Boolean visit(ProjectedColumnExpression node) { + if (projector == null) { + return super.visit(node); + } + Expression expression = projector.getExpressions()[node.getPosition()]; + // Only look one level down the projection. + if (expression instanceof ProjectedColumnExpression) { + return super.visit(node); + } + return expression.accept(this); + } + } + /** + * + * Visitor used to determine if order is preserved across a list of expressions (GROUP BY or ORDER BY expressions) + * + */ private static class TrackOrderPreservingExpressionVisitor extends StatelessTraverseNoExpressionVisitor<Info> { private final TupleProjector projector; http://git-wip-us.apache.org/repos/asf/phoenix/blob/3f76e118/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java b/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java index 30e3aaa..1706133 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java @@ -2781,6 +2781,107 @@ public class QueryCompilerTest extends BaseConnectionlessQueryTest { } @Test + public void testOrderPreservingGroupBy() throws Exception { + try (Connection conn= DriverManager.getConnection(getUrl())) { + + conn.createStatement().execute("CREATE TABLE test (\n" + + " pk1 INTEGER NOT NULL,\n" + + " pk2 INTEGER NOT NULL,\n" + + " pk3 INTEGER NOT NULL,\n" + + " pk4 INTEGER NOT NULL,\n" + + " v1 INTEGER,\n" + + " CONSTRAINT pk PRIMARY KEY (\n" + + " pk1,\n" + + " pk2,\n" + + " pk3,\n" + + " pk4\n" + + " )\n" + + " )"); + String[] queries = new String[] { + "SELECT pk3 FROM test WHERE pk2 = 1 GROUP BY pk2+1,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk2 = 1 GROUP BY pk2,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY pk1+pk2,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY pk4,CASE WHEN pk1 > pk2 THEN pk1 ELSE pk2 END,pk3 ORDER BY pk4,pk3", + }; + int index = 0; + for (String query : queries) { + QueryPlan plan = getQueryPlan(conn, query); + assertTrue((index + 1) + ") " + queries[index], plan.getOrderBy().getOrderByExpressions().isEmpty()); + index++; + } + } + } + + @Test + public void testNotOrderPreservingGroupBy() throws Exception { + try (Connection conn= DriverManager.getConnection(getUrl())) { + + conn.createStatement().execute("CREATE TABLE test (\n" + + " pk1 INTEGER NOT NULL,\n" + + " pk2 INTEGER NOT NULL,\n" + + " pk3 INTEGER NOT NULL,\n" + + " pk4 INTEGER NOT NULL,\n" + + " v1 INTEGER,\n" + + " CONSTRAINT pk PRIMARY KEY (\n" + + " pk1,\n" + + " pk2,\n" + + " pk3,\n" + + " pk4\n" + + " )\n" + + " )"); + String[] queries = new String[] { + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY pk4,CASE WHEN pk1 > pk2 THEN coalesce(v1,1) ELSE pk2 END,pk3 ORDER BY pk4,pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 END,pk3 ORDER BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 END,pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 END,pk3 ORDER BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 END,pk3", + "SELECT pk3 FROM test GROUP BY pk2,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 GROUP BY pk1,pk2,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 GROUP BY RAND()+pk1,pk2,pk3 ORDER BY pk3", + "SELECT pk3 FROM test WHERE pk1 = 1 and pk2 = 2 GROUP BY CASE WHEN pk1 > pk2 THEN pk1 ELSE RAND(1) END,pk3 ORDER BY pk3", + }; + int index = 0; + for (String query : queries) { + QueryPlan plan = getQueryPlan(conn, query); + assertFalse((index + 1) + ") " + queries[index], plan.getOrderBy().getOrderByExpressions().isEmpty()); + index++; + } + } + } + + @Test + public void testGroupByDescColumnBug3451() throws Exception { + + try (Connection conn= DriverManager.getConnection(getUrl())) { + + conn.createStatement().execute("CREATE TABLE IF NOT EXISTS GROUPBYTEST (\n" + + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + + " CONTAINER_ID CHAR(15) NOT NULL,\n" + + " ENTITY_ID CHAR(15) NOT NULL,\n" + + " SCORE DOUBLE,\n" + + " CONSTRAINT TEST_PK PRIMARY KEY (\n" + + " ORGANIZATION_ID,\n" + + " CONTAINER_ID,\n" + + " ENTITY_ID\n" + + " )\n" + + " )"); + conn.createStatement().execute("CREATE INDEX SCORE_IDX ON GROUPBYTEST (ORGANIZATION_ID,CONTAINER_ID, SCORE DESC, ENTITY_ID DESC)"); + QueryPlan plan = getQueryPlan(conn, "SELECT DISTINCT entity_id, score\n" + + " FROM GROUPBYTEST\n" + + " WHERE organization_id = 'org2'\n" + + " AND container_id IN ( 'container1','container2','container3' )\n" + + " ORDER BY score DESC\n" + + " LIMIT 2"); + assertFalse(plan.getOrderBy().getOrderByExpressions().isEmpty()); + plan = getQueryPlan(conn, "SELECT DISTINCT entity_id, score\n" + + " FROM GROUPBYTEST\n" + + " WHERE entity_id = 'entity1'\n" + + " AND container_id IN ( 'container1','container2','container3' )\n" + + " ORDER BY score DESC\n" + + " LIMIT 2"); + assertTrue(plan.getOrderBy().getOrderByExpressions().isEmpty()); + } + } + + @Test public void testGroupByDescColumnBug3452() throws Exception { Connection conn=null;