You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by pb...@apache.org on 2018/11/27 15:18:27 UTC

[17/28] phoenix git commit: PHOENIX-4841 staging patch commit.

PHOENIX-4841 staging patch commit.


Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo
Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/1c656192
Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/1c656192
Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/1c656192

Branch: refs/heads/4.x-cdh5.15
Commit: 1c656192f6d0ea061630c7d1ef8ab3f0970e7071
Parents: bcf2cc7
Author: Daniel Wong <da...@salesforce.com>
Authored: Wed Oct 10 00:38:11 2018 +0100
Committer: Pedro Boado <pb...@apache.org>
Committed: Tue Nov 27 15:11:54 2018 +0000

----------------------------------------------------------------------
 .../org/apache/phoenix/end2end/QueryMoreIT.java | 171 +++++++++++++++++--
 .../apache/phoenix/compile/WhereOptimizer.java  |  58 ++++++-
 .../expression/ComparisonExpression.java        |  18 +-
 .../RowValueConstructorExpressionRewriter.java  |  54 ++++++
 .../org/apache/phoenix/schema/RowKeySchema.java |   4 +
 ...wValueConstructorExpressionRewriterTest.java |  78 +++++++++
 6 files changed, 362 insertions(+), 21 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java
index 04272fa..2b1d31e 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java
@@ -17,11 +17,13 @@
  */
 package org.apache.phoenix.end2end;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import com.google.common.collect.Lists;
+import org.apache.hadoop.hbase.util.Pair;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
 
 import java.sql.Connection;
 import java.sql.Date;
@@ -37,18 +39,19 @@ import java.util.Map;
 import java.util.Properties;
 
 import org.apache.hadoop.hbase.util.Base64;
-import org.apache.hadoop.hbase.util.Pair;
-import org.apache.phoenix.jdbc.PhoenixConnection;
-import org.apache.phoenix.query.QueryServices;
-import org.apache.phoenix.util.PhoenixRuntime;
-import org.apache.phoenix.util.TestUtil;
-import org.junit.Test;
 
-import com.google.common.collect.Lists;
+import static org.apache.phoenix.util.PhoenixRuntime.TENANT_ID_ATTRIB;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 
 public class QueryMoreIT extends ParallelStatsDisabledIT {
     
+    private final String TENANT_SPECIFIC_URL1 = getUrl() + ';' + TENANT_ID_ATTRIB + "=tenant1";
+    
     private String dataTableName;
     //queryAgainstTenantSpecificView = true, dataTableSalted = true 
     @Test
@@ -510,4 +513,148 @@ public class QueryMoreIT extends ParallelStatsDisabledIT {
             stmt.execute();
         }
     }
+
+    @Test public void testRVCWithDescAndAscendingPK() throws Exception {
+        final Connection conn = DriverManager.getConnection(getUrl());
+        String fullTableName = generateUniqueName();
+        try (Statement stmt = conn.createStatement()) {
+            stmt.execute("CREATE TABLE " + fullTableName + "(\n"
+                    + "    ORGANIZATION_ID CHAR(15) NOT NULL,\n" + "    SCORE VARCHAR NOT NULL,\n"
+                    + "    ENTITY_ID VARCHAR NOT NULL\n"
+                    + "    CONSTRAINT PAGE_SNAPSHOT_PK PRIMARY KEY (\n"
+                    + "        ORGANIZATION_ID,\n" + "        SCORE DESC,\n" + "        ENTITY_ID\n"
+                    + "    )\n" + ") MULTI_TENANT=TRUE");
+        }
+
+        conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','c','1')");
+        conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','3')");
+        conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','4')");
+        conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','a','2')");
+        conn.commit();
+
+        try (Statement stmt = conn.createStatement()) {
+            final ResultSet
+                    rs =
+                    stmt.executeQuery("SELECT score, entity_id \n" + "FROM " + fullTableName + "\n"
+                            + "WHERE organization_id = 'org1'\n"
+                            + "AND (score, entity_id) < ('b', '4')\n"
+                            + "ORDER BY score DESC, entity_id\n" + "LIMIT 3");
+            assertTrue(rs.next());
+            assertEquals("b", rs.getString(1));
+            assertEquals("3", rs.getString(2));
+            assertTrue(rs.next());
+            assertEquals("a", rs.getString(1));
+            assertEquals("2", rs.getString(2));
+            assertFalse(rs.next());
+        }
+    }
+
+    @Test
+    public void testRVCOnTenantViewThroughGlobalIdxOrderByDesc() throws Exception {
+        String fullTableName = generateUniqueName();
+        String fullViewName = generateUniqueName();
+        String tenantView = generateUniqueName();
+        String indexName = generateUniqueName();
+        // create base table and global view using global connection
+        try (Connection conn = DriverManager.getConnection(getUrl())) {
+            Statement stmt = conn.createStatement();
+            stmt.execute(
+                    "CREATE TABLE " + fullTableName + "(\n"
+                            + "    TENANT_ID CHAR(15) NOT NULL,\n"
+                            + "    KEY_PREFIX CHAR(3) NOT NULL,\n"
+                            + "    CREATED_DATE DATE,\n"
+                            + "    CREATED_BY CHAR(15),\n"
+                            + "    SYSTEM_MODSTAMP DATE\n"
+                            + "    CONSTRAINT PK PRIMARY KEY (\n"
+                            + "       TENANT_ID,"
+                            + "       KEY_PREFIX\n"
+                            + ")) MULTI_TENANT=TRUE");
+
+            stmt.execute("CREATE VIEW " + fullViewName + "(\n"
+                    + "    DATE_TIME1 DATE NOT NULL,\n"
+                    + "    TEXT2 VARCHAR,\n"
+                    + "    DOUBLE1 DECIMAL(12, 3),\n"
+                    + "    IS_BOOLEAN BOOLEAN,\n"
+                    + "    RELATIONSHIP_ID CHAR(15),\n"
+                    + "    TEXT1 VARCHAR,\n"
+                    + "    TEXT_READ_ONLY VARCHAR,\n"
+                    + "    JSON1 VARCHAR,\n"
+                    + "    IP_START_ADDRESS VARCHAR,\n"
+                    + "    CONSTRAINT PKVIEW PRIMARY KEY\n"
+                    + "    (\n"
+                    + "        DATE_TIME1, TEXT2, TEXT1\n"
+                    + "    )) AS SELECT * FROM " + fullTableName
+                    + "    WHERE KEY_PREFIX = '0CY'");
+
+            stmt.execute("CREATE INDEX " + indexName + " " + "ON " + fullViewName
+                    + " (TEXT1 DESC, TEXT2)\n"
+                    + "INCLUDE (CREATED_BY,\n"
+                    + "    RELATIONSHIP_ID,\n"
+                    + "    JSON1,\n"
+                    + "    DOUBLE1,\n"
+                    + "    IS_BOOLEAN,\n"
+                    + "    IP_START_ADDRESS,\n"
+                    + "    CREATED_DATE,\n"
+                    + "    SYSTEM_MODSTAMP,\n"
+                    + "    TEXT_READ_ONLY)");
+        }
+
+        // create and use an tenant specific view to write data
+        try (Connection viewConn = DriverManager.getConnection(TENANT_SPECIFIC_URL1)) {
+            Statement stmt = viewConn.createStatement();
+            stmt.execute("CREATE VIEW IF NOT EXISTS " + tenantView + " AS SELECT * FROM "
+                    + fullViewName);
+            viewConn.createStatement().execute("UPSERT INTO " + tenantView
+                    + "(DATE_TIME1, TEXT1, TEXT2) "
+                    + " VALUES (TO_DATE('2017-10-16 22:00:00', 'yyyy-MM-dd HH:mm:ss'), 'd', '1')");
+            viewConn.createStatement().execute("UPSERT INTO " + tenantView
+                    + "(DATE_TIME1, TEXT1, TEXT2) "
+                    + " VALUES (TO_DATE('2017-10-16 22:00:00', 'yyyy-MM-dd HH:mm:ss'), 'c', '2')");
+            viewConn.createStatement().execute("UPSERT INTO " + tenantView
+                    + "(DATE_TIME1, TEXT1, TEXT2) "
+                    + " VALUES (TO_DATE('2017-10-16 22:00:00', 'yyyy-MM-dd HH:mm:ss'), 'b', '3')");
+            viewConn.createStatement().execute("UPSERT INTO " + tenantView
+                    + "(DATE_TIME1, TEXT1, TEXT2) "
+                    + " VALUES (TO_DATE('2017-10-16 22:00:00', 'yyyy-MM-dd HH:mm:ss'), 'b', '4')");
+            viewConn.createStatement().execute("UPSERT INTO " + tenantView
+                    + "(DATE_TIME1, TEXT1, TEXT2) "
+                    + " VALUES (TO_DATE('2017-10-16 22:00:00', 'yyyy-MM-dd HH:mm:ss'), 'a', '4')");
+            viewConn.commit();
+
+            // query using desc order by so that the index is used
+            ResultSet
+                    rs =
+                    stmt.executeQuery("SELECT TEXT1, TEXT2 FROM " + tenantView
+                            + " WHERE (TEXT1, TEXT2) > ('b', '3') ORDER BY TEXT1 DESC, TEXT2");
+            assertTrue(rs.next());
+            assertEquals("d", rs.getString(1));
+            assertEquals("1", rs.getString(2));
+            assertTrue(rs.next());
+            assertEquals("c", rs.getString(1));
+            assertEquals("2", rs.getString(2));
+            assertTrue(rs.next());
+            assertEquals("b", rs.getString(1));
+            assertEquals("4", rs.getString(2));
+            assertFalse(rs.next());
+        }
+
+        // validate that running query using global view gives same results
+        try (Connection conn = DriverManager.getConnection(getUrl())) {
+            ResultSet
+                    rs =
+                    conn.createStatement().executeQuery("SELECT TEXT1, TEXT2 FROM " + fullViewName
+                            + " WHERE (TEXT1, TEXT2) > ('b', '3') ORDER BY TEXT1 DESC, TEXT2");
+            assertTrue(rs.next());
+            assertEquals("d", rs.getString(1));
+            assertEquals("1", rs.getString(2));
+            assertTrue(rs.next());
+            assertEquals("c", rs.getString(1));
+            assertEquals("2", rs.getString(2));
+            assertTrue(rs.next());
+            assertEquals("b", rs.getString(1));
+            assertEquals("4", rs.getString(2));
+            assertFalse(rs.next());
+        }
+    }
+
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
index 12e09d2..b2e4c41 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
@@ -227,14 +227,38 @@ public class WhereOptimizer {
             
             // Iterate through all spans of this slot
             while (true) {
-                SortOrder sortOrder =  schema.getField(slot.getPKPosition() + slotOffset).getSortOrder();
+                SortOrder sortOrder =
+                        schema.getField(slot.getPKPosition() + slotOffset).getSortOrder();
                 if (prevSortOrder == null)  {
                     prevSortOrder = sortOrder;
                 } else if (prevSortOrder != sortOrder) {
+                    //Consider the Universe of keys to be [0,7]+ on the leading column A
+                    // and [0,7]+ on trailing column B, with a padbyte of 0 for ASC and 7 for DESC
+                    //if our key range for ASC keys is leading [2,*] and trailing [3,*],
+                    //   → [x203 - x777]
+                    //for this particular plan the leading key is descending (ie index desc)
+                    // consider the data
+                    // (3,2) ORDER BY A,B→ x302 → ORDER BY A DESC,B → x472
+                    // (3,3) ORDER BY A,B→ x303 → ORDER BY A DESC,B → x473
+                    // (3,4) ORDER BY A,B→ x304 → ORDER BY A DESC,B → x474
+                    // (2,3) ORDER BY A,B→ x203 → ORDER BY A DESC,B → x573
+                    // (2,7) ORDER BY A,B→ x207 → ORDER BY A DESC,B → x577
+                    // And the logical expression (A,B) > (2,3)
+                    // In the DESC A order the selected values are not contiguous,
+                    // (2,7),(3,2),(3,3),(3,4)
+                    // In the normal ASC order by the values are all contiguous
+                    // Therefore the key cannot be extracted out and a full filter must be applied
+                    // In addition, the boundary of the scan is tricky as the values are not bound
+                    // by (2,3) it is instead bound by (2,7), this should map to, [x000,x577]
+                    // FUTURE: May be able to perform a type of skip scan for this case.
+
                     // If the sort order changes, we must clip the portion with the same sort order
                     // and invert the key ranges and swap the upper and lower bounds.
-                    List<KeyRange> leftRanges = clipLeft(schema, slot.getPKPosition() + slotOffset - clipLeftSpan, clipLeftSpan, keyRanges, ptr);
-                    keyRanges = clipRight(schema, slot.getPKPosition() + slotOffset - 1, keyRanges, leftRanges, ptr);
+                    List<KeyRange> leftRanges = clipLeft(schema, slot.getPKPosition()
+                            + slotOffset - clipLeftSpan, clipLeftSpan, keyRanges, ptr);
+                    keyRanges =
+                            clipRight(schema, slot.getPKPosition() + slotOffset - 1, keyRanges,
+                                    leftRanges, ptr);
                     if (prevSortOrder == SortOrder.DESC) {
                         leftRanges = invertKeyRanges(leftRanges);
                     }
@@ -242,6 +266,13 @@ public class WhereOptimizer {
                     cnf.add(leftRanges);
                     clipLeftSpan = 0;
                     prevSortOrder = sortOrder;
+                    // since we have to clip the portion with the same sort order, we can no longer
+                    // extract the nodes from the where clause
+                    // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query
+                    //   WHERE (A,B) < ('a','b')
+                    // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+                    // so we still need to filter on A,B
+                    stopExtracting = true;
                 }
                 clipLeftSpan++;
                 slotOffset++;
@@ -264,11 +295,12 @@ public class WhereOptimizer {
             // cardinality of this slot is low.
             /*
              *  Stop extracting nodes once we encounter:
-             *  1) An unbound range unless we're forcing a skip scan and havn't encountered
+             *  1) An unbound range unless we're forcing a skip scan and haven't encountered
              *     a multi-column span. Even if we're trying to force a skip scan, we can't
              *     execute it over a multi-column span.
              *  2) A non range key as we can extract the first one, but further ones need
              *     to be evaluated in a filter.
+             *  3) As above a non-contiguous range due to sort order
              */
             stopExtracting |= (hasUnboundedRange && !forcedSkipScan) || (hasRangeKey && forcedRangeScan);
             useSkipScan |= !stopExtracting && !forcedRangeScan && (keyRanges.size() > 1 || hasRangeKey);
@@ -2060,10 +2092,20 @@ public class WhereOptimizer {
 
                                 @Override
                                 public SortOrder getSortOrder() {
-                                    // The parts of the RVC have already been converted
-                                    // to ascending, so we don't need to consider the
-                                    // childPart sort order.
-                                    return SortOrder.ASC;
+                                    //See PHOENIX-4969: Clean up and unify code paths for RVCs with
+                                    //  respect to Optimizations for SortOrder
+                                    //Handle the different paths for InList vs Normal Comparison
+                                    //The code paths in InList assume the sortOrder is ASC for
+                                    // their optimizations
+                                    //The code paths for Comparisons on RVC rewrite equality,
+                                    // for the non-equality cases return actual sort order
+                                    //This work around should work
+                                    // but a more general approach can be taken.
+                                    if(rvcElementOp == CompareOp.EQUAL ||
+                                            rvcElementOp == CompareOp.NOT_EQUAL){
+                                        return SortOrder.ASC;
+                                    }
+                                    return childPart.getColumn().getSortOrder();
                                 }
 
                                 @Override

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
index 074ac0a..1810c3b 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
@@ -22,14 +22,18 @@ import java.io.DataOutput;
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.sql.SQLException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
+import com.google.common.collect.ImmutableList;
 import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.io.WritableUtils;
 import org.apache.phoenix.expression.function.ArrayElemRefExpression;
+import org.apache.phoenix.expression.function.InvertFunction;
+import org.apache.phoenix.expression.rewrite.RowValueConstructorExpressionRewriter;
 import org.apache.phoenix.expression.visitor.ExpressionVisitor;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.TypeMismatchException;
@@ -111,7 +115,7 @@ public class ComparisonExpression extends BaseCompoundExpression {
         Expression rhsExpr = children.get(1);
         PDataType lhsExprDataType = lhsExpr.getDataType();
         PDataType rhsExprDataType = rhsExpr.getDataType();
-        
+
         if ((lhsExpr instanceof RowValueConstructorExpression || rhsExpr instanceof RowValueConstructorExpression) && !(lhsExpr instanceof ArrayElemRefExpression) && !(rhsExpr instanceof ArrayElemRefExpression)) {
             if (op == CompareOp.EQUAL || op == CompareOp.NOT_EQUAL) {
                 List<Expression> andNodes = Lists.<Expression>newArrayListWithExpectedSize(Math.max(lhsExpr.getChildren().size(), rhsExpr.getChildren().size()));
@@ -128,6 +132,18 @@ public class ComparisonExpression extends BaseCompoundExpression {
             if ( ! ( lhsExpr instanceof RowValueConstructorExpression ) ) {
                 lhsExpr = new RowValueConstructorExpression(Collections.singletonList(lhsExpr), lhsExpr.isStateless());
             }
+
+            /*
+            At this point both sides should be in the same row format.
+            We add the inverts so the filtering can be done properly for mixed sort type RVCs.
+            The entire RVC has to be in ASC for the actual compare to work since compare simply does
+             a varbyte compare.  See PHOENIX-4841
+            */
+            RowValueConstructorExpressionRewriter rvcRewriter =
+                    RowValueConstructorExpressionRewriter.getSingleton();
+            lhsExpr = rvcRewriter.rewriteAllChildrenAsc((RowValueConstructorExpression) lhsExpr);
+            rhsExpr = rvcRewriter.rewriteAllChildrenAsc((RowValueConstructorExpression) rhsExpr);
+
             children = Arrays.asList(lhsExpr, rhsExpr);
         } else if(lhsExprDataType != null && rhsExprDataType != null && !lhsExprDataType.isComparableTo(rhsExprDataType)) {
             throw TypeMismatchException.newException(lhsExprDataType, rhsExprDataType,

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/main/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriter.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriter.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriter.java
new file mode 100644
index 0000000..fc0cafd
--- /dev/null
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriter.java
@@ -0,0 +1,54 @@
+/*
+ * 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.rewrite;
+
+import org.apache.phoenix.expression.CoerceExpression;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.RowValueConstructorExpression;
+import org.apache.phoenix.schema.SortOrder;
+
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.List;
+
+public class RowValueConstructorExpressionRewriter {
+
+    static RowValueConstructorExpressionRewriter singleton = null;
+
+    public static RowValueConstructorExpressionRewriter getSingleton() {
+        if (singleton == null) {
+            singleton = new RowValueConstructorExpressionRewriter();
+        }
+        return singleton;
+    }
+
+    public RowValueConstructorExpression rewriteAllChildrenAsc(
+            RowValueConstructorExpression rvcExpression) throws SQLException {
+        List<Expression> replacementChildren = new ArrayList<>(rvcExpression.getChildren().size());
+        for (int i = 0; i < rvcExpression.getChildren().size(); i++) {
+            Expression child = rvcExpression.getChildren().get(i);
+            if (child.getSortOrder() == SortOrder.DESC) {
+                //As The KeySlot visitor has not been setup for InvertFunction need to Use Coerce
+                child = CoerceExpression.create(child, child.getDataType(), SortOrder.ASC, null);
+            }
+            replacementChildren.add(child);
+        }
+        return rvcExpression.clone(replacementChildren);
+    }
+}

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java
index 3210516..01be40a 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java
@@ -431,6 +431,10 @@ public class RowKeySchema extends ValueSchema {
             ptr.set(upperRange, 0, ptr.getOffset() + ptr.getLength());
             upperRange = ByteUtil.copyKeyBytesIfNecessary(ptr);
         }
+        //Have to update the bounds to inclusive
+        //Consider a partial key on pk columns (INT A, INT B, ....)  and a predicate (A,B) > (3,5)
+        //This initial key as a row key would look like  (x0305 - *]
+        //If we were to clip the left to (x03 - *], we would skip values like (3,6)
         return KeyRange.getKeyRange(lowerRange, true, upperRange, true);
     }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/1c656192/phoenix-core/src/test/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriterTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriterTest.java b/phoenix-core/src/test/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriterTest.java
new file mode 100644
index 0000000..3ea0280
--- /dev/null
+++ b/phoenix-core/src/test/java/org/apache/phoenix/expression/rewrite/RowValueConstructorExpressionRewriterTest.java
@@ -0,0 +1,78 @@
+/*
+ * 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.rewrite;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.phoenix.expression.CoerceExpression;
+import org.apache.phoenix.expression.Determinism;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.RowValueConstructorExpression;
+import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.schema.types.PFloat;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.sql.SQLException;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class RowValueConstructorExpressionRewriterTest {
+    @Test
+    public void testRewriteAllChildrenAsc() throws SQLException {
+
+
+        Expression ascChild = Mockito.mock(Expression.class);
+        Mockito.when(ascChild.getSortOrder()).thenReturn(SortOrder.ASC);
+        Mockito.when(ascChild.getDataType()).thenReturn(PFloat.INSTANCE);
+        Mockito.when(ascChild.getDeterminism()).thenReturn(Determinism.ALWAYS);
+        Mockito.when(ascChild.requiresFinalEvaluation()).thenReturn(true);
+
+        Expression descChild = Mockito.mock(Expression.class);
+        Mockito.when(descChild.getSortOrder()).thenReturn(SortOrder.DESC);
+        Mockito.when(descChild.getDataType()).thenReturn(PFloat.INSTANCE);
+        Mockito.when(descChild.getDeterminism()).thenReturn(Determinism.ALWAYS);
+        Mockito.when(descChild.requiresFinalEvaluation()).thenReturn(true);
+
+        List<Expression> children = ImmutableList.of(ascChild,descChild);
+        RowValueConstructorExpression expression =
+                new RowValueConstructorExpression(children,false);
+
+
+        RowValueConstructorExpressionRewriter
+                rewriter =
+                RowValueConstructorExpressionRewriter.getSingleton();
+
+        RowValueConstructorExpression result = rewriter.rewriteAllChildrenAsc(expression);
+
+        assertEquals(2,result.getChildren().size());
+
+        Expression child1 = result.getChildren().get(0);
+        Expression child2 = result.getChildren().get(1);
+
+        assertEquals(SortOrder.ASC, child1.getSortOrder());
+        assertEquals(SortOrder.ASC, child2.getSortOrder());
+
+        assertEquals(ascChild, child1);
+        assertTrue(child2 instanceof CoerceExpression);
+        assertEquals(descChild, ((CoerceExpression)child2).getChild());
+
+    }
+}