phoenix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From maryann...@apache.org
Subject [22/50] [abbrv] phoenix git commit: PHOENX-3451 Incorrect determination of preservation of order for an aggregate query leads to incorrect query results (chenglei)
Date Tue, 27 Dec 2016 18:29:45 GMT
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 <jamestaylor@apache.org>
Authored: Wed Nov 16 10:40:31 2016 -0800
Committer: James Taylor <jamestaylor@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;


Mime
View raw message