You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by ja...@apache.org on 2015/07/21 02:53:49 UTC

phoenix git commit: PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns

Repository: phoenix
Updated Branches:
  refs/heads/master 936de8815 -> dcf845c25


PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns


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

Branch: refs/heads/master
Commit: dcf845c25eb9bd619a999d16ce9e2f548ce7b491
Parents: 936de88
Author: James Taylor <ja...@apache.org>
Authored: Mon Jul 20 17:52:53 2015 -0700
Committer: James Taylor <ja...@apache.org>
Committed: Mon Jul 20 17:53:18 2015 -0700

----------------------------------------------------------------------
 .../apache/phoenix/end2end/LpadFunctionIT.java  |  48 +++-
 .../org/apache/phoenix/compile/KeyPart.java     |   8 +-
 .../apache/phoenix/compile/WhereOptimizer.java  | 132 +++++++---
 .../UngroupedAggregateRegionObserver.java       |  33 ++-
 .../phoenix/expression/LiteralExpression.java   |   3 +-
 .../expression/function/InvertFunction.java     |   8 +-
 .../expression/function/PrefixFunction.java     |  13 +-
 .../expression/function/RTrimFunction.java      |  15 +-
 .../function/RoundDateExpression.java           |  12 +-
 .../function/RoundDecimalExpression.java        |  22 +-
 .../query/ConnectionQueryServicesImpl.java      |   8 +-
 .../org/apache/phoenix/schema/PTableImpl.java   |  25 +-
 .../apache/phoenix/schema/types/PBinary.java    |  28 +-
 .../org/apache/phoenix/schema/types/PChar.java  |  11 +
 .../apache/phoenix/schema/types/PDataType.java  |   1 +
 .../org/apache/phoenix/util/PhoenixRuntime.java |  43 +++-
 .../org/apache/phoenix/util/StringUtil.java     |   7 -
 .../org/apache/phoenix/util/UpgradeUtil.java    | 256 +++++++++++++------
 18 files changed, 495 insertions(+), 178 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
index 4aa66c2..4070103 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
@@ -16,6 +16,7 @@
  */
 package org.apache.phoenix.end2end;
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -27,8 +28,10 @@ import java.sql.ResultSet;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.query.QueryConstants;
+import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.TestUtil;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import com.google.common.collect.Lists;
@@ -107,26 +110,59 @@ public class LpadFunctionIT extends BaseHBaseManagedTimeIT {
         testLpad(conn, inputList, length, fillStringList, "pk", expectedOutputList);
     }
 
-    @Ignore
     @Test
     public void testCharPadding() throws Exception {
         ResultSet rs;
         Connection conn = DriverManager.getConnection(getUrl());
+        
+        conn.createStatement().execute("CREATE TABLE t (k CHAR(3) PRIMARY KEY)");
+        conn.createStatement().execute("UPSERT INTO t VALUES('a')");
+        conn.createStatement().execute("UPSERT INTO t VALUES('ab')");
+        conn.commit();
+        rs = conn.createStatement().executeQuery("SELECT * FROM t ORDER BY k");
+        assertTrue(rs.next());
+        assertEquals("a", rs.getString(1));
+        assertTrue(rs.next());
+        assertEquals("ab", rs.getString(1));
+        assertFalse(rs.next());
 
         conn.createStatement().execute("CREATE TABLE tdesc (k CHAR(3) PRIMARY KEY DESC)");
         conn.createStatement().execute("UPSERT INTO tdesc VALUES('a')");
+        conn.createStatement().execute("UPSERT INTO tdesc VALUES('ab')");
         conn.commit();
-        rs = conn.createStatement().executeQuery("SELECT * FROM tdesc");
+        rs = conn.createStatement().executeQuery("SELECT * FROM tdesc ORDER BY k DESC");
+        assertTrue(rs.next());
+        assertEquals("ab", rs.getString(1));
         assertTrue(rs.next());
         assertEquals("a", rs.getString(1));
         assertFalse(rs.next());
+    }
+
+    @Test
+    public void testBinaryPadding() throws Exception {
+        ResultSet rs;
+        Connection conn = DriverManager.getConnection(getUrl());
         
-        conn.createStatement().execute("CREATE TABLE t (k CHAR(3) PRIMARY KEY)");
+        conn.createStatement().execute("CREATE TABLE t (k BINARY(3) PRIMARY KEY)");
         conn.createStatement().execute("UPSERT INTO t VALUES('a')");
+        conn.createStatement().execute("UPSERT INTO t VALUES('ab')");
         conn.commit();
-        rs = conn.createStatement().executeQuery("SELECT * FROM t");
+        rs = conn.createStatement().executeQuery("SELECT * FROM t ORDER BY k");
         assertTrue(rs.next());
-        assertEquals("a", rs.getString(1));
+        assertArrayEquals(ByteUtil.concat(Bytes.toBytes("a"), QueryConstants.SEPARATOR_BYTE_ARRAY, QueryConstants.SEPARATOR_BYTE_ARRAY), rs.getBytes(1));
+        assertTrue(rs.next());
+        assertArrayEquals(ByteUtil.concat(Bytes.toBytes("ab"), QueryConstants.SEPARATOR_BYTE_ARRAY), rs.getBytes(1));
+        assertFalse(rs.next());
+
+        conn.createStatement().execute("CREATE TABLE tdesc (k BINARY(3) PRIMARY KEY DESC)");
+        conn.createStatement().execute("UPSERT INTO tdesc VALUES('a')");
+        conn.createStatement().execute("UPSERT INTO tdesc VALUES('ab')");
+        conn.commit();
+        rs = conn.createStatement().executeQuery("SELECT * FROM tdesc ORDER BY k DESC");
+        assertTrue(rs.next());
+        assertArrayEquals(ByteUtil.concat(Bytes.toBytes("ab"), QueryConstants.SEPARATOR_BYTE_ARRAY), rs.getBytes(1));
+        assertTrue(rs.next());
+        assertArrayEquals(ByteUtil.concat(Bytes.toBytes("a"), QueryConstants.SEPARATOR_BYTE_ARRAY, QueryConstants.SEPARATOR_BYTE_ARRAY), rs.getBytes(1));
         assertFalse(rs.next());
     }
 

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java
index 4eb53d3..55385f2 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java
@@ -20,10 +20,10 @@ package org.apache.phoenix.compile;
 import java.util.List;
 
 import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp;
-
 import org.apache.phoenix.expression.Expression;
 import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
 
 /**
  * 
@@ -74,4 +74,10 @@ public interface KeyPart {
      * @return the primary key column for this key part
      */
     public PColumn getColumn();
+    
+    /**
+     * Gets the table metadata object associated with this key part
+     * @return the table for this key part
+     */
+    public PTable getTable();
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/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 332f293..a270f12 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
@@ -63,7 +63,7 @@ import org.apache.phoenix.schema.SaltingUtil;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.ValueSchema.Field;
 import org.apache.phoenix.schema.tuple.Tuple;
-import org.apache.phoenix.schema.types.PArrayDataType;
+import org.apache.phoenix.schema.types.PBinary;
 import org.apache.phoenix.schema.types.PChar;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PVarbinary;
@@ -322,6 +322,14 @@ public class WhereOptimizer {
         }
     }
     
+    private static int computeUnpaddedLength(byte[] b, byte padByte) {
+        int len = b.length;
+        while (len > 0 && b[len - 1] == padByte) {
+            len--;
+        }
+        return len;                                       
+    }
+    
     // Special hack for PHOENIX-2067 to change the constant array to match
     // the separators used for descending, variable length arrays.
     // Note that there'd already be a coerce expression around the constant
@@ -335,27 +343,60 @@ public class WhereOptimizer {
             PTable table = context.getCurrentTable().getTable();
             // Constants are always build with rowKeyOptimizable as true, using the correct separators
             // We only need to do this conversion if we have a table that has not yet been converted.
-            if (type != null && type.isArrayType() && column.getSortOrder() == SortOrder.DESC && !PArrayDataType.arrayBaseType(type).isFixedWidth() && !table.rowKeyOrderOptimizable()) {
-                ImmutableBytesWritable ptr = context.getTempPtr();
-                List<KeyRange> newKeyRanges = Lists.newArrayListWithExpectedSize(keyRanges.size());
-                for (KeyRange keyRange : keyRanges) {
-                    byte[] lower = keyRange.getLowerRange();
-                    if (!keyRange.lowerUnbound()) {
-                        ptr.set(lower);;
-                        type.coerceBytes(ptr, null, type, null, null, SortOrder.DESC, null, null, SortOrder.DESC, false);
-                        lower = ByteUtil.copyKeyBytesIfNecessary(ptr);
+            if (type != null && !table.rowKeyOrderOptimizable()) {
+                if (type.isArrayType() && column.getSortOrder() == SortOrder.DESC) {
+                    ImmutableBytesWritable ptr = context.getTempPtr();
+                    List<KeyRange> newKeyRanges = Lists.newArrayListWithExpectedSize(keyRanges.size());
+                    for (KeyRange keyRange : keyRanges) {
+                        byte[] lower = keyRange.getLowerRange();
+                        if (!keyRange.lowerUnbound()) {
+                            ptr.set(lower);
+                            type.coerceBytes(ptr, null, type, null, null, SortOrder.DESC, null, null, SortOrder.DESC, false);
+                            lower = ByteUtil.copyKeyBytesIfNecessary(ptr);
+                        }
+                        byte[] upper = keyRange.getUpperRange();
+                        if (!keyRange.upperUnbound()) {
+                            ptr.set(upper);
+                            type.coerceBytes(ptr, null, type, null, null, SortOrder.DESC, null, null, SortOrder.DESC, false);
+                            upper = ByteUtil.copyKeyBytesIfNecessary(ptr);
+                        }
+                        keyRange = KeyRange.getKeyRange(lower, keyRange.isLowerInclusive(), upper, keyRange.isUpperInclusive());
+                        newKeyRanges.add(keyRange);
                     }
-                    byte[] upper = keyRange.getUpperRange();
-                    if (!keyRange.upperUnbound()) {
-                        ptr.set(upper);;
-                        type.coerceBytes(ptr, null, type, null, null, SortOrder.DESC, null, null, SortOrder.DESC, false);
-                        upper = ByteUtil.copyKeyBytesIfNecessary(ptr);
+                    return newKeyRanges;
+                } else if (type == PBinary.INSTANCE || (column.getSortOrder() == SortOrder.DESC && type == PChar.INSTANCE)) {
+                    // Since the table has not been upgraded, we need to replace the correct trailing byte back to the incorrect
+                    // trailing byte so that we form the correct start/stop key
+                    List<KeyRange> newKeyRanges = Lists.newArrayListWithExpectedSize(keyRanges.size());
+                    byte byteToReplace;
+                    if (type == PBinary.INSTANCE) {
+                        byteToReplace = column.getSortOrder() == SortOrder.ASC ? QueryConstants.SEPARATOR_BYTE : QueryConstants.DESC_SEPARATOR_BYTE;
+                    } else {
+                        byteToReplace = StringUtil.INVERTED_SPACE_UTF8;
+                    }
+                    for (KeyRange keyRange : keyRanges) {
+                        byte[] lower = keyRange.getLowerRange();
+                        if (!keyRange.lowerUnbound()) {
+                            int len = computeUnpaddedLength(lower, byteToReplace);
+                            if (len != lower.length) {
+                                lower = Arrays.copyOf(lower, len);
+                                lower = StringUtil.padChar(lower, lower.length);
+                            }
+                        }
+                        byte[] upper = keyRange.getUpperRange();
+                        if (!keyRange.upperUnbound()) {
+                            int len = computeUnpaddedLength(upper, byteToReplace);
+                            if (len != upper.length) {
+                                upper = Arrays.copyOf(upper, len);
+                                upper = StringUtil.padChar(upper, upper.length);
+                            }
+                        }
+                        keyRange = KeyRange.getKeyRange(lower, keyRange.isLowerInclusive(), upper, keyRange.isUpperInclusive());
+                        newKeyRanges.add(keyRange);
                     }
-                    keyRange = KeyRange.getKeyRange(lower, keyRange.isLowerInclusive(), upper, keyRange.isUpperInclusive());
-                    newKeyRanges.add(keyRange);
+                    return newKeyRanges;
                 }
                 
-                return newKeyRanges;
             }
         }
         return keyRanges;
@@ -532,7 +573,7 @@ public class WhereOptimizer {
             List<Expression> extractNodes = extractNode == null || slot.getKeyPart().getExtractNodes().isEmpty()
                   ? Collections.<Expression>emptyList()
                   : Collections.<Expression>singletonList(extractNode);
-            return new SingleKeySlot(new BaseKeyPart(slot.getKeyPart().getColumn(), extractNodes), slot.getPKPosition(), slot.getPKSpan(), keyRanges, minMaxRange, slot.getOrderPreserving());
+            return new SingleKeySlot(new BaseKeyPart(table, slot.getKeyPart().getColumn(), extractNodes), slot.getPKPosition(), slot.getPKSpan(), keyRanges, minMaxRange, slot.getOrderPreserving());
         }
 
         private KeySlots newKeyParts(KeySlot slot, List<Expression> extractNodes, List<KeyRange> keyRanges, KeyRange minMaxRange) {
@@ -540,7 +581,7 @@ public class WhereOptimizer {
                 return EMPTY_KEY_SLOTS;
             }
             
-            return new SingleKeySlot(new BaseKeyPart(slot.getKeyPart().getColumn(), extractNodes), slot.getPKPosition(), slot.getPKSpan(), keyRanges, minMaxRange, slot.getOrderPreserving());
+            return new SingleKeySlot(new BaseKeyPart(table, slot.getKeyPart().getColumn(), extractNodes), slot.getPKPosition(), slot.getPKSpan(), keyRanges, minMaxRange, slot.getOrderPreserving());
         }
 
         private KeySlots newRowValueConstructorKeyParts(RowValueConstructorExpression rvc, List<KeySlots> childSlots) {
@@ -648,6 +689,11 @@ public class WhereOptimizer {
                 public PColumn getColumn() {
                     return childPart.getColumn();
                 }
+
+                @Override
+                public PTable getTable() {
+                    return childPart.getTable();
+                }
             }, slot.getPKPosition(), slot.getKeyRanges());
         }
 
@@ -724,7 +770,7 @@ public class WhereOptimizer {
 
             if (!minMaxExtractNodes.isEmpty()) {
                 if (keySlot[initPosition] == null) {
-                    keySlot[initPosition] = new KeySlot(new BaseKeyPart(table.getPKColumns().get(initPosition), minMaxExtractNodes), initPosition, 1, EVERYTHING_RANGES, null);
+                    keySlot[initPosition] = new KeySlot(new BaseKeyPart(table, table.getPKColumns().get(initPosition), minMaxExtractNodes), initPosition, 1, EVERYTHING_RANGES, null);
                 } else {
                     keySlot[initPosition] = keySlot[initPosition].concatExtractNodes(minMaxExtractNodes);
                 }
@@ -841,7 +887,7 @@ public class WhereOptimizer {
                 slotRanges = Collections.emptyList();
             }
             if (theSlot == null) {
-                theSlot = new KeySlot(new BaseKeyPart(table.getPKColumns().get(initialPos), slotExtractNodes), initialPos, 1, EVERYTHING_RANGES, null);
+                theSlot = new KeySlot(new BaseKeyPart(table, table.getPKColumns().get(initialPos), slotExtractNodes), initialPos, 1, EVERYTHING_RANGES, null);
             } else if (minMaxRange != KeyRange.EMPTY_RANGE && !slotExtractNodes.isEmpty()) {
                 theSlot = theSlot.concatExtractNodes(slotExtractNodes);
             }
@@ -926,7 +972,7 @@ public class WhereOptimizer {
         @Override
         public KeySlots visit(RowKeyColumnExpression node) {
             PColumn column = table.getPKColumns().get(node.getPosition());
-            return new SingleKeySlot(new BaseKeyPart(column, Collections.<Expression>singletonList(node)), node.getPosition(), 1, EVERYTHING_RANGES);
+            return new SingleKeySlot(new BaseKeyPart(table, column, Collections.<Expression>singletonList(node)), node.getPosition(), 1, EVERYTHING_RANGES);
         }
 
         @Override
@@ -997,7 +1043,8 @@ public class WhereOptimizer {
             KeySlots childSlots = childParts.get(0);
             KeySlot childSlot = childSlots.iterator().next();
             final String startsWith = node.getLiteralPrefix();
-            byte[] key = PVarchar.INSTANCE.toBytes(startsWith, node.getChildren().get(0).getSortOrder());
+            SortOrder sortOrder = node.getChildren().get(0).getSortOrder();
+            byte[] key = PVarchar.INSTANCE.toBytes(startsWith, sortOrder);
             // If the expression is an equality expression against a fixed length column
             // and the key length doesn't match the column length, the expression can
             // never be true.
@@ -1014,8 +1061,15 @@ public class WhereOptimizer {
             byte[] upperRange = ByteUtil.nextKey(key);
             Integer columnFixedLength = column.getMaxLength();
             if (type.isFixedWidth() && columnFixedLength != null) {
-                lowerRange = StringUtil.padChar(lowerRange, columnFixedLength);
-                upperRange = StringUtil.padChar(upperRange, columnFixedLength);
+                if (table.rowKeyOrderOptimizable()) {
+                    // Always use minimum byte to fill as otherwise our key is bigger
+                    // that it should be when the sort order is descending.
+                    lowerRange = type.pad(lowerRange, columnFixedLength, SortOrder.ASC);
+                    upperRange = type.pad(upperRange, columnFixedLength, SortOrder.ASC);
+                } else { // TODO: remove broken logic once tables are required to have been upgraded for PHOENIX-2067 and PHOENIX-2120
+                    lowerRange = StringUtil.padChar(lowerRange, columnFixedLength);
+                    upperRange = StringUtil.padChar(upperRange, columnFixedLength);
+                }
             }
             KeyRange keyRange = type.getKeyRange(lowerRange, true, upperRange, false);
             // Only extract LIKE expression if pattern ends with a wildcard and everything else was extracted
@@ -1117,7 +1171,7 @@ public class WhereOptimizer {
 
             public final KeySlot concatExtractNodes(List<Expression> extractNodes) {
                 return new KeySlot(
-                        new BaseKeyPart(this.getKeyPart().getColumn(),
+                        new BaseKeyPart(this.getKeyPart().getTable(), this.getKeyPart().getColumn(),
                                     SchemaUtil.concat(this.getKeyPart().getExtractNodes(),extractNodes)),
                         this.getPKPosition(),
                         this.getPKSpan(),
@@ -1135,7 +1189,7 @@ public class WhereOptimizer {
                         return null;
                     }
                     return new KeySlot(
-                            new BaseKeyPart(this.getKeyPart().getColumn(),
+                            new BaseKeyPart(this.getKeyPart().getTable(), this.getKeyPart().getColumn(),
                                         SchemaUtil.concat(this.getKeyPart().getExtractNodes(),
                                                           that.getKeyPart().getExtractNodes())),
                             this.getPKPosition(),
@@ -1181,7 +1235,7 @@ public class WhereOptimizer {
                             return null;
                         }
                         return new KeySlot(
-                                new BaseKeyPart(this.getKeyPart().getColumn(),
+                                new BaseKeyPart(this.getKeyPart().getTable(), this.getKeyPart().getColumn(),
                                             SchemaUtil.concat(this.getKeyPart().getExtractNodes(),
                                                               that.getKeyPart().getExtractNodes())),
                                 this.getPKPosition(),
@@ -1272,10 +1326,12 @@ public class WhereOptimizer {
                 return ByteUtil.getKeyRange(key, op, type);
             }
 
+            private final PTable table;
             private final PColumn column;
             private final List<Expression> nodes;
 
-            private BaseKeyPart(PColumn column, List<Expression> nodes) {
+            private BaseKeyPart(PTable table, PColumn column, List<Expression> nodes) {
+                this.table = table;
                 this.column = column;
                 this.nodes = nodes;
             }
@@ -1289,9 +1345,14 @@ public class WhereOptimizer {
             public PColumn getColumn() {
                 return column;
             }
+
+            @Override
+            public PTable getTable() {
+                return table;
+            }
         }
         
-        private  class RowValueConstructorKeyPart implements KeyPart {
+        private class RowValueConstructorKeyPart implements KeyPart {
             private final RowValueConstructorExpression rvc;
             private final PColumn column;
             private final List<Expression> nodes;
@@ -1319,7 +1380,13 @@ public class WhereOptimizer {
             public PColumn getColumn() {
                 return column;
             }
-           @Override
+
+            @Override
+            public PTable getTable() {
+                return table;
+            }
+
+            @Override
             public KeyRange getKeyRange(CompareOp op, Expression rhs) {
                // With row value constructors, we need to convert the operator for any transformation we do on individual values
                // to prevent keys from being increased to the next key as would be done for fixed width values. The next key is
@@ -1446,7 +1513,6 @@ public class WhereOptimizer {
                 byte[] key = ByteUtil.copyKeyBytesIfNecessary(ptr);
                 return ByteUtil.getKeyRange(key, op, PVarbinary.INSTANCE);
             }
-
         }
     }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
index 788a342..a7e3e44 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
@@ -85,7 +85,8 @@ import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.ValueSchema.Field;
 import org.apache.phoenix.schema.stats.StatisticsCollector;
 import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
-import org.apache.phoenix.schema.types.PArrayDataType;
+import org.apache.phoenix.schema.types.PBinary;
+import org.apache.phoenix.schema.types.PChar;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.IndexUtil;
@@ -95,6 +96,7 @@ import org.apache.phoenix.util.MetaDataUtil;
 import org.apache.phoenix.util.ScanUtil;
 import org.apache.phoenix.util.SchemaUtil;
 import org.apache.phoenix.util.ServerUtil;
+import org.apache.phoenix.util.StringUtil;
 import org.apache.phoenix.util.TimeKeeper;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -296,11 +298,30 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver{
                                         break;
                                     }
                                     Field field = schema.getField(i);
-                                    // Special case for re-writing DESC ARRAY, as the actual byte value needs to change in this case
-                                    if (field.getDataType().isArrayType() && field.getSortOrder() == SortOrder.DESC && !PArrayDataType.arrayBaseType(field.getDataType()).isFixedWidth()) {
-                                        field.getDataType().coerceBytes(ptr, null, field.getDataType(),
-                                                field.getMaxLength(), field.getScale(), field.getSortOrder(), 
-                                                field.getMaxLength(), field.getScale(), field.getSortOrder(), true); // force to use correct separator byte
+                                    if (field.getSortOrder() == SortOrder.DESC) {
+                                        // Special case for re-writing DESC ARRAY, as the actual byte value needs to change in this case
+                                        if (field.getDataType().isArrayType()) {
+                                            field.getDataType().coerceBytes(ptr, null, field.getDataType(),
+                                                    field.getMaxLength(), field.getScale(), field.getSortOrder(), 
+                                                    field.getMaxLength(), field.getScale(), field.getSortOrder(), true); // force to use correct separator byte
+                                        }
+                                        // Special case for re-writing DESC CHAR or DESC BINARY, to force the re-writing of trailing space characters
+                                        else if (field.getDataType() == PChar.INSTANCE || field.getDataType() == PBinary.INSTANCE) {
+                                            int len = ptr.getLength();
+                                            while (len > 0 && ptr.get()[ptr.getOffset() + len - 1] == StringUtil.SPACE_UTF8) {
+                                                len--;
+                                            }
+                                            ptr.set(ptr.get(), ptr.getOffset(), len);
+                                        }
+                                    } else if (field.getDataType() == PBinary.INSTANCE) {
+                                        // Remove trailing space characters so that the setValues call below will replace them
+                                        // with the correct zero byte character. Note this is somewhat dangerous as these
+                                        // could be legit, but I don't know what the alternative is.
+                                        int len = ptr.getLength();
+                                        while (len > 0 && ptr.get()[ptr.getOffset() + len - 1] == StringUtil.SPACE_UTF8) {
+                                            len--;
+                                        }
+                                        ptr.set(ptr.get(), ptr.getOffset(), len);                                        
                                     }
                                     values[i] = ptr.copyBytes();
                                 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
index 26c076c..c1faf66 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
@@ -37,7 +37,6 @@ import org.apache.phoenix.schema.types.PTimestamp;
 import org.apache.phoenix.schema.types.PVarchar;
 import org.apache.phoenix.schema.types.PhoenixArray;
 import org.apache.phoenix.util.ByteUtil;
-import org.apache.phoenix.util.StringUtil;
 
 import com.google.common.base.Preconditions;
 
@@ -174,7 +173,7 @@ public class LiteralExpression extends BaseTerminalExpression {
         byte[] b = type.toBytes(value, sortOrder);
         if (type == PVarchar.INSTANCE || type == PChar.INSTANCE) {
             if (type == PChar.INSTANCE && maxLength != null  && b.length < maxLength) {
-                b = StringUtil.padChar(b, maxLength);
+                b = type.pad(b, maxLength, sortOrder);
             } else if (value != null) {
                 maxLength = ((String)value).length();
             }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
index 3dcbf7c..6a3e2a1 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
@@ -28,9 +28,10 @@ import org.apache.phoenix.parse.FunctionParseNode.Argument;
 import org.apache.phoenix.parse.FunctionParseNode.BuiltInFunction;
 import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.schema.PColumn;
-import org.apache.phoenix.schema.types.PDataType;
+import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.schema.types.PDataType;
 
 @BuiltInFunction(name = InvertFunction.NAME, args = { @Argument() })
 public class InvertFunction extends ScalarFunction {
@@ -107,6 +108,11 @@ public class InvertFunction extends ScalarFunction {
             public PColumn getColumn() {
                 return childPart.getColumn();
             }
+
+            @Override
+            public PTable getTable() {
+                return childPart.getTable();
+            }
         };
     }
 

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
index 3373df7..111c8b3 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
@@ -26,9 +26,10 @@ import org.apache.phoenix.compile.KeyPart;
 import org.apache.phoenix.expression.Expression;
 import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.util.ByteUtil;
-import org.apache.phoenix.util.StringUtil;
 
 abstract public class PrefixFunction extends ScalarFunction {
     public PrefixFunction() {
@@ -84,16 +85,22 @@ abstract public class PrefixFunction extends ScalarFunction {
                     return childPart.getKeyRange(op, rhs);
                 }
                 Integer length = getColumn().getMaxLength();
+                SortOrder sortOrder = getColumn().getSortOrder();
                 if (type.isFixedWidth() && length != null) {
                     if (lowerRange != KeyRange.UNBOUND) {
-                        lowerRange = StringUtil.padChar(lowerRange, length);
+                        lowerRange = type.pad(lowerRange, length, sortOrder);
                     }
                     if (upperRange != KeyRange.UNBOUND) {
-                        upperRange = StringUtil.padChar(upperRange, length);
+                        upperRange = type.pad(upperRange, length, sortOrder);
                     }
                 }
                 return KeyRange.getKeyRange(lowerRange, lowerInclusive, upperRange, false);
             }
+
+            @Override
+            public PTable getTable() {
+                return childPart.getTable();
+            }
         };
     }
 

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
index 911ed19..aa632e2 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
@@ -29,10 +29,11 @@ import org.apache.phoenix.parse.FunctionParseNode.Argument;
 import org.apache.phoenix.parse.FunctionParseNode.BuiltInFunction;
 import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.schema.PColumn;
-import org.apache.phoenix.schema.types.PDataType;
-import org.apache.phoenix.schema.types.PVarchar;
+import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.schema.types.PDataType;
+import org.apache.phoenix.schema.types.PVarchar;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.StringUtil;
 
@@ -123,12 +124,13 @@ public class RTrimFunction extends ScalarFunction {
                     return childPart.getKeyRange(op, rhs);
                 }
                 Integer length = getColumn().getMaxLength();
+                SortOrder sortOrder = getColumn().getSortOrder();
                 if (type.isFixedWidth() && length != null) {
                     if (lowerRange != KeyRange.UNBOUND) {
-                        lowerRange = StringUtil.padChar(lowerRange, length);
+                        lowerRange = type.pad(lowerRange, length, sortOrder);
                     }
                     if (upperRange != KeyRange.UNBOUND) {
-                        upperRange = StringUtil.padChar(upperRange, length);
+                        upperRange = type.pad(upperRange, length, sortOrder);
                     }
                 }
                 return KeyRange.getKeyRange(lowerRange, lowerInclusive, upperRange, false);
@@ -143,6 +145,11 @@ public class RTrimFunction extends ScalarFunction {
             public PColumn getColumn() {
                 return childPart.getColumn();
             }
+
+            @Override
+            public PTable getTable() {
+                return childPart.getTable();
+            }
         };
     }
 

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
index d8fa1dc..e410e34 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
@@ -33,13 +33,14 @@ import org.apache.phoenix.expression.Determinism;
 import org.apache.phoenix.expression.Expression;
 import org.apache.phoenix.expression.LiteralExpression;
 import org.apache.phoenix.query.KeyRange;
-import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.schema.tuple.Tuple;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PDataType.PDataCodec;
+import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.types.PVarchar;
-import org.apache.phoenix.schema.SortOrder;
-import org.apache.phoenix.schema.tuple.Tuple;
 import org.apache.phoenix.util.ByteUtil;
 
 import com.google.common.collect.Lists;
@@ -255,6 +256,11 @@ public class RoundDateExpression extends ScalarFunction {
                     return childPart.getKeyRange(op, rhs);
                 }
             }
+
+            @Override
+            public PTable getTable() {
+                return childPart.getTable();
+            }
         };
     }
 

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
index e81650f..7f44082 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
@@ -23,26 +23,27 @@ import java.io.IOException;
 import java.math.BigDecimal;
 import java.math.RoundingMode;
 import java.sql.SQLException;
+import java.util.Collections;
 import java.util.List;
 
+import org.apache.hadoop.hbase.filter.CompareFilter;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.io.WritableUtils;
+import org.apache.phoenix.compile.KeyPart;
 import org.apache.phoenix.expression.Determinism;
 import org.apache.phoenix.expression.Expression;
 import org.apache.phoenix.expression.LiteralExpression;
-import org.apache.phoenix.schema.types.PDecimal;
+import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.schema.IllegalDataException;
-import org.apache.phoenix.schema.types.PInteger;
+import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.tuple.Tuple;
 import org.apache.phoenix.schema.types.PDataType;
+import org.apache.phoenix.schema.types.PDecimal;
+import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.types.PLong;
-import org.apache.phoenix.schema.tuple.Tuple;
 
 import com.google.common.collect.Lists;
-import java.util.Collections;
-import org.apache.hadoop.hbase.filter.CompareFilter;
-import org.apache.phoenix.compile.KeyPart;
-import org.apache.phoenix.query.KeyRange;
-import org.apache.phoenix.schema.PColumn;
 
 /**
  *
@@ -274,6 +275,11 @@ public class RoundDecimalExpression extends ScalarFunction {
                 // otherwise, rounding has not affected the operator, so return normally
                 return rounded;
             }
+
+            @Override
+            public PTable getTable() {
+                return childPart.getTable();
+            }
         };
     }
     

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
index a17e28a..dede98d 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
@@ -1983,9 +1983,13 @@ public class ConnectionQueryServicesImpl extends DelegateQueryServices implement
                                     props.remove(PhoenixRuntime.TENANT_ID_ATTRIB);
                                     PhoenixConnection conn = new PhoenixConnection(ConnectionQueryServicesImpl.this, metaConnection.getURL(), props, metaConnection.getMetaDataCache());
                                     try {
-                                        Set<String> tablesNeedingUpgrade = UpgradeUtil.getPhysicalTablesWithDescVarLengthRowKey(conn);
+                                        List<String> tablesNeedingUpgrade = UpgradeUtil.getPhysicalTablesWithDescRowKey(conn);
                                         if (!tablesNeedingUpgrade.isEmpty()) {
-                                            logger.warn("The following tables require upgrade due to a bug causing the row key to be incorrect for descending columns (PHOENIX-2067):\n" + Joiner.on(' ').join(tablesNeedingUpgrade));
+                                            logger.warn("The following tables require upgrade due to a bug causing the row key to be incorrect for descending columns and ascending BINARY columns (PHOENIX-2067 and PHOENIX-2120):\n" + Joiner.on(' ').join(tablesNeedingUpgrade) + "\nTo upgrade issue the \"bin/psql.py -u\" command.");
+                                        }
+                                        List<String> unsupportedTables = UpgradeUtil.getPhysicalTablesWithDescVarbinaryRowKey(conn);
+                                        if (!unsupportedTables.isEmpty()) {
+                                            logger.warn("The following tables use an unsupported VARBINARY DESC construct and need to be changed:\n" + Joiner.on(' ').join(unsupportedTables));
                                         }
                                     } catch (Exception ex) {
                                         logger.error("Unable to determine tables requiring upgrade due to PHOENIX-2067", ex);

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
index 1756c2f..521fb42 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
@@ -54,6 +54,8 @@ import org.apache.phoenix.schema.RowKeySchema.RowKeySchemaBuilder;
 import org.apache.phoenix.schema.stats.GuidePostsInfo;
 import org.apache.phoenix.schema.stats.PTableStats;
 import org.apache.phoenix.schema.stats.PTableStatsImpl;
+import org.apache.phoenix.schema.types.PBinary;
+import org.apache.phoenix.schema.types.PChar;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PVarchar;
 import org.apache.phoenix.util.ByteUtil;
@@ -125,8 +127,8 @@ public class PTableImpl implements PTable {
     private IndexType indexType;
     private PTableStats tableStats = PTableStats.EMPTY_STATS;
     private int baseColumnCount;
-    private boolean hasDescVarLengthColumns;
-    private boolean rowKeyOrderOptimizable;
+    private boolean rowKeyOrderOptimizable; // TODO: remove when required that tables have been upgrade for PHOENIX-2067
+    private boolean hasColumnsRequiringUpgrade; // TODO: remove when required that tables have been upgrade for PHOENIX-2067
 
     public PTableImpl() {
         this.indexes = Collections.emptyList();
@@ -405,7 +407,8 @@ public class PTableImpl implements PTable {
         for (PColumn column : allColumns) {
             PName familyName = column.getFamilyName();
             if (familyName == null) {
-                hasDescVarLengthColumns |= (column.getSortOrder() == SortOrder.DESC && !column.getDataType().isFixedWidth());
+                hasColumnsRequiringUpgrade |= (column.getSortOrder() == SortOrder.DESC && (!column.getDataType().isFixedWidth() || column.getDataType() == PChar.INSTANCE || column.getDataType() == PBinary.INSTANCE))
+                        || (column.getSortOrder() == SortOrder.ASC && column.getDataType() == PBinary.INSTANCE && column.getMaxLength() != null && column.getMaxLength() > 1);
             	pkColumns.add(column);
             }
             if (familyName == null) {
@@ -547,8 +550,16 @@ public class PTableImpl implements PTable {
                     throw new ConstraintViolationException(name.getString() + "." + column.getName().getString() + " may not be null");
                 }
                 Integer	maxLength = column.getMaxLength();
-                if (maxLength != null && type.isFixedWidth() && byteValue.length <= maxLength) {
-                    byteValue = StringUtil.padChar(byteValue, maxLength);
+                if (maxLength != null && type.isFixedWidth() && byteValue.length < maxLength) {
+                    if (rowKeyOrderOptimizable()) {
+                        key.set(byteValue);
+                        type.pad(key, maxLength, sortOrder);
+                        byteValue = ByteUtil.copyKeyBytesIfNecessary(key);
+                    } else {
+                        // TODO: remove this incorrect code and move StringUtil.padChar() to TestUtil
+                        // once we require tables to have been upgraded
+                        byteValue = StringUtil.padChar(byteValue, maxLength);
+                    }
                 } else if (maxLength != null && byteValue.length > maxLength) {
                     throw new DataExceedsCapacityException(name.getString() + "." + column.getName().getString() + " may not exceed " + maxLength + " bytes (" + SchemaUtil.toString(type, byteValue) + ")");
                 }
@@ -733,7 +744,7 @@ public class PTableImpl implements PTable {
                         HConstants.EMPTY_BYTE_ARRAY : byteValue);
                 Integer	maxLength = column.getMaxLength();
             	if (!isNull && type.isFixedWidth() && maxLength != null) {
-    				if (ptr.getLength() <= maxLength) {
+    				if (ptr.getLength() < maxLength) {
                         type.pad(ptr, maxLength, column.getSortOrder());
                     } else if (ptr.getLength() > maxLength) {
                         throw new DataExceedsCapacityException(name.getString() + "." + column.getName().getString() + " may not exceed " + maxLength + " bytes (" + type.toObject(byteValue) + ")");
@@ -1131,6 +1142,6 @@ public class PTableImpl implements PTable {
 
     @Override
     public boolean rowKeyOrderOptimizable() {
-        return rowKeyOrderOptimizable || !hasDescVarLengthColumns;
+        return rowKeyOrderOptimizable || !hasColumnsRequiringUpgrade;
     }
 }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java
index b397554..8e0c4b5 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java
@@ -19,11 +19,13 @@ package org.apache.phoenix.schema.types;
 
 import java.sql.Types;
 import java.text.Format;
+import java.util.Arrays;
 
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.hbase.util.Base64;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.phoenix.exception.DataExceedsCapacityException;
+import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.schema.SortOrder;
 
 public class PBinary extends PDataType<byte[]> {
@@ -35,29 +37,43 @@ public class PBinary extends PDataType<byte[]> {
   }
 
   @Override
+  public byte[] pad(byte[] b, Integer maxLength, SortOrder sortOrder) {
+      if (b == null || b.length >= maxLength) {
+          return b;
+        }
+        byte[] newBytes = new byte[maxLength];
+        System.arraycopy(b, 0, newBytes, 0, b.length);
+        if (sortOrder == SortOrder.DESC) {
+            Arrays.fill(newBytes, b.length, maxLength, QueryConstants.DESC_SEPARATOR_BYTE);
+        }
+        return newBytes;
+  }
+  
+  @Override
   public void pad(ImmutableBytesWritable ptr, Integer maxLength, SortOrder sortOrder) {
     if (ptr.getLength() >= maxLength) {
       return;
     }
     byte[] newBytes = new byte[maxLength];
     System.arraycopy(ptr.get(), ptr.getOffset(), newBytes, 0, ptr.getLength());
+    if (sortOrder == SortOrder.DESC) {
+        Arrays.fill(newBytes, ptr.getLength(), maxLength, QueryConstants.DESC_SEPARATOR_BYTE);
+    }
     ptr.set(newBytes);
   }
 
   @Override
   public Object pad(Object object, Integer maxLength) {
     byte[] b = (byte[]) object;
-    if (b == null) {
-      return new byte[maxLength];
-    }
-    if (b.length == maxLength) {
+    int length = (b == null ? 0 : b.length);
+    if (length == maxLength) {
       return object;
     }
-    if (b.length > maxLength) {
+    if (length > maxLength) {
       throw new DataExceedsCapacityException(this, maxLength, null);
     }
     byte[] newBytes = new byte[maxLength];
-    System.arraycopy(b, 0, newBytes, 0, b.length);
+    System.arraycopy(b, 0, newBytes, 0, length);
 
     return newBytes;
   }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java
index c7cc1c1..6b26197 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java
@@ -53,6 +53,17 @@ public class PChar extends PDataType<String> {
     }
 
     @Override
+    public byte[] pad(byte[] b, Integer maxLength, SortOrder sortOrder) {
+      if (b == null || b.length >= maxLength) {
+        return b;
+      }
+      byte[] newBytes = new byte[maxLength];
+      System.arraycopy(b, 0, newBytes, 0, b.length);
+      Arrays.fill(newBytes, b.length, maxLength, sortOrder == SortOrder.ASC ? StringUtil.SPACE_UTF8 : StringUtil.INVERTED_SPACE_UTF8);
+      return newBytes;
+    }
+
+    @Override
     public Object pad(Object object, Integer maxLength) {
       String s = (String) object;
       if (s == null) {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
index 2c91dc5..43bab0e 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
@@ -1147,6 +1147,7 @@ public abstract class PDataType<T> implements DataType<T>, Comparable<PDataType<
     }
 
     public void pad(ImmutableBytesWritable ptr, Integer maxLength, SortOrder sortOrder) {}
+    public byte[] pad(byte[] b, Integer maxLength, SortOrder sortOrder) { return b; }
 
     public static PDataType arrayBaseType(PDataType arrayType) {
         Preconditions.checkArgument(arrayType.isArrayType(), "Not a phoenix array type");

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java b/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
index 1a2019d..c99d47e 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
@@ -193,17 +193,28 @@ public class PhoenixRuntime {
                     .unwrap(PhoenixConnection.class);
 
             if (execCmd.isUpgrade()) {
+                if (conn.getClientInfo(PhoenixRuntime.CURRENT_SCN_ATTRIB) != null) {
+                    throw new SQLException("May not specify the CURRENT_SCN property when upgrading");
+                }
+                if (conn.getClientInfo(PhoenixRuntime.TENANT_ID_ATTRIB) != null) {
+                    throw new SQLException("May not specify the TENANT_ID_ATTRIB property when upgrading");
+                }
                 if (execCmd.getInputFiles().isEmpty()) {
-                    Set<String> tablesNeedingUpgrade = UpgradeUtil.getPhysicalTablesWithDescVarLengthRowKey(conn);
+                    List<String> tablesNeedingUpgrade = UpgradeUtil.getPhysicalTablesWithDescRowKey(conn);
                     if (tablesNeedingUpgrade.isEmpty()) {
-                        String msg = "No tables are required to be upgraded due to incorrect row key order for descending, variable length columsn (PHOENIX-2067)";
+                        String msg = "No tables are required to be upgraded due to incorrect row key order (PHOENIX-2067 and PHOENIX-2120)";
                         System.out.println(msg);
                     } else {
-                        String msg = "The following tables require upgrade due to a bug causing the row key to be incorrect for descending columns (PHOENIX-2067):\n" + Joiner.on(' ').join(tablesNeedingUpgrade);
+                        String msg = "The following tables require upgrade due to a bug causing the row key to be incorrectly ordered (PHOENIX-2067 and PHOENIX-2120):\n" + Joiner.on(' ').join(tablesNeedingUpgrade);
+                        System.out.println("WARNING: " + msg);
+                    }
+                    List<String> unsupportedTables = UpgradeUtil.getPhysicalTablesWithDescVarbinaryRowKey(conn);
+                    if (!unsupportedTables.isEmpty()) {
+                        String msg = "The following tables use an unsupported VARBINARY DESC construct and need to be changed:\n" + Joiner.on(' ').join(unsupportedTables);
                         System.out.println("WARNING: " + msg);
                     }
                 } else {
-                    UpgradeUtil.upgradeDescVarLengthRowKeys(conn, execCmd.getInputFiles());
+                    UpgradeUtil.upgradeDescVarLengthRowKeys(conn, execCmd.getInputFiles(), execCmd.isBypassUpgrade());
                 }
             } else {
                 for (String inputFile : execCmd.getInputFiles()) {
@@ -476,6 +487,7 @@ public class PhoenixRuntime {
         private boolean strict;
         private List<String> inputFiles;
         private boolean isUpgrade;
+        private boolean isBypassUpgrade;
 
         /**
          * Factory method to build up an {@code ExecutionCommand} based on supplied parameters.
@@ -502,9 +514,18 @@ public class PhoenixRuntime {
                     "Define the array element separator, defaults to ':'");
             Option upgradeOption = new Option("u", "upgrade", false, "Upgrades tables specified as arguments " +
                     "by rewriting them with the correct row key for descending columns. If no arguments are " +
-                    "specified, then tables that need to be upgraded will be displayed. " +
+                    "specified, then tables that need to be upgraded will be displayed without being upgraded. " +
+                    "Use the -b option to bypass the rewrite if you know that your data does not need to be upgrade. " +
+                    "This would only be the case if you have not relied on auto padding for BINARY and CHAR data, " +
+                    "but instead have always provided data up to the full max length of the column. See PHOENIX-2067 " +
+                    "and PHOENIX-2120 for more information. " +
                     "Note that " + QueryServices.THREAD_TIMEOUT_MS_ATTRIB + " and hbase.regionserver.lease.period " +
                     "parameters must be set very high to prevent timeouts when upgrading.");
+            Option bypassUpgradeOption = new Option("b", "bypass-upgrade", false,
+                    "Used in conjunction with the -u option to bypass the rewrite during upgrade if you know that your data does not need to be upgrade. " +
+                    "This would only be the case if you have not relied on auto padding for BINARY and CHAR data, " +
+                    "but instead have always provided data up to the full max length of the column. See PHOENIX-2067 " +
+                    "and PHOENIX-2120 for more information. ");
             Options options = new Options();
             options.addOption(tableOption);
             options.addOption(headerOption);
@@ -514,6 +535,7 @@ public class PhoenixRuntime {
             options.addOption(escapeCharacterOption);
             options.addOption(arrayValueSeparatorOption);
             options.addOption(upgradeOption);
+            options.addOption(bypassUpgradeOption);
 
             CommandLineParser parser = new PosixParser();
             CommandLine cmdLine = null;
@@ -558,6 +580,13 @@ public class PhoenixRuntime {
                 execCmd.isUpgrade = true;
             }
 
+            if (cmdLine.hasOption(bypassUpgradeOption.getOpt())) {
+                if (!execCmd.isUpgrade()) {
+                    usageError("The bypass-upgrade option may only be used in conjunction with the -u option", options);
+                }
+                execCmd.isBypassUpgrade = true;
+            }
+
 
             List<String> argList = Lists.newArrayList(cmdLine.getArgList());
             if (argList.isEmpty()) {
@@ -649,6 +678,10 @@ public class PhoenixRuntime {
         public boolean isUpgrade() {
             return isUpgrade;
         }
+
+        public boolean isBypassUpgrade() {
+            return isBypassUpgrade;
+        }
     }
     
     /**

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java
index 89ae43b..4d3a36f 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java
@@ -268,13 +268,6 @@ public class StringUtil {
         return getFirstNonBlankCharIdxFromEnd(b, offset, length, sortOrder) - offset + 1;
     }
 
-    public static byte[] padChar(byte[] value, int offset, int length, int paddedLength) {
-        byte[] key = new byte[paddedLength];
-        System.arraycopy(value,offset, key, 0, length);
-        Arrays.fill(key, length, paddedLength, SPACE_UTF8);
-        return key;
-    }
-
     public static byte[] padChar(byte[] value, Integer byteSize) {
         byte[] newValue = Arrays.copyOf(value, byteSize);
         if (newValue.length > value.length) {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/dcf845c2/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
index b4fcef8..2ab4f8b 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
@@ -79,11 +79,14 @@ import org.apache.phoenix.schema.PTable.LinkType;
 import org.apache.phoenix.schema.PTableType;
 import org.apache.phoenix.schema.SaltingUtil;
 import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.schema.types.PBinary;
 import org.apache.phoenix.schema.types.PBoolean;
+import org.apache.phoenix.schema.types.PChar;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PDecimal;
 import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.types.PLong;
+import org.apache.phoenix.schema.types.PVarbinary;
 import org.apache.phoenix.schema.types.PVarchar;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -878,10 +881,17 @@ public class UpgradeUtil {
         return otherTables;
     }
     
-    // Return all types that are not fixed width that may need upgrading due to PHOENIX-2067
+    // Return all types that are descending and either:
+    // 1) variable length, which includes all array types (PHOENIX-2067)
+    // 2) fixed length with padding (PHOENIX-2120)
     // We exclude VARBINARY as we no longer support DESC for it.
     private static String getAffectedDataTypes() {
-        StringBuilder buf = new StringBuilder("(" + PVarchar.INSTANCE.getSqlType() + "," + PDecimal.INSTANCE.getSqlType() + ",");
+        StringBuilder buf = new StringBuilder("(" 
+                + PVarchar.INSTANCE.getSqlType() + "," +
+                + PChar.INSTANCE.getSqlType() + "," +
+                + PBinary.INSTANCE.getSqlType() + "," +
+                + PDecimal.INSTANCE.getSqlType() + ","
+                );
         for (PDataType type : PDataType.values()) {
             if (type.isArrayType()) {
                 buf.append(type.getSqlType());
@@ -891,20 +901,46 @@ public class UpgradeUtil {
         buf.setCharAt(buf.length()-1, ')');
         return buf.toString();
     }
+    
+    
     /**
-     * Identify the tables that need to be upgraded due to PHOENIX-2067
+     * Identify the tables that are DESC VARBINARY as this is no longer supported
      */
-    public static Set<String> getPhysicalTablesWithDescVarLengthRowKey(PhoenixConnection conn) throws SQLException {
-        // First query finds column rows of tables that need to be upgraded.
-        // We cannot tell if the column is from a table, view, or index however.
-        ResultSet rs = conn.createStatement().executeQuery(
-                "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
+    public static List<String> getPhysicalTablesWithDescVarbinaryRowKey(PhoenixConnection conn) throws SQLException {
+        String query = "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
                 "FROM SYSTEM.CATALOG cat1\n" + 
                 "WHERE COLUMN_NAME IS NOT NULL\n" + 
                 "AND COLUMN_FAMILY IS NULL\n" + 
                 "AND SORT_ORDER = " + SortOrder.DESC.getSystemValue() + "\n" + 
-                "AND DATA_TYPE IN " + getAffectedDataTypes() + "\n" +
-                "GROUP BY TENANT_ID,TABLE_SCHEM,TABLE_NAME");
+                "AND DATA_TYPE = " + PVarbinary.INSTANCE.getSqlType() + "\n" +
+                "GROUP BY TENANT_ID,TABLE_SCHEM,TABLE_NAME";
+        return getPhysicalTablesWithDescRowKey(query, conn);
+    }
+    
+    /**
+     * Identify the tables that need to be upgraded due to PHOENIX-2067 and PHOENIX-2120
+     */
+    public static List<String> getPhysicalTablesWithDescRowKey(PhoenixConnection conn) throws SQLException {
+        String query = "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
+                "FROM SYSTEM.CATALOG cat1\n" + 
+                "WHERE COLUMN_NAME IS NOT NULL\n" + 
+                "AND COLUMN_FAMILY IS NULL\n" + 
+                "AND ( ( SORT_ORDER = " + SortOrder.DESC.getSystemValue() + "\n" + 
+                "        AND DATA_TYPE IN " + getAffectedDataTypes() + ")\n" +
+                "    OR ( SORT_ORDER = " + SortOrder.ASC.getSystemValue() + "\n" + 
+                "         AND DATA_TYPE = " + PBinary.INSTANCE.getSqlType() + "\n" +
+                "         AND COLUMN_SIZE > 1 ) )\n" +
+                "GROUP BY TENANT_ID,TABLE_SCHEM,TABLE_NAME";
+        return getPhysicalTablesWithDescRowKey(query, conn);
+    }
+    
+    /**
+     * Identify the tables that need to be upgraded due to PHOENIX-2067
+     */
+    private static List<String> getPhysicalTablesWithDescRowKey(String query, PhoenixConnection conn) throws SQLException {
+        // First query finds column rows of tables that need to be upgraded.
+        // We cannot tell if the column is from a table, view, or index however.
+        ResultSet rs = conn.createStatement().executeQuery(query);
         Set<String> physicalTables = Sets.newHashSetWithExpectedSize(1024);
         List<String> remainingTableNames = addPhysicalTables(conn, rs, PTableType.INDEX, physicalTables);
         if (!remainingTableNames.isEmpty()) {
@@ -931,71 +967,129 @@ public class UpgradeUtil {
                  addPhysicalTables(conn, rs, PTableType.TABLE, physicalTables);
              }
         }
-        return physicalTables;
+        List<String> sortedPhysicalTables = new ArrayList<String>(physicalTables);
+        Collections.sort(sortedPhysicalTables);
+        return sortedPhysicalTables;
     }
 
-    private static void upgradeDescVarLengthRowKeys(PhoenixConnection upgradeConn, PhoenixConnection globalConn, String schemaName, String tableName, boolean isTable) throws SQLException {
-        String escapedTableName = SchemaUtil.getEscapedTableName(schemaName, tableName);
-        String tenantInfo = "";
-        PName tenantId = PName.EMPTY_NAME;
-        if (upgradeConn.getTenantId() != null) {
-            tenantId = upgradeConn.getTenantId();
-            tenantInfo = " for tenant " + tenantId.getString();
+    private static void upgradeDescVarLengthRowKeys(PhoenixConnection upgradeConn, PhoenixConnection globalConn, String schemaName, String tableName, boolean isTable, boolean bypassUpgrade) throws SQLException {
+        String physicalName = SchemaUtil.getTableName(schemaName, tableName);
+        long currentTime = System.currentTimeMillis();
+        String snapshotName = physicalName + "_" + currentTime;
+        HBaseAdmin admin = null;
+        if (isTable && !bypassUpgrade) {
+            admin = globalConn.getQueryServices().getAdmin();
         }
-        String msg = "Starting upgrade of " + escapedTableName + tenantInfo + "...";
-        System.out.println(msg);
-        logger.info(msg);
-        ResultSet rs = upgradeConn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ count(*) FROM " + escapedTableName);
-        rs.next(); // Run query
-        List<String> tableNames = Lists.newArrayListWithExpectedSize(1024);
-        tableNames.add(tenantId == PName.EMPTY_NAME ? null : tenantId.getString());
-        tableNames.add(schemaName);
-        tableNames.add(tableName);
-        // Find views to mark as upgraded
-        if (isTable) {
-            String physicalName = SchemaUtil.getTableName(schemaName, tableName);
-            String query =
-                    "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
-                    "FROM SYSTEM.CATALOG\n" + 
-                    "WHERE COLUMN_NAME IS NULL\n" + 
-                    "AND COLUMN_FAMILY = '" + physicalName + "'" +
-                    "AND LINK_TYPE = " + LinkType.PHYSICAL_TABLE.getSerializedValue();
-            rs = globalConn.createStatement().executeQuery(query);
-            while (rs.next()) {
-                tableNames.add(rs.getString(1));
-                tableNames.add(rs.getString(2));
-                tableNames.add(rs.getString(3));
+        boolean restoreSnapshot = false;
+        boolean success = false;
+        try {
+            if (isTable && !bypassUpgrade) {
+                String msg = "Taking snapshot of physical table " + physicalName + " prior to upgrade...";
+                System.out.println(msg);
+                logger.info(msg);
+                admin.disableTable(physicalName);
+                admin.snapshot(snapshotName, physicalName);
+                admin.enableTable(physicalName);
+                restoreSnapshot = true;
+            }
+            String escapedTableName = SchemaUtil.getEscapedTableName(schemaName, tableName);
+            String tenantInfo = "";
+            PName tenantId = PName.EMPTY_NAME;
+            if (upgradeConn.getTenantId() != null) {
+                tenantId = upgradeConn.getTenantId();
+                tenantInfo = " for tenant " + tenantId.getString();
+            }
+            String msg = "Starting upgrade of " + escapedTableName + tenantInfo + "...";
+            System.out.println(msg);
+            logger.info(msg);
+            ResultSet rs;
+            if (!bypassUpgrade) {
+                rs = upgradeConn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ count(*) FROM " + escapedTableName);
+                rs.next(); // Run query
+            }
+            List<String> tableNames = Lists.newArrayListWithExpectedSize(1024);
+            tableNames.add(tenantId == PName.EMPTY_NAME ? null : tenantId.getString());
+            tableNames.add(schemaName);
+            tableNames.add(tableName);
+            // Find views to mark as upgraded
+            if (isTable) {
+                String query =
+                        "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
+                        "FROM SYSTEM.CATALOG\n" + 
+                        "WHERE COLUMN_NAME IS NULL\n" + 
+                        "AND COLUMN_FAMILY = '" + physicalName + "'" +
+                        "AND LINK_TYPE = " + LinkType.PHYSICAL_TABLE.getSerializedValue();
+                rs = globalConn.createStatement().executeQuery(query);
+                while (rs.next()) {
+                    tableNames.add(rs.getString(1));
+                    tableNames.add(rs.getString(2));
+                    tableNames.add(rs.getString(3));
+                }
+            }
+            // Mark the table and views as upgraded now
+            for (int i = 0; i < tableNames.size(); i += 3) {
+                String theTenantId = tableNames.get(i);
+                String theSchemaName = tableNames.get(i+1);
+                String theTableName = tableNames.get(i+2);
+                globalConn.createStatement().execute("UPSERT INTO " + PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME + 
+                    " (" + PhoenixDatabaseMetaData.TENANT_ID + "," +
+                           PhoenixDatabaseMetaData.TABLE_SCHEM + "," +
+                           PhoenixDatabaseMetaData.TABLE_NAME + "," +
+                           MetaDataEndpointImpl.ROW_KEY_ORDER_OPTIMIZABLE + " BOOLEAN"
+                   + ") VALUES (" +
+                           "'" + (theTenantId == null ? StringUtil.EMPTY_STRING : theTenantId) + "'," +
+                           "'" + (theSchemaName == null ? StringUtil.EMPTY_STRING : theSchemaName) + "'," +
+                           "'" + theTableName + "'," +
+                           "TRUE)");
+            }
+            globalConn.commit();
+            for (int i = 0; i < tableNames.size(); i += 3) {
+                String theTenantId = tableNames.get(i);
+                String theSchemaName = tableNames.get(i+1);
+                String theTableName = tableNames.get(i+2);
+                globalConn.getQueryServices().clearTableFromCache(
+                        theTenantId == null ? ByteUtil.EMPTY_BYTE_ARRAY : Bytes.toBytes(theTenantId),
+                        theSchemaName == null ? ByteUtil.EMPTY_BYTE_ARRAY : Bytes.toBytes(schemaName),
+                        Bytes.toBytes(theTableName), HConstants.LATEST_TIMESTAMP);
+            }
+            success = true;
+            msg = "Completed upgrade of " + escapedTableName + tenantInfo;
+            System.out.println(msg);
+            logger.info(msg);
+        } catch (Exception e) {
+            logger.error("Exception during upgrade of " + physicalName + ":", e);
+        } finally {
+            boolean restored = false;
+            try {
+                if (!success && restoreSnapshot) {
+                    admin.disableTable(physicalName);
+                    admin.restoreSnapshot(snapshotName, false);
+                    admin.enableTable(physicalName);
+                    String msg = "Restored snapshot of " + physicalName + " due to failure of upgrade";
+                    System.out.println(msg);
+                    logger.info(msg);
+                }
+                restored = true;
+            } catch (Exception e) {
+                logger.warn("Unable to restoring snapshot " + snapshotName + " after failed upgrade", e);
+            } finally {
+                try {
+                    if (restoreSnapshot && restored) {
+                        admin.deleteSnapshot(snapshotName);
+                    }
+                } catch (Exception e) {
+                    logger.warn("Unable to delete snapshot " + snapshotName + " after upgrade:", e);
+                } finally {
+                    try {
+                        if (admin != null) {
+                            admin.close();
+                        }
+                    } catch (IOException e) {
+                        logger.warn("Unable to close admin after upgrade:", e);
+                    }
+                }
             }
         }
-        // Mark the table and views as upgraded now
-        for (int i = 0; i < tableNames.size(); i += 3) {
-            String theTenantId = tableNames.get(i);
-            String theSchemaName = tableNames.get(i+1);
-            String theTableName = tableNames.get(i+2);
-            globalConn.createStatement().execute("UPSERT INTO " + PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME + 
-                " (" + PhoenixDatabaseMetaData.TENANT_ID + "," +
-                       PhoenixDatabaseMetaData.TABLE_SCHEM + "," +
-                       PhoenixDatabaseMetaData.TABLE_NAME + "," +
-                       MetaDataEndpointImpl.ROW_KEY_ORDER_OPTIMIZABLE + " BOOLEAN"
-               + ") VALUES (" +
-                       "'" + (theTenantId == null ? StringUtil.EMPTY_STRING : theTenantId) + "'," +
-                       "'" + (theSchemaName == null ? StringUtil.EMPTY_STRING : theSchemaName) + "'," +
-                       "'" + theTableName + "'," +
-                       "TRUE)");
-        }
-        globalConn.commit();
-        for (int i = 0; i < tableNames.size(); i += 3) {
-            String theTenantId = tableNames.get(i);
-            String theSchemaName = tableNames.get(i+1);
-            String theTableName = tableNames.get(i+2);
-            globalConn.getQueryServices().clearTableFromCache(
-                    theTenantId == null ? ByteUtil.EMPTY_BYTE_ARRAY : Bytes.toBytes(theTenantId),
-                    theSchemaName == null ? ByteUtil.EMPTY_BYTE_ARRAY : Bytes.toBytes(schemaName),
-                    Bytes.toBytes(theTableName), HConstants.LATEST_TIMESTAMP);
-        }
-        msg = "Completed upgrade of " + escapedTableName + tenantInfo;
-        System.out.println(msg);
-        logger.info(msg);
     }
     
     private static boolean isInvalidTableToUpgrade(PTable table) throws SQLException {
@@ -1007,16 +1101,10 @@ public class UpgradeUtil {
      * Upgrade tables and their indexes due to a bug causing descending row keys to have a row key that
      * prevents them from being sorted correctly (PHOENIX-2067).
      */
-    public static void upgradeDescVarLengthRowKeys(PhoenixConnection conn, List<String> tablesToUpgrade) throws SQLException {
+    public static void upgradeDescVarLengthRowKeys(PhoenixConnection conn, List<String> tablesToUpgrade, boolean bypassUpgrade) throws SQLException {
         if (tablesToUpgrade.isEmpty()) {
             return;
         }
-        if (conn.getClientInfo(PhoenixRuntime.CURRENT_SCN_ATTRIB) != null) {
-            throw new SQLException("May not specify the CURRENT_SCN property when upgrading");
-        }
-        if (conn.getClientInfo(PhoenixRuntime.TENANT_ID_ATTRIB) != null) {
-            throw new SQLException("May not specify the TENANT_ID_ATTRIB property when upgrading");
-        }
         List<PTable> tablesNeedingUpgrading = Lists.newArrayListWithExpectedSize(tablesToUpgrade.size());
         List<String> invalidTables = Lists.newArrayListWithExpectedSize(tablesToUpgrade.size());
         for (String fullTableName : tablesToUpgrade) {
@@ -1042,23 +1130,23 @@ public class UpgradeUtil {
                 boolean wasUpgraded = false;
                 if (!table.rowKeyOrderOptimizable()) {
                     wasUpgraded = true;
-                    upgradeDescVarLengthRowKeys(upgradeConn, conn, table.getSchemaName().getString(), table.getTableName().getString(), true);
+                    upgradeDescVarLengthRowKeys(upgradeConn, conn, table.getSchemaName().getString(), table.getTableName().getString(), true, bypassUpgrade);
                 }
                 
                 // Upgrade global indexes
                 for (PTable index : table.getIndexes()) {
                     if (!index.rowKeyOrderOptimizable() && index.getIndexType() != IndexType.LOCAL) {
                         wasUpgraded = true;
-                        upgradeDescVarLengthRowKeys(upgradeConn, conn, index.getSchemaName().getString(), index.getTableName().getString(), false);
+                        upgradeDescVarLengthRowKeys(upgradeConn, conn, index.getSchemaName().getString(), index.getTableName().getString(), false, bypassUpgrade);
                     }
                 }
                 
                 String sharedViewIndexName = Bytes.toString(MetaDataUtil.getViewIndexPhysicalName(table.getName().getBytes()));
                 // Upgrade view indexes
-                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName);
+                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName, bypassUpgrade);
                 String sharedLocalIndexName = Bytes.toString(MetaDataUtil.getLocalIndexPhysicalName(table.getName().getBytes()));
-                // Upgrade view indexes
-                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName);
+                // Upgrade local indexes
+                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName, bypassUpgrade);
                 
                 if (!wasUpgraded) {
                     System.out.println("Upgrade not required for this table or its indexes: " + table.getName().getString());
@@ -1074,7 +1162,7 @@ public class UpgradeUtil {
      * physical table.
      * @return true if any upgrades were performed and false otherwise.
      */
-    private static boolean upgradeSharedIndex(PhoenixConnection upgradeConn, PhoenixConnection globalConn, String physicalName) throws SQLException {
+    private static boolean upgradeSharedIndex(PhoenixConnection upgradeConn, PhoenixConnection globalConn, String physicalName, boolean bypassUpgrade) throws SQLException {
         String query =
                 "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" + 
                 "FROM SYSTEM.CATALOG cat1\n" + 
@@ -1105,7 +1193,7 @@ public class UpgradeUtil {
             PTable table = PhoenixRuntime.getTable(conn, fullTableName);
             String tableTenantId = table.getTenantId() == null ? null : table.getTenantId().getString();
             if (Objects.equal(lastTenantId, tableTenantId) && !table.rowKeyOrderOptimizable()) {
-                upgradeDescVarLengthRowKeys(upgradeConn, globalConn, table.getSchemaName().getString(), table.getTableName().getString(), false);
+                upgradeDescVarLengthRowKeys(upgradeConn, globalConn, table.getSchemaName().getString(), table.getTableName().getString(), false, bypassUpgrade);
                 wasUpgraded = true;
             }
         }