You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by sa...@apache.org on 2016/11/22 02:54:16 UTC
[12/36] phoenix git commit: PHOENX-3451 Incorrect determination of
preservation of order for an aggregate query leads to incorrect query results
(chenglei)
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/48728729
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/48728729
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/48728729
Branch: refs/heads/encodecolumns2
Commit: 487287297bbc68765fe6d5bfc56f167458f90e35
Parents: 9f29340
Author: James Taylor <ja...@apache.org>
Authored: Wed Nov 16 10:40:31 2016 -0800
Committer: James Taylor <ja...@apache.org>
Committed: Wed Nov 16 21:49:16 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/48728729/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 e6c2dbd..847fb4e 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;
@@ -586,6 +588,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;
@@ -595,7 +645,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/48728729/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/48728729/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;