This is an automated email from the ASF dual-hosted git repository.

pboado pushed a commit to branch 4.x-cdh5.16
in repository https://gitbox.apache.org/repos/asf/phoenix.git

commit 460da6136a75245b119d4f0393e08e9f61d579d5
Author: chenglei <cheng...@apache.org>
AuthorDate: Sat Jan 5 01:58:00 2019 +0000

    PHOENIX-4820 Optimize OrderBy for ClientAggregatePlan
---
 .../org/apache/phoenix/end2end/AggregateIT.java    | 104 +++++++
 .../apache/phoenix/compile/GroupByCompiler.java    |   8 +-
 .../apache/phoenix/compile/OrderByCompiler.java    |  18 +-
 .../phoenix/compile/OrderPreservingTracker.java    |  53 ++--
 .../org/apache/phoenix/compile/QueryCompiler.java  |  12 +-
 .../org/apache/phoenix/compile/RowProjector.java   |  15 +-
 .../phoenix/expression/BaseCompoundExpression.java |  11 +-
 .../apache/phoenix/expression/BaseExpression.java  |  11 +
 .../phoenix/expression/BaseSingleExpression.java   |   5 +
 .../phoenix/expression/DelegateExpression.java     |   5 +
 .../org/apache/phoenix/expression/Expression.java  |   6 +
 .../expression/ProjectedColumnExpression.java      |   8 +-
 .../expression/function/RandomFunction.java        |   5 +
 .../expression/visitor/CloneExpressionVisitor.java |   6 +-
 .../CloneNonDeterministicExpressionVisitor.java    |  31 --
 .../org/apache/phoenix/util/ExpressionUtil.java    | 160 +++++++++-
 .../apache/phoenix/compile/QueryCompilerTest.java  | 324 +++++++++++++++++++++
 .../expression/ArithmeticOperationTest.java        |   8 +-
 18 files changed, 705 insertions(+), 85 deletions(-)

diff --git 
a/phoenix-core/src/it/java/org/apache/phoenix/end2end/AggregateIT.java 
b/phoenix-core/src/it/java/org/apache/phoenix/end2end/AggregateIT.java
index 8916d4d..d52025e 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/AggregateIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/AggregateIT.java
@@ -227,5 +227,109 @@ public class AggregateIT extends BaseAggregateIT {
             assertEquals(4, rs.getLong(1));
         }
     }
+
+    @Test
+    public void testOrderByOptimizeForClientAggregatePlanBug4820() throws 
Exception {
+        doTestOrderByOptimizeForClientAggregatePlanBug4820(false,false);
+        doTestOrderByOptimizeForClientAggregatePlanBug4820(false,true);
+        doTestOrderByOptimizeForClientAggregatePlanBug4820(true,false);
+        doTestOrderByOptimizeForClientAggregatePlanBug4820(true,true);
+    }
+
+    private void doTestOrderByOptimizeForClientAggregatePlanBug4820(boolean 
desc ,boolean salted) throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        Connection conn = null;
+        try {
+            conn = DriverManager.getConnection(getUrl(), props);
+            String tableName = generateUniqueName();
+            String sql = "create table " + tableName + "( "+
+                    " pk1 varchar not null , " +
+                    " pk2 varchar not null, " +
+                    " pk3 varchar not null," +
+                    " v1 varchar, " +
+                    " v2 varchar, " +
+                    " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+                    "pk1 "+(desc ? "desc" : "")+", "+
+                    "pk2 "+(desc ? "desc" : "")+", "+
+                    "pk3 "+(desc ? "desc" : "")+
+                    " )) "+(salted ? "SALT_BUCKETS =4" : "split on('b')");
+            conn.createStatement().execute(sql);
+
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('a11','a12','a13','a14','a15')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('a21','a22','a23','a24','a25')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('a31','a32','a33','a34','a35')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('b11','b12','b13','b14','b15')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('b21','b22','b23','b24','b25')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
('b31','b32','b33','b34','b35')");
+            conn.commit();
+
+            sql = "select a.ak3 "+
+                  "from (select pk1 ak1,pk2 ak2,pk3 ak3, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                  "group by a.ak3,a.av1 order by a.ak3 desc,a.av1";
+            ResultSet rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new 
Object[][]{{"b33"},{"b23"},{"b13"},{"a33"},{"a23"},{"a13"}});
+
+            sql = "select a.ak3 "+
+                  "from (select pk1 ak1,pk2 ak2,pk3 ak3, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                  "group by a.ak3,a.av1 order by a.ak3,a.av1";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new 
Object[][]{{"a13"},{"a23"},{"a33"},{"b13"},{"b23"},{"b33"}});
+
+            sql = "select a.ak3 "+
+                  "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                  "where a.av1 = 'a' group by a.av1,a.ak3 order by a.ak3 desc";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new Object[][]{{"a33"},{"a23"},{"a13"}});
+
+            sql = "select a.ak3 "+
+                    "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                    "where a.av1 = 'a' group by a.av1,a.ak3 order by a.ak3";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new Object[][]{{"a13"},{"a23"},{"a33"}});
+
+            sql = "select a.ak3 "+
+                    "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                    "where a.av1 = 'b' and a.av2= 'b' group by CASE WHEN a.av1 
> a.av2 THEN a.av1 ELSE a.av2 END,a.ak3,a.ak2 order by a.ak3 desc,a.ak2 desc";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new Object[][]{{"b33"},{"b23"},{"b13"}});
+
+            sql = "select a.ak3 "+
+                    "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                    "where a.av1 = 'b' and a.av2= 'b' group by CASE WHEN a.av1 
> a.av2 THEN a.av1 ELSE a.av2 END,a.ak3,a.ak2 order by a.ak3,a.ak2 desc";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new Object[][]{{"b13"},{"b23"},{"b33"}});
+
+            tableName = generateUniqueName();
+            sql = "create table " + tableName + "( "+
+                    " pk1 double not null , " +
+                    " pk2 double not null, " +
+                    " pk3 double not null," +
+                    " v1 varchar, " +
+                    " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+                    "pk1 "+(desc ? "desc" : "")+", "+
+                    "pk2 "+(desc ? "desc" : "")+", "+
+                    "pk3 "+(desc ? "desc" : "")+
+                    " )) "+(salted ? "SALT_BUCKETS =4" : "split on(2.3)");
+            conn.createStatement().execute(sql);
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
(2.1,2.11,2.12,'e')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
(2.2,2.21,2.23,'d')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
(2.3,2.31,2.32,'c')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
(2.4,2.41,2.42,'b')");
+            conn.createStatement().execute("UPSERT INTO "+tableName+" VALUES 
(2.5,2.51,2.52,'a')");
+            conn.commit();
+
+            sql = "select a.av1 "+
+                  "from (select pk1 ak1,pk2 ak2,pk3 ak3, substr(v1,1,1) av1 
from "+tableName+" order by pk1,pk2 limit 10) a "+
+                  "where cast(a.ak1 as integer)=2 group by a.ak1,a.av1 order 
by a.av1";
+            rs = conn.prepareStatement(sql).executeQuery();
+            assertResultSet(rs, new Object[][]{{"a"},{"b"},{"c"},{"d"},{"e"}});
+
+        } finally {
+            if(conn != null) {
+                conn.close();
+            }
+        }
+    }
+
 }
 
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/compile/GroupByCompiler.java 
b/phoenix-core/src/main/java/org/apache/phoenix/compile/GroupByCompiler.java
index 4777c29..2bdea9a 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/GroupByCompiler.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/GroupByCompiler.java
@@ -143,7 +143,13 @@ public class GroupByCompiler {
             boolean isOrderPreserving = this.isOrderPreserving;
             int orderPreservingColumnCount = 0;
             if (isOrderPreserving) {
-                OrderPreservingTracker tracker = new 
OrderPreservingTracker(context, GroupBy.EMPTY_GROUP_BY, Ordering.UNORDERED, 
expressions.size(), tupleProjector);
+                OrderPreservingTracker tracker = new OrderPreservingTracker(
+                        context,
+                        GroupBy.EMPTY_GROUP_BY,
+                        Ordering.UNORDERED,
+                        expressions.size(),
+                        tupleProjector,
+                        null);
                 for (int i = 0; i < expressions.size(); i++) {
                     Expression expression = expressions.get(i);
                     tracker.track(expression);
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java 
b/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java
index b83c7a8..3c3f429 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java
@@ -88,7 +88,8 @@ public class OrderByCompiler {
                                   Integer offset,
                                   RowProjector rowProjector,
                                   TupleProjector tupleProjector,
-                                  boolean isInRowKeyOrder) throws SQLException 
{
+                                  boolean isInRowKeyOrder,
+                                  Expression whereExpression) throws 
SQLException {
         List<OrderByNode> orderByNodes = statement.getOrderBy();
         if (orderByNodes.isEmpty()) {
             return OrderBy.EMPTY_ORDER_BY;
@@ -105,9 +106,22 @@ public class OrderByCompiler {
         } else {
             compiler = new ExpressionCompiler(context, groupBy);
         }
+
+        if(groupBy != GroupBy.EMPTY_GROUP_BY) {
+            //if there is groupBy,the groupBy.expressions are viewed as new 
rowKey columns,so
+            //tupleProjector and isInRowKeyOrder is cleared
+            tupleProjector = null;
+            isInRowKeyOrder = true;
+        }
         // accumulate columns in ORDER BY
         OrderPreservingTracker tracker = 
-                new OrderPreservingTracker(context, groupBy, Ordering.ORDERED, 
orderByNodes.size(), tupleProjector);
+                new OrderPreservingTracker(
+                        context,
+                        groupBy,
+                        Ordering.ORDERED,
+                        orderByNodes.size(),
+                        tupleProjector,
+                        whereExpression);
         LinkedHashSet<OrderByExpression> orderByExpressions = 
Sets.newLinkedHashSetWithExpectedSize(orderByNodes.size());
         for (OrderByNode node : orderByNodes) {
             ParseNode parseNode = node.getNode();
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 d1175f6..29b3794 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
@@ -18,8 +18,8 @@ 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.KeyValueColumnExpression;
 import org.apache.phoenix.expression.LiteralExpression;
 import org.apache.phoenix.expression.ProjectedColumnExpression;
 import org.apache.phoenix.expression.RowKeyColumnExpression;
@@ -30,6 +30,7 @@ import 
org.apache.phoenix.expression.visitor.StatelessTraverseAllExpressionVisit
 import 
org.apache.phoenix.expression.visitor.StatelessTraverseNoExpressionVisitor;
 import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.util.ExpressionUtil;
 
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Lists;
@@ -76,16 +77,22 @@ 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;
+    private Expression whereExpression;
     
     public OrderPreservingTracker(StatementContext context, GroupBy groupBy, 
Ordering ordering, int nNodes) {
-        this(context, groupBy, ordering, nNodes, null);
+        this(context, groupBy, ordering, nNodes, null, null);
     }
     
-    public OrderPreservingTracker(StatementContext context, GroupBy groupBy, 
Ordering ordering, int nNodes, TupleProjector projector) {
+    public OrderPreservingTracker(
+            StatementContext context,
+            GroupBy groupBy,
+            Ordering ordering,
+            int nNodes,
+            TupleProjector projector,
+            Expression whereExpression) {
         this.context = context;
         if (groupBy.isEmpty()) {
             PTable table = context.getResolver().getTables().get(0).getTable();
@@ -103,7 +110,7 @@ public class OrderPreservingTracker {
         this.visitor = new TrackOrderPreservingExpressionVisitor(projector);
         this.orderPreservingInfos = Lists.newArrayListWithExpectedSize(nNodes);
         this.ordering = ordering;
-        this.projector = projector;
+        this.whereExpression = whereExpression;
     }
     
     public void track(Expression node) {
@@ -208,20 +215,14 @@ public class OrderPreservingTracker {
         // 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);
+                IsConstantVisitor visitor = new IsConstantVisitor(ranges, 
whereExpression);
                 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;
@@ -248,17 +249,17 @@ public class OrderPreservingTracker {
      *
      */
     private static class IsConstantVisitor extends 
StatelessTraverseAllExpressionVisitor<Boolean> {
-        private final TupleProjector projector;
         private final ScanRanges scanRanges;
+        private final Expression whereExpression;
         
-        public IsConstantVisitor(TupleProjector projector, ScanRanges 
scanRanges) {
-            this.projector = projector;
-            this.scanRanges = scanRanges;
-        }
+        public IsConstantVisitor(ScanRanges scanRanges, Expression 
whereExpression) {
+           this.scanRanges = scanRanges;
+           this.whereExpression = whereExpression;
+       }
         
         @Override
         public Boolean defaultReturn(Expression node, List<Boolean> 
returnValues) {
-            if (node.getDeterminism().ordinal() > 
Determinism.PER_STATEMENT.ordinal() || 
+            if (!ExpressionUtil.isContantForStatement(node) ||
                     returnValues.size() < node.getChildren().size()) {
                 return Boolean.FALSE;
             }
@@ -281,16 +282,12 @@ public class OrderPreservingTracker {
         }
 
         @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);
+        public Boolean visit(KeyValueColumnExpression 
keyValueColumnExpression) {
+            return 
ExpressionUtil.isColumnExpressionConstant(keyValueColumnExpression, 
whereExpression);
+        }
+         @Override
+        public Boolean visit(ProjectedColumnExpression 
projectedColumnExpression) {
+            return 
ExpressionUtil.isColumnExpressionConstant(projectedColumnExpression, 
whereExpression);
         }
     }
     /**
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java 
b/phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java
index 603da0b..6e36158 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java
@@ -562,8 +562,16 @@ public class QueryCompiler {
         groupBy = groupBy.compile(context, innerPlanTupleProjector);
         context.setResolver(resolver); // recover resolver
         RowProjector projector = ProjectionCompiler.compile(context, select, 
groupBy, asSubquery ? Collections.<PDatum>emptyList() : targetColumns, where);
-        OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, 
limit, offset, projector,
-                groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : 
null, isInRowKeyOrder);
+        OrderBy orderBy = OrderByCompiler.compile(
+                context,
+                select,
+                groupBy,
+                limit,
+                offset,
+                projector,
+                innerPlanTupleProjector,
+                isInRowKeyOrder,
+                where);
         context.getAggregationManager().compile(context, groupBy);
         // Final step is to build the query plan
         if (!asSubquery) {
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/compile/RowProjector.java 
b/phoenix-core/src/main/java/org/apache/phoenix/compile/RowProjector.java
index 2123788..8532e0c 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/RowProjector.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/RowProjector.java
@@ -22,9 +22,8 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
-import org.apache.phoenix.expression.Determinism;
 import org.apache.phoenix.expression.Expression;
-import 
org.apache.phoenix.expression.visitor.CloneNonDeterministicExpressionVisitor;
+import org.apache.phoenix.expression.visitor.CloneExpressionVisitor;
 import org.apache.phoenix.schema.ColumnNotFoundException;
 import org.apache.phoenix.util.SchemaUtil;
 
@@ -97,17 +96,17 @@ public class RowProjector {
         this.isProjectEmptyKeyValue = isProjectEmptyKeyValue;
         this.isProjectAll = isProjectAll;
         this.hasUDFs = hasUDFs;
-        boolean hasPerInvocationExpression = false;
+        boolean cloneRequired = false;
         if (!hasUDFs) {
             for (int i = 0; i < this.columnProjectors.size(); i++) {
                 Expression expression = 
this.columnProjectors.get(i).getExpression();
-                if (expression.getDeterminism() == Determinism.PER_INVOCATION) 
{
-                    hasPerInvocationExpression = true;
+                if (expression.isCloneExpression()) {
+                    cloneRequired = true;
                     break;
                 }
             }
         }
-        this.cloneRequired = hasPerInvocationExpression || hasUDFs;
+        this.cloneRequired = cloneRequired || hasUDFs;
     }
 
     public RowProjector cloneIfNecessary() {
@@ -118,8 +117,8 @@ public class RowProjector {
         for (int i = 0; i < this.columnProjectors.size(); i++) {
             ColumnProjector colProjector = columnProjectors.get(i);
             Expression expression = colProjector.getExpression();
-            if (expression.getDeterminism() == Determinism.PER_INVOCATION) {
-                CloneNonDeterministicExpressionVisitor visitor = new 
CloneNonDeterministicExpressionVisitor();
+            if (expression.isCloneExpression()) {
+                CloneExpressionVisitor visitor = new CloneExpressionVisitor();
                 Expression clonedExpression = expression.accept(visitor);
                 clonedColProjectors.add(new 
ExpressionProjector(colProjector.getName(),
                         colProjector.getTableName(), 
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseCompoundExpression.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseCompoundExpression.java
index 5fc8361..e26c902 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseCompoundExpression.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseCompoundExpression.java
@@ -35,6 +35,7 @@ public abstract class BaseCompoundExpression extends 
BaseExpression {
     private boolean isStateless;
     private Determinism determinism;
     private boolean requiresFinalEvaluation;
+    private boolean cloneExpression;
    
     public BaseCompoundExpression() {
         init(Collections.<Expression>emptyList());
@@ -49,6 +50,7 @@ public abstract class BaseCompoundExpression extends 
BaseExpression {
         boolean isStateless = true;
         boolean isNullable = false;
         boolean requiresFinalEvaluation = false;
+        boolean cloneExpression = false;
         this.determinism = Determinism.ALWAYS;
         for (int i = 0; i < children.size(); i++) {
             Expression child = children.get(i);
@@ -56,10 +58,12 @@ public abstract class BaseCompoundExpression extends 
BaseExpression {
             isStateless &= child.isStateless();
             this.determinism = 
this.determinism.combine(child.getDeterminism());
             requiresFinalEvaluation |= child.requiresFinalEvaluation();
+            cloneExpression |= child.isCloneExpression();
         }
         this.isStateless = isStateless;
         this.isNullable = isNullable;
         this.requiresFinalEvaluation = requiresFinalEvaluation;
+        this.cloneExpression = cloneExpression;
     }
     
     @Override
@@ -72,7 +76,12 @@ public abstract class BaseCompoundExpression extends 
BaseExpression {
     public Determinism getDeterminism() {
         return determinism;
     }
-    
+
+    @Override
+    public boolean isCloneExpression() {
+        return this.cloneExpression;
+    }
+
     @Override
     public boolean isStateless() {
         return isStateless;
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java
index efdceac..ccb6073 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java
@@ -255,4 +255,15 @@ public abstract class BaseExpression implements Expression 
{
         return false;
     }
 
+    @Override
+    public boolean isCloneExpression()  {
+       return isCloneExpressionByDeterminism(this);
+    }
+
+    protected static boolean isCloneExpressionByDeterminism(BaseExpression 
expression) {
+        if(expression.getDeterminism() == Determinism.PER_INVOCATION) {
+            return true;
+        }
+        return false;
+    }
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseSingleExpression.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseSingleExpression.java
index fbe8040..c636319 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseSingleExpression.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/BaseSingleExpression.java
@@ -118,4 +118,9 @@ public abstract class BaseSingleExpression extends 
BaseExpression {
     public Determinism getDeterminism() {
         return children.get(0).getDeterminism();
     }
+
+    @Override
+    public boolean isCloneExpression() {
+        return children.get(0).isCloneExpression();
+    }
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/DelegateExpression.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/DelegateExpression.java
index 3ca93dd..fd783c3 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/DelegateExpression.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/DelegateExpression.java
@@ -105,4 +105,9 @@ public class DelegateExpression implements Expression {
         return delegate.requiresFinalEvaluation();
     }
 
+    @Override
+    public boolean isCloneExpression() {
+        return delegate.isCloneExpression();
+    }
+
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java
index aeea0c8..d490ced 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java
@@ -88,4 +88,10 @@ public interface Expression extends PDatum, Writable {
      * @return
      */
     boolean requiresFinalEvaluation();
+
+    /**
+     * Determines if expression needs to be cloned in {@link 
org.apache.phoenix.compile.RowProjector}
+     * @return
+     */
+    boolean isCloneExpression();
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/ProjectedColumnExpression.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/ProjectedColumnExpression.java
index f851efa..e60bf00 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/ProjectedColumnExpression.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/ProjectedColumnExpression.java
@@ -153,12 +153,12 @@ public class ProjectedColumnExpression extends 
ColumnExpression {
     }
 
     @Override
-    public Determinism getDeterminism() {
-        return Determinism.PER_INVOCATION;
+    public ProjectedColumnExpression clone() {
+        return new ProjectedColumnExpression(this.column, this.columns, 
this.position, this.displayName);
     }
 
     @Override
-    public ProjectedColumnExpression clone() {
-        return new ProjectedColumnExpression(this.column, this.columns, 
this.position, this.displayName);
+    public boolean isCloneExpression() {
+        return true;
     }
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RandomFunction.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RandomFunction.java
index 01a4eed..d9048f4 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RandomFunction.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RandomFunction.java
@@ -118,6 +118,11 @@ public class RandomFunction extends ScalarFunction {
     }
 
     @Override
+    public boolean isCloneExpression()  {
+        return isCloneExpressionByDeterminism(this);
+    }
+
+    @Override
     public boolean isStateless() {
         return true;
     }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java
index c6d7c9e..b7ea4ab 100644
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java
+++ 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java
@@ -51,7 +51,7 @@ import org.apache.phoenix.expression.function.ScalarFunction;
 import org.apache.phoenix.expression.function.SingleAggregateFunction;
 import org.apache.phoenix.expression.function.UDFExpression;
 
-public abstract class CloneExpressionVisitor extends 
TraverseAllExpressionVisitor<Expression> {
+public class CloneExpressionVisitor extends 
TraverseAllExpressionVisitor<Expression> {
 
     public CloneExpressionVisitor() {
     }
@@ -215,5 +215,7 @@ public abstract class CloneExpressionVisitor extends 
TraverseAllExpressionVisito
         return isCloneNode(node, l) ? new ArrayElemRefExpression(l) : node;
     }
 
-    public abstract boolean isCloneNode(Expression node, List<Expression> 
children);
+    public boolean isCloneNode(Expression node, List<Expression> children) {
+        return node.isCloneExpression();
+    }
 }
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneNonDeterministicExpressionVisitor.java
 
b/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneNonDeterministicExpressionVisitor.java
deleted file mode 100644
index 9a56e36..0000000
--- 
a/phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneNonDeterministicExpressionVisitor.java
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.phoenix.expression.visitor;
-
-import java.util.List;
-
-import org.apache.phoenix.expression.Determinism;
-import org.apache.phoenix.expression.Expression;
-
-public class CloneNonDeterministicExpressionVisitor extends 
CloneExpressionVisitor {
-
-    @Override
-    public boolean isCloneNode(Expression node, List<Expression> children) {
-        return Determinism.PER_INVOCATION.compareTo(node.getDeterminism()) <= 
0;
-    }
-}
diff --git 
a/phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java 
b/phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java
index 881b0e1..e737721 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java
@@ -10,12 +10,20 @@
 package org.apache.phoenix.util;
 
 import java.sql.SQLException;
+import java.util.Iterator;
 import java.util.List;
 
+import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.AndExpression;
+import org.apache.phoenix.expression.ColumnExpression;
+import org.apache.phoenix.expression.ComparisonExpression;
 import org.apache.phoenix.expression.Determinism;
 import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.IsNullExpression;
 import org.apache.phoenix.expression.LiteralExpression;
+import 
org.apache.phoenix.expression.visitor.StatelessTraverseAllExpressionVisitor;
+import 
org.apache.phoenix.expression.visitor.StatelessTraverseNoExpressionVisitor;
 import org.apache.phoenix.schema.ColumnRef;
 import org.apache.phoenix.schema.PColumn;
 import org.apache.phoenix.schema.TableRef;
@@ -27,10 +35,19 @@ public class ExpressionUtil {
        }
 
        public static boolean isConstant(Expression expression) {
-               return (expression.isStateless() && 
(expression.getDeterminism() == Determinism.ALWAYS
-                               || expression.getDeterminism() == 
Determinism.PER_STATEMENT));
+               return (expression.isStateless() && 
isContantForStatement(expression));
        }
 
+   /**
+    * this method determines if expression is constant if all children of it 
are constants.
+    * @param expression
+    * @return
+    */
+    public static boolean isContantForStatement(Expression expression) {
+        return  (expression.getDeterminism() == Determinism.ALWAYS
+                || expression.getDeterminism() == Determinism.PER_STATEMENT);
+    }
+
     public static LiteralExpression getConstantExpression(Expression 
expression, ImmutableBytesWritable ptr)
             throws SQLException {
         Object value = null;
@@ -68,4 +85,143 @@ public class ExpressionUtil {
         return false;
     }
 
+    /**
+     * check the whereExpression to see if the columnExpression is constant.
+     * eg. for "where a =3 and b > 9", a is constant,but b is not.
+     * @param columnExpression
+     * @param whereExpression
+     * @return
+     */
+    public static boolean isColumnExpressionConstant(ColumnExpression 
columnExpression, Expression whereExpression) {
+        if(whereExpression == null) {
+            return false;
+        }
+        IsColumnConstantExpressionVisitor isColumnConstantExpressionVisitor =
+                new IsColumnConstantExpressionVisitor(columnExpression);
+        whereExpression.accept(isColumnConstantExpressionVisitor);
+        return isColumnConstantExpressionVisitor.isConstant();
+    }
+
+    private static class IsColumnConstantExpressionVisitor extends 
StatelessTraverseNoExpressionVisitor<Void> {
+        private final Expression columnExpression ;
+        private Expression firstRhsConstantExpression = null;
+        private int rhsConstantCount = 0;
+        private boolean isNullExpressionVisited = false;
+
+        public IsColumnConstantExpressionVisitor(Expression columnExpression) {
+            this.columnExpression = columnExpression;
+        }
+        /**
+         * only consider and,for "where a = 3 or b = 9", neither a or b is 
constant.
+         */
+        @Override
+        public Iterator<Expression> visitEnter(AndExpression andExpression) {
+            if(rhsConstantCount > 1) {
+                return null;
+            }
+            return andExpression.getChildren().iterator();
+        }
+        /**
+         * <pre>
+         * We just consider {@link ComparisonExpression} because:
+         * 1.for {@link InListExpression} as "a in ('2')", the {@link 
InListExpression} is rewritten to
+         *  {@link ComparisonExpression} in {@link InListExpression#create}
+         * 2.for {@link RowValueConstructorExpression} as "(a,b)=(1,2)",{@link 
RowValueConstructorExpression}
+         *   is rewritten to {@link ComparisonExpression} in {@link 
ComparisonExpression#create}
+         * 3.not consider {@link CoerceExpression}, because for "where cast(a 
as integer)=2", when a is double,
+         *   a is not constant.
+         * </pre>
+         */
+        @Override
+        public Iterator<Expression> visitEnter(ComparisonExpression 
comparisonExpression) {
+            if(rhsConstantCount > 1) {
+                return null;
+            }
+            if(comparisonExpression.getFilterOp() != CompareOp.EQUAL) {
+                return null;
+            }
+            Expression lhsExpresssion = 
comparisonExpression.getChildren().get(0);
+            if(!this.columnExpression.equals(lhsExpresssion)) {
+                return null;
+            }
+            Expression rhsExpression = 
comparisonExpression.getChildren().get(1);
+            if(rhsExpression == null) {
+                return null;
+            }
+            Boolean isConstant = rhsExpression.accept(new 
IsCompositeLiteralExpressionVisitor());
+            if(isConstant != null && isConstant.booleanValue()) {
+                checkConstantValue(rhsExpression);
+            }
+            return null;
+        }
+
+        public boolean isConstant() {
+            return this.rhsConstantCount == 1;
+        }
+
+        @Override
+        public Iterator<Expression> visitEnter(IsNullExpression 
isNullExpression) {
+            if(rhsConstantCount > 1) {
+                return null;
+            }
+            if(isNullExpression.isNegate()) {
+                return null;
+            }
+            Expression lhsExpresssion = isNullExpression.getChildren().get(0);
+            if(!this.columnExpression.equals(lhsExpresssion)) {
+                return null;
+            }
+            this.checkConstantValue(null);
+            return null;
+        }
+
+        private void checkConstantValue(Expression rhsExpression) {
+            if(!this.isNullExpressionVisited && 
this.firstRhsConstantExpression == null) {
+                this.firstRhsConstantExpression = rhsExpression;
+                rhsConstantCount++;
+                if(rhsExpression == null) {
+                    this.isNullExpressionVisited = true;
+                }
+                return;
+            }
+
+            if(!isExpressionEquals(this.isNullExpressionVisited ? null : 
this.firstRhsConstantExpression, rhsExpression)) {
+                rhsConstantCount++;
+                return;
+            }
+        }
+
+        private static boolean isExpressionEquals(Expression 
oldExpression,Expression newExpression) {
+            if(oldExpression == null) {
+                if(newExpression == null) {
+                    return true;
+                }
+                return ExpressionUtil.isNull(newExpression, new 
ImmutableBytesWritable());
+            }
+            if(newExpression == null) {
+                return ExpressionUtil.isNull(oldExpression, new 
ImmutableBytesWritable());
+            }
+            return oldExpression.equals(newExpression);
+        }
+    }
+
+    private static class IsCompositeLiteralExpressionVisitor extends 
StatelessTraverseAllExpressionVisitor<Boolean> {
+        @Override
+        public Boolean defaultReturn(Expression expression, List<Boolean> 
childResultValues) {
+            if (!ExpressionUtil.isContantForStatement(expression) ||
+                    childResultValues.size() < 
expression.getChildren().size()) {
+                return Boolean.FALSE;
+            }
+            for (Boolean childResultValue : childResultValues) {
+                if (!childResultValue) {
+                    return Boolean.FALSE;
+                }
+            }
+            return Boolean.TRUE;
+        }
+        @Override
+        public Boolean visit(LiteralExpression literalExpression) {
+            return Boolean.TRUE;
+        }
+    }
 }
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 154dd7a..68954b8 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
@@ -2951,6 +2951,129 @@ public class QueryCompilerTest extends 
BaseConnectionlessQueryTest {
             }
         }
     }
+
+    @Test
+    public void testOrderPreservingGroupByForNotPkColumns() throws Exception {
+         try (Connection conn= DriverManager.getConnection(getUrl())) {
+             conn.createStatement().execute("CREATE TABLE test (\n" +
+                    "            pk1 varchar, \n" +
+                    "            pk2 varchar, \n" +
+                    "            pk3 varchar, \n" +
+                    "            pk4 varchar, \n" +
+                    "            v1 varchar, \n" +
+                    "            v2 varchar,\n" +
+                    "            CONSTRAINT pk PRIMARY KEY (\n" +
+                    "               pk1,\n" +
+                    "               pk2,\n" +
+                    "               pk3,\n" +
+                    "               pk4\n" +
+                    "             )\n" +
+                    "         )");
+            String[] queries = new String[] {
+                    "SELECT pk3 FROM test WHERE v2 = 'a' GROUP BY 
substr(v2,0,1),pk3 ORDER BY pk3",
+                    "SELECT pk3 FROM test WHERE pk1 = 'c' and v2 = 
substr('abc',1,1) GROUP BY v2,pk3 ORDER BY pk3",
+                    "SELECT pk3 FROM test WHERE v1 = 'a' and v2 = 'b' GROUP BY 
length(v1)+length(v2),pk3 ORDER BY pk3",
+                    "SELECT pk3 FROM test WHERE pk1 = 'a' and v2 = 'b' GROUP 
BY length(pk1)+length(v2),pk3 ORDER BY pk3",
+                    "SELECT pk3 FROM test WHERE v1 = 'a' and v2 = 
substr('abc',2,1) GROUP BY pk4,CASE WHEN v1 > v2 THEN v1 ELSE v2 END,pk3 ORDER 
BY pk4,pk3",
+                    "SELECT pk3 FROM test WHERE pk1 = 'a' and v2 = 
substr('abc',2,1) GROUP BY pk4,CASE WHEN pk1 > v2 THEN pk1 ELSE v2 END,pk3 
ORDER BY pk4,pk3",
+                    "SELECT pk3 FROM test WHERE pk1 = 'a' and pk2 = 'b' and v1 
= 'c' GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 
END,pk3 ORDER BY 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 testOrderPreservingGroupByForClientAggregatePlan() throws 
Exception {
+        Connection conn = null;
+         try {
+             conn = DriverManager.getConnection(getUrl());
+             String tableName = "test_table";
+             String sql = "create table " + tableName + "( "+
+                     " pk1 varchar not null , " +
+                     " pk2 varchar not null, " +
+                     " pk3 varchar not null," +
+                     " v1 varchar, " +
+                     " v2 varchar, " +
+                     " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+                        "pk1,"+
+                        "pk2,"+
+                        "pk3 ))";
+             conn.createStatement().execute(sql);
+
+             String[] queries = new String[] {
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "group by a.ak3,a.av1 order by a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av2 = 'a' GROUP BY substr(a.av2,0,1),ak3 ORDER BY 
ak3",
+
+                   //for InListExpression
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av2 in('a') GROUP BY substr(a.av2,0,1),ak3 ORDER 
BY ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.ak1 = 'c' and a.av2 = substr('abc',1,1) GROUP BY 
a.av2,a.ak3 ORDER BY a.ak3",
+
+                   //for RVC
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where (a.ak1,a.av2) = ('c', substr('abc',1,1)) GROUP BY 
a.av2,a.ak3 ORDER BY a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av1 = 'a' and a.av2 = 'b' GROUP BY 
length(a.av1)+length(a.av2),a.ak3 ORDER BY a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from 
"+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.ak1 = 'a' and a.av2 = 'b' GROUP BY 
length(a.ak1)+length(a.av2),a.ak3 ORDER BY a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3, coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av1 = 'a' and a.av2 = substr('abc',2,1) GROUP BY 
a.ak4,CASE WHEN a.av1 > a.av2 THEN a.av1 ELSE a.av2 END,a.ak3 ORDER BY 
a.ak4,a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 = 0.0 and a.av2 = (5+3*2) GROUP BY a.ak3,CASE 
WHEN a.ak1 > a.av2 THEN a.ak1 ELSE a.av2 END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 = 0.0 and a.av2 = length(substr('abc',1,1)) 
GROUP BY a.ak3,CASE WHEN a.ak1 > a.av2 THEN a.ak1 ELSE a.av2 END,a.av1 ORDER BY 
a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 = 0.0 and a.av2 = length(substr('abc',1,1)) 
GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN 
coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   //for IS NULL
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 is null and a.av2 = length(substr('abc',1,1)) 
GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN 
coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 = 0.0 and a.av2 is null GROUP BY a.ak3,CASE 
WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE 
coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+             };
+              int index = 0;
+             for (String query : queries) {
+                 QueryPlan plan =  TestUtil.getOptimizeQueryPlan(conn, query);
+                 assertTrue((index + 1) + ") " + queries[index], 
plan.getOrderBy()== OrderBy.FWD_ROW_KEY_ORDER_BY);
+                 index++;
+             }
+         }
+         finally {
+             if(conn != null) {
+                 conn.close();
+             }
+         }
+    }
     
     @Test
     public void testNotOrderPreservingGroupBy() throws Exception {
@@ -2988,6 +3111,207 @@ public class QueryCompilerTest extends 
BaseConnectionlessQueryTest {
     }
     
     @Test
+    public void testNotOrderPreservingGroupByForNotPkColumns() throws 
Exception {
+        try (Connection conn= DriverManager.getConnection(getUrl())) {
+            conn.createStatement().execute("CREATE TABLE test (\n" +
+                    "            pk1 varchar,\n" +
+                    "            pk2 varchar,\n" +
+                    "            pk3 varchar,\n" +
+                    "            pk4 varchar,\n" +
+                    "            v1 varchar,\n" +
+                    "            v2 varchar,\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 = 'a' and pk2 = 'b') or 
v1 ='c' 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 = 'a' or pk2 = 'b' GROUP 
BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 END,pk3 ORDER 
BY pk3",
+                     "SELECT pk3 FROM test WHERE pk1 = 'a' and (pk2 = 'b' or 
v1 = 'c') GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE pk2 
END,pk3 ORDER BY pk3",
+                     "SELECT v2 FROM test GROUP BY v1,v2 ORDER BY v2",
+                     "SELECT pk3 FROM test WHERE v1 = 'a' GROUP BY v1,v2,pk3 
ORDER BY pk3",
+                     "SELECT length(pk3) FROM test WHERE v1 = 'a' GROUP BY 
RAND()+length(v1),length(v2),length(pk3) ORDER BY length(v2),length(pk3)",
+                     "SELECT length(pk3) FROM test WHERE v1 = 'a' and v2 = 'b' 
GROUP BY CASE WHEN v1 > v2 THEN length(v1) ELSE RAND(1) END,length(pk3) ORDER 
BY length(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 testNotOrderPreservingGroupByForClientAggregatePlan() throws 
Exception {
+        Connection conn = null;
+        try {
+            conn = DriverManager.getConnection(getUrl());
+            String tableName = "table_test";
+            String sql = "create table " + tableName + "( "+
+                    " pk1 varchar not null , " +
+                    " pk2 varchar not null, " +
+                    " pk3 varchar not null," +
+                    " v1 varchar, " +
+                    " v2 varchar, " +
+                    " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+                    "pk1,"+
+                    "pk2,"+
+                    "pk3 ))";
+            conn.createStatement().execute(sql);
+
+            String[] queries = new String[] {
+                  "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where (a.ak1 = 'a' and a.ak2 = 'b') or a.av1 ='c' GROUP BY 
a.ak4,CASE WHEN a.ak1 > a.ak2 THEN coalesce(a.av1,'1') ELSE a.ak2 END,a.ak3 
ORDER BY a.ak4,a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.ak1 = 'a' or a.ak2 = 'b' GROUP BY CASE WHEN a.ak1 
> a.ak2 THEN a.av1 WHEN a.ak1 = a.ak2 THEN a.ak1 ELSE a.ak2 END,a.ak3 ORDER BY 
a.ak3",
+
+                   //for in
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.ak1 in ( 'a','b') GROUP BY CASE WHEN a.ak1 > a.ak2 
THEN a.av1 WHEN a.ak1 = a.ak2 THEN a.ak1 ELSE a.ak2 END,a.ak3 ORDER BY a.ak3",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.ak1 = 'a' and (a.ak2 = 'b' or a.av1 = 'c') GROUP 
BY CASE WHEN a.ak1 > a.ak2 THEN a.av1 WHEN a.ak1 = a.ak2 THEN a.ak1 ELSE a.ak2 
END,a.ak3 ORDER BY a.ak3",
+
+                   "select a.av2 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "GROUP BY a.av1,a.av2 ORDER BY a.av2",
+
+                   "select a.ak3 "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av1 = 'a' GROUP BY a.av1,a.av2,a.ak3 ORDER BY 
a.ak3",
+
+                   "select length(a.ak3) "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av1 = 'a' GROUP BY 
RAND()+length(a.av1),length(a.av2),length(a.ak3) ORDER BY 
length(a.av2),length(a.ak3)",
+
+                   "select length(a.ak3) "+
+                   "from (select substr(pk1,1,1) ak1,substr(pk2,1,1) 
ak2,substr(pk3,1,1) ak3,coalesce(pk3,'1') ak4, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                   "where a.av1 = 'a' and a.av2 = 'b' GROUP BY CASE WHEN a.av1 
> a.av2 THEN length(a.av1) ELSE RAND(1) END,length(a.ak3) ORDER BY 
length(a.ak3)",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 > 0.0 and a.av2 = (5+3*2) GROUP BY a.ak3,CASE 
WHEN a.ak1 > a.av2 THEN a.ak1 ELSE a.av2 END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   //for CoerceExpression
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where CAST(a.ak1 AS INTEGER) = 0 and a.av2 = (5+3*2) GROUP 
BY a.ak3,a.ak1,a.av1 ORDER BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 = 0.0 or a.av2 = length(substr('abc',1,1)) 
GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN 
coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   //for IS NULL
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 is not null and a.av2 = 
length(substr('abc',1,1)) GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > 
coalesce(a.av2,2) THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER 
BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 is null or a.av2 = length(substr('abc',1,1)) 
GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) THEN 
coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 is null and a.av2 = length(substr('abc',1,1)) 
and a.ak1 = 0.0 GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) 
THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+
+                   "select a.ak3 "+
+                   "from (select rand() ak1,length(pk2) ak2,length(pk3) 
ak3,length(v1) av1,length(v2) av2 from "+tableName+" order by pk2,pk3 limit 10) 
a "+
+                   "where a.ak1 is null and a.av2 = length(substr('abc',1,1)) 
or a.ak1 = 0.0 GROUP BY a.ak3,CASE WHEN coalesce(a.ak1,1) > coalesce(a.av2,2) 
THEN coalesce(a.ak1,1) ELSE coalesce(a.av2,2) END,a.av1 ORDER BY a.ak3,a.av1",
+             };
+            int index = 0;
+            for (String query : queries) {
+                QueryPlan plan = TestUtil.getOptimizeQueryPlan(conn, query);
+                assertTrue((index + 1) + ") " + queries[index], 
plan.getOrderBy().getOrderByExpressions().size() > 0);
+                index++;
+            }
+        }
+        finally {
+            if(conn != null) {
+                conn.close();
+            }
+        }
+    }
+
+    @Test
+    public void testOrderByOptimizeForClientAggregatePlanAndDesc() throws 
Exception {
+        Connection conn = null;
+        try {
+            conn = DriverManager.getConnection(getUrl());
+            String tableName = "test_table";
+            String sql = "create table " + tableName + "( "+
+                    " pk1 varchar not null, " +
+                    " pk2 varchar not null, " +
+                    " pk3 varchar not null, " +
+                    " v1 varchar, " +
+                    " v2 varchar, " +
+                    " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+                    "pk1 desc,"+
+                    "pk2 desc,"+
+                    "pk3 desc))";
+            conn.createStatement().execute(sql);
+
+            String[] queries = new String[] {
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3, substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "group by a.ak3,a.av1 order by a.ak3 desc,a.av1",
+
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "where a.av1 = 'a' group by a.av1,a.ak3 order by a.ak3 
desc",
+
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "where a.av1 = 'a' and a.av2= 'b' group by CASE WHEN 
a.av1 > a.av2 THEN a.av1 ELSE a.av2 END,a.ak3,a.ak2 order by a.ak3 desc,a.ak2 
desc"
+            };
+
+            int index = 0;
+            for (String query : queries) {
+                QueryPlan plan =  TestUtil.getOptimizeQueryPlan(conn, query);
+                assertTrue((index + 1) + ") " + queries[index], 
plan.getOrderBy()== OrderBy.FWD_ROW_KEY_ORDER_BY);
+                index++;
+            }
+
+            queries = new String[] {
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "group by a.ak3,a.av1 order by a.ak3,a.av1",
+
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "where a.av1 = 'a' group by a.av1,a.ak3 order by a.ak3",
+
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "where a.av1 = 'a' and a.av2= 'b' group by CASE WHEN 
a.av1 > a.av2 THEN a.av1 ELSE a.av2 END,a.ak3,a.ak2 order by a.ak3,a.ak2",
+
+                     "select a.ak3 "+
+                     "from (select pk1 ak1,pk2 ak2,pk3 ak3,substr(v1,1,1) 
av1,substr(v2,1,1) av2 from "+tableName+" order by pk2,pk3 limit 10) a "+
+                     "where a.av1 = 'a' and a.av2= 'b' group by CASE WHEN 
a.av1 > a.av2 THEN a.av1 ELSE a.av2 END,a.ak3,a.ak2 order by a.ak3 asc,a.ak2 
desc"
+            };
+            index = 0;
+            for (String query : queries) {
+                QueryPlan plan = TestUtil.getOptimizeQueryPlan(conn, query);
+                assertTrue((index + 1) + ") " + queries[index], 
plan.getOrderBy().getOrderByExpressions().size() > 0);
+                index++;
+            }
+        }
+        finally {
+            if(conn != null) {
+                conn.close();
+            }
+        }
+    }
+
+    @Test
     public void testGroupByDescColumnBug3451() throws Exception {
 
         try (Connection conn= DriverManager.getConnection(getUrl())) {
diff --git 
a/phoenix-core/src/test/java/org/apache/phoenix/expression/ArithmeticOperationTest.java
 
b/phoenix-core/src/test/java/org/apache/phoenix/expression/ArithmeticOperationTest.java
index 7876fce..1b830f2 100644
--- 
a/phoenix-core/src/test/java/org/apache/phoenix/expression/ArithmeticOperationTest.java
+++ 
b/phoenix-core/src/test/java/org/apache/phoenix/expression/ArithmeticOperationTest.java
@@ -30,7 +30,7 @@ import java.util.List;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.phoenix.exception.DataExceedsCapacityException;
 import org.apache.phoenix.expression.function.RandomFunction;
-import 
org.apache.phoenix.expression.visitor.CloneNonDeterministicExpressionVisitor;
+import org.apache.phoenix.expression.visitor.CloneExpressionVisitor;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PDecimal;
 import org.apache.phoenix.schema.types.PInteger;
@@ -273,7 +273,7 @@ public class ArithmeticOperationTest {
         e2 = new DoubleSubtractExpression(children);
         e3 = new DoubleAddExpression(Arrays.<Expression>asList(e1, e2));
         e4 = new DoubleAddExpression(Arrays.<Expression>asList(new 
RandomFunction(Arrays.<Expression>asList(LiteralExpression.newConstant(null))), 
e3));
-        CloneNonDeterministicExpressionVisitor visitor = new 
CloneNonDeterministicExpressionVisitor();
+        CloneExpressionVisitor visitor = new CloneExpressionVisitor();
         Expression clone = e4.accept(visitor);
         assertTrue(clone != e4);
         e4.evaluate(null, ptr1);
@@ -281,7 +281,7 @@ public class ArithmeticOperationTest {
         assertNotEquals(ptr1, ptr2);
         
         e4 = new DoubleAddExpression(Arrays.<Expression>asList(new 
RandomFunction(Arrays.<Expression>asList(LiteralExpression.newConstant(1))), 
e3));
-        visitor = new CloneNonDeterministicExpressionVisitor();
+        visitor = new CloneExpressionVisitor();
         clone = e4.accept(visitor);
         assertTrue(clone == e4);
         e4.evaluate(null, ptr1);
@@ -294,7 +294,7 @@ public class ArithmeticOperationTest {
         boolean evaluated = e.evaluate(null, ptr);
         assertTrue(evaluated);
         assertEquals(value, type.toObject(ptr.get()));
-        CloneNonDeterministicExpressionVisitor visitor = new 
CloneNonDeterministicExpressionVisitor();
+        CloneExpressionVisitor visitor = new CloneExpressionVisitor();
         Expression clone = e.accept(visitor);
         evaluated = clone.evaluate(null, ptr);
         assertTrue(evaluated);

Reply via email to