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/09/11 09:32:05 UTC

[1/2] phoenix git commit: PHOENIX-2137 Range query on DECIMAL DESC sometimes incorrect

Repository: phoenix
Updated Branches:
  refs/heads/4.x-HBase-0.98 40f46b48f -> 18ec7d6a4


PHOENIX-2137 Range query on DECIMAL DESC sometimes incorrect

Conflicts:
	phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java


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

Branch: refs/heads/4.x-HBase-0.98
Commit: ac2b20838d025579015f677d1fe5a4e9cef3808d
Parents: 40f46b4
Author: James Taylor <jt...@salesforce.com>
Authored: Mon Aug 3 16:40:47 2015 -0700
Committer: James Taylor <jt...@salesforce.com>
Committed: Thu Sep 10 21:23:50 2015 -0700

----------------------------------------------------------------------
 .../org/apache/phoenix/end2end/SortOrderIT.java | 13 ++--
 .../org/apache/phoenix/compile/ScanRanges.java  |  7 ++-
 .../DescVarLengthFastByteComparisons.java       | 12 ++++
 .../apache/phoenix/filter/SkipScanFilter.java   | 13 ++--
 .../java/org/apache/phoenix/query/KeyRange.java | 42 ++++++-------
 .../apache/phoenix/schema/types/PDataType.java  |  3 +-
 .../java/org/apache/phoenix/util/ScanUtil.java  | 63 +++++++++++++++++---
 .../org/apache/phoenix/util/SchemaUtil.java     |  2 +-
 .../phoenix/compile/WhereOptimizerTest.java     | 20 +++++++
 .../DescVarLengthFastByteComparisonsTest.java   | 45 ++++++++++++++
 .../expression/SortOrderExpressionTest.java     | 13 ++--
 11 files changed, 186 insertions(+), 47 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
index 7cc0931..25782ba 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
@@ -41,8 +41,6 @@ import org.apache.commons.lang.ArrayUtils;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PDecimal;
-import org.apache.phoenix.schema.types.PDouble;
-import org.apache.phoenix.schema.types.PFloat;
 import org.apache.phoenix.util.PropertiesUtil;
 import org.junit.Assert;
 import org.junit.Test;
@@ -433,7 +431,7 @@ public class SortOrderIT extends BaseHBaseManagedTimeIT {
     public void testNonPKCompare() throws Exception {
         List<Integer> expectedResults = Lists.newArrayList(2,3,4);
         Integer[] saltBuckets = new Integer[] {null,3};
-        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE, PDouble.INSTANCE, PFloat.INSTANCE};
+        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE};
         for (Integer saltBucket : saltBuckets) {
             for (PDataType dataType : dataTypes) {
                 for (SortOrder sortOrder : SortOrder.values()) {
@@ -449,7 +447,7 @@ public class SortOrderIT extends BaseHBaseManagedTimeIT {
         List<Integer> rExpectedResults = new ArrayList<>(expectedResults);
         Collections.reverse(rExpectedResults);
         Integer[] saltBuckets = new Integer[] {null,3};
-        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE, PDouble.INSTANCE, PFloat.INSTANCE};
+        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE};
         for (Integer saltBucket : saltBuckets) {
             for (PDataType dataType : dataTypes) {
                 for (SortOrder sortOrder : SortOrder.values()) {
@@ -678,8 +676,13 @@ public class SortOrderIT extends BaseHBaseManagedTimeIT {
                 return ">";
             } else if (operator.equals(">")) {
                 return "<";
+            } else if (operator.equals(">=")) {
+                return "<=";
+            } else if (operator.equals("<=")) {
+                return ">=";
+            } else {
+                return operator;
             }
-            return operator;
         }
     }
     

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java
index aee5f63..2a032a3 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java
@@ -37,6 +37,7 @@ import org.apache.phoenix.schema.RowKeySchema;
 import org.apache.phoenix.schema.SaltingUtil;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.ScanUtil;
+import org.apache.phoenix.util.ScanUtil.BytesComparator;
 import org.apache.phoenix.util.SchemaUtil;
 
 import com.google.common.collect.ImmutableList;
@@ -81,10 +82,12 @@ public class ScanRanges {
                         stripPrefix(minMaxRange.getUpperRange(),offset), 
                         minMaxRange.upperUnbound());
             }
+            // We have full keys here, so use field from our varbinary schema
+            BytesComparator comparator = ScanUtil.getComparator(SchemaUtil.VAR_BINARY_SCHEMA.getField(0));
             for (byte[] key : keys) {
                 // Filter now based on unsalted minMaxRange and ignore the point key salt byte
-                if ( unsaltedMinMaxRange.compareLowerToUpperBound(key, offset, key.length-offset, true) <= 0 &&
-                     unsaltedMinMaxRange.compareUpperToLowerBound(key, offset, key.length-offset, true) >= 0) {
+                if ( unsaltedMinMaxRange.compareLowerToUpperBound(key, offset, key.length-offset, true, comparator) <= 0 &&
+                     unsaltedMinMaxRange.compareUpperToLowerBound(key, offset, key.length-offset, true, comparator) >= 0) {
                     keyRanges.add(KeyRange.getKeyRange(key));
                 }
             }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java b/phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java
index 40960e0..67d23fc 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java
@@ -90,6 +90,12 @@ public class DescVarLengthFastByteComparisons {
             public int compareTo(byte[] buffer1, int offset1, int length1, byte[] buffer2, int offset2, int length2) {
                 // Short circuit equal case
                 if (buffer1 == buffer2 && offset1 == offset2 && length1 == length2) { return 0; }
+                if (length1 == 0 && length2 != 0) { // nulls sort first, even for descending
+                    return -1; 
+                } 
+                if (length2 == 0 && length1 != 0) { // nulls sort first, even for descending
+                    return 1; 
+                }
                 // Bring WritableComparator code local
                 int end1 = offset1 + length1;
                 int end2 = offset2 + length2;
@@ -166,6 +172,12 @@ public class DescVarLengthFastByteComparisons {
             public int compareTo(byte[] buffer1, int offset1, int length1, byte[] buffer2, int offset2, int length2) {
                 // Short circuit equal case
                 if (buffer1 == buffer2 && offset1 == offset2 && length1 == length2) { return 0; }
+                if (length1 == 0 && length2 != 0) { // nulls sort first, even for descending
+                    return -1; 
+                }
+                if (length2 == 0 && length1 != 0) { // nulls sort first, even for descending
+                    return 1; 
+                }
                 int minLength = Math.min(length1, length2);
                 int minWords = minLength / Longs.BYTES;
                 int offset1Adj = offset1 + BYTE_ARRAY_BASE_OFFSET;

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java b/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java
index 6dc2a2d..d1c8532 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java
@@ -40,6 +40,7 @@ import org.apache.phoenix.query.KeyRange.Bound;
 import org.apache.phoenix.schema.RowKeySchema;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.ScanUtil;
+import org.apache.phoenix.util.ScanUtil.BytesComparator;
 import org.apache.phoenix.util.SchemaUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -210,7 +211,7 @@ public class SkipScanFilter extends FilterBase implements Writable {
         if (!lowerUnbound) {
             // Find the position of the first slot of the lower range
             schema.next(ptr, 0, schema.iterator(lowerInclusiveKey,ptr), slotSpan[0]);
-            startPos = ScanUtil.searchClosestKeyRangeWithUpperHigherThanPtr(slots.get(0), ptr, 0);
+            startPos = ScanUtil.searchClosestKeyRangeWithUpperHigherThanPtr(slots.get(0), ptr, 0, schema.getField(0));
             // Lower range is past last upper range of first slot, so cannot possibly be in range
             if (startPos >= slots.get(0).size()) {
                 return false;
@@ -221,7 +222,7 @@ public class SkipScanFilter extends FilterBase implements Writable {
         if (!upperUnbound) {
             // Find the position of the first slot of the upper range
             schema.next(ptr, 0, schema.iterator(upperExclusiveKey,ptr), slotSpan[0]);
-            endPos = ScanUtil.searchClosestKeyRangeWithUpperHigherThanPtr(slots.get(0), ptr, startPos);
+            endPos = ScanUtil.searchClosestKeyRangeWithUpperHigherThanPtr(slots.get(0), ptr, startPos, schema.getField(0));
             // Upper range lower than first lower range of first slot, so cannot possibly be in range
 //            if (endPos == 0 && Bytes.compareTo(upperExclusiveKey, slots.get(0).get(0).getLowerRange()) <= 0) {
 //                return false;
@@ -230,7 +231,7 @@ public class SkipScanFilter extends FilterBase implements Writable {
             if (endPos >= slots.get(0).size()) {
                 upperUnbound = true;
                 endPos = slots.get(0).size()-1;
-            } else if (slots.get(0).get(endPos).compareLowerToUpperBound(upperExclusiveKey) >= 0) {
+            } else if (slots.get(0).get(endPos).compareLowerToUpperBound(upperExclusiveKey, ScanUtil.getComparator(schema.getField(0))) >= 0) {
                 // We know that the endPos range is higher than the previous range, but we need
                 // to test if it ends before the next range starts.
                 endPos--;
@@ -397,8 +398,10 @@ public class SkipScanFilter extends FilterBase implements Writable {
         int maxOffset = schema.iterator(currentKey, minOffset, length, ptr);
         schema.next(ptr, ScanUtil.getRowKeyPosition(slotSpan, i), maxOffset, slotSpan[i]);
         while (true) {
+            // Comparator depends on field in schema
+            BytesComparator comparator = ScanUtil.getComparator(schema.getField(ScanUtil.getRowKeyPosition(slotSpan, i)));
             // Increment to the next range while the upper bound of our current slot is less than our current key
-            while (position[i] < slots.get(i).size() && slots.get(i).get(position[i]).compareUpperToLowerBound(ptr) < 0) {
+            while (position[i] < slots.get(i).size() && slots.get(i).get(position[i]).compareUpperToLowerBound(ptr, comparator) < 0) {
                 position[i]++;
             }
             Arrays.fill(position, i+1, position.length, 0);
@@ -448,7 +451,7 @@ public class SkipScanFilter extends FilterBase implements Writable {
                     ByteUtil.nextKey(startKey, currentLength);
                 }
                 i = j;
-            } else if (slots.get(i).get(position[i]).compareLowerToUpperBound(ptr) > 0) {
+            } else if (slots.get(i).get(position[i]).compareLowerToUpperBound(ptr, comparator) > 0) {
                 // Our current key is less than the lower range of the current position in the current slot.
                 // Seek to the lower range, since it's bigger than the current key
                 setStartKey(ptr, minOffset, i, nSlots);

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java b/phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java
index 0612046..366a123 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java
@@ -32,6 +32,7 @@ import org.apache.hadoop.io.Writable;
 import org.apache.hadoop.io.WritableUtils;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.util.ByteUtil;
+import org.apache.phoenix.util.ScanUtil.BytesComparator;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ComparisonChain;
@@ -183,28 +184,28 @@ public class KeyRange implements Writable {
         return isSingleKey;
     }
     
-    public int compareLowerToUpperBound(ImmutableBytesWritable ptr, boolean isInclusive) {
-        return compareLowerToUpperBound(ptr.get(), ptr.getOffset(), ptr.getLength(), isInclusive);
+    public int compareLowerToUpperBound(ImmutableBytesWritable ptr, boolean isInclusive, BytesComparator comparator) {
+        return compareLowerToUpperBound(ptr.get(), ptr.getOffset(), ptr.getLength(), isInclusive, comparator);
     }
     
-    public int compareLowerToUpperBound(ImmutableBytesWritable ptr) {
-        return compareLowerToUpperBound(ptr, true);
+    public int compareLowerToUpperBound(ImmutableBytesWritable ptr, BytesComparator comparator) {
+        return compareLowerToUpperBound(ptr, true, comparator);
     }
     
-    public int compareUpperToLowerBound(ImmutableBytesWritable ptr, boolean isInclusive) {
-        return compareUpperToLowerBound(ptr.get(), ptr.getOffset(), ptr.getLength(), isInclusive);
+    public int compareUpperToLowerBound(ImmutableBytesWritable ptr, boolean isInclusive, BytesComparator comparator) {
+        return compareUpperToLowerBound(ptr.get(), ptr.getOffset(), ptr.getLength(), isInclusive, comparator);
     }
     
-    public int compareUpperToLowerBound(ImmutableBytesWritable ptr) {
-        return compareUpperToLowerBound(ptr, true);
+    public int compareUpperToLowerBound(ImmutableBytesWritable ptr, BytesComparator comparator) {
+        return compareUpperToLowerBound(ptr, true, comparator);
     }
     
-    public int compareLowerToUpperBound( byte[] b, int o, int l) {
-        return compareLowerToUpperBound(b,o,l,true);
+    public int compareLowerToUpperBound( byte[] b, int o, int l, BytesComparator comparator) {
+        return compareLowerToUpperBound(b,o,l,true, comparator);
     }
 
-    public int compareLowerToUpperBound( byte[] b) {
-        return compareLowerToUpperBound(b,0,b.length);
+    public int compareLowerToUpperBound( byte[] b, BytesComparator comparator) {
+        return compareLowerToUpperBound(b,0,b.length, comparator);
     }
 
     /**
@@ -213,15 +214,16 @@ public class KeyRange implements Writable {
      * @param o upper bound offset
      * @param l upper bound length
      * @param isInclusive upper bound inclusive
+     * @param comparator comparator used to do compare the byte array using offset and length
      * @return -1 if the lower bound is less than the upper bound,
      *          1 if the lower bound is greater than the upper bound,
      *          and 0 if they are equal.
      */
-    public int compareLowerToUpperBound( byte[] b, int o, int l, boolean isInclusive) {
+    public int compareLowerToUpperBound( byte[] b, int o, int l, boolean isInclusive, BytesComparator comparator) {
         if (lowerUnbound() || b == KeyRange.UNBOUND) {
             return -1;
         }
-        int cmp = Bytes.compareTo(lowerRange, 0, lowerRange.length, b, o, l);
+        int cmp = comparator.compare(lowerRange, 0, lowerRange.length, b, o, l);
         if (cmp > 0) {
             return 1;
         }
@@ -234,19 +236,19 @@ public class KeyRange implements Writable {
         return 1;
     }
     
-    public int compareUpperToLowerBound(byte[] b) {
-        return compareUpperToLowerBound(b,0,b.length);
+    public int compareUpperToLowerBound(byte[] b, BytesComparator comparator) {
+        return compareUpperToLowerBound(b,0,b.length, comparator);
     }
     
-    public int compareUpperToLowerBound(byte[] b, int o, int l) {
-        return compareUpperToLowerBound(b,o,l, true);
+    public int compareUpperToLowerBound(byte[] b, int o, int l, BytesComparator comparator) {
+        return compareUpperToLowerBound(b,o,l, true, comparator);
     }
     
-    public int compareUpperToLowerBound(byte[] b, int o, int l, boolean isInclusive) {
+    public int compareUpperToLowerBound(byte[] b, int o, int l, boolean isInclusive, BytesComparator comparator) {
         if (upperUnbound() || b == KeyRange.UNBOUND) {
             return 1;
         }
-        int cmp = Bytes.compareTo(upperRange, 0, upperRange.length, b, o, l);
+        int cmp = comparator.compare(upperRange, 0, upperRange.length, b, o, l);
         if (cmp > 0) {
             return 1;
         }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/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 43bab0e..d79de60 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
@@ -36,6 +36,7 @@ import org.apache.phoenix.schema.ConstraintViolationException;
 import org.apache.phoenix.schema.IllegalDataException;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.util.ByteUtil;
+import org.apache.phoenix.util.ScanUtil;
 
 import com.google.common.base.Preconditions;
 import com.google.common.math.LongMath;
@@ -762,7 +763,7 @@ public abstract class PDataType<T> implements DataType<T>, Comparable<PDataType<
             }
             return (length1 - length2);
         }
-        return Bytes.compareTo(ba1, offset1, length1, ba2, offset2, length2) * (so1 == SortOrder.DESC ? -1 : 1);
+        return (so1 == SortOrder.DESC ? -1 : 1) * ScanUtil.getComparator(length1 == length2, so1).compare(ba1, offset1, length1, ba2, offset2, length2);
     }
 
     public final int compareTo(ImmutableBytesWritable ptr1, SortOrder ptr1SortOrder, ImmutableBytesWritable ptr2,

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
index 3b7ed4a..7b76a2b 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
@@ -39,6 +39,7 @@ import org.apache.hadoop.hbase.filter.FilterList;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.hbase.io.TimeRange;
 import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.io.WritableComparator;
 import org.apache.phoenix.compile.OrderByCompiler.OrderBy;
 import org.apache.phoenix.compile.ScanRanges;
 import org.apache.phoenix.compile.StatementContext;
@@ -57,6 +58,7 @@ import org.apache.phoenix.query.QueryServicesOptions;
 import org.apache.phoenix.schema.IllegalDataException;
 import org.apache.phoenix.schema.PName;
 import org.apache.phoenix.schema.RowKeySchema;
+import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.ValueSchema.Field;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PVarbinary;
@@ -285,7 +287,15 @@ public class ScanUtil {
         for (int i = 0; i < position.length; i++) {
             position[i] = bound == Bound.LOWER ? 0 : slots.get(i).size()-1;
             KeyRange range = slots.get(i).get(position[i]);
-            maxLength += range.getRange(bound).length + (schema.getField(i + slotSpan[i]).getDataType().isFixedWidth() ? 0 : 1);
+            Field field = schema.getField(i + slotSpan[i]);
+            int keyLength = range.getRange(bound).length;
+            if (!field.getDataType().isFixedWidth()) {
+                keyLength++;
+                if (range.isUnbound(bound) && !range.isInclusive(bound) && field.getSortOrder() == SortOrder.DESC) {
+                    keyLength++;
+                }
+            }
+            maxLength += keyLength;
         }
         byte[] key = new byte[maxLength];
         int length = setKey(schema, slots, slotSpan, position, bound, key, 0, 0, position.length);
@@ -379,8 +389,8 @@ public class ScanUtil {
             // key slots would cause the flag to become true.
             lastInclusiveUpperSingleKey = range.isSingleKey() && inclusiveUpper;
             anyInclusiveUpperRangeKey |= !range.isSingleKey() && inclusiveUpper;
-            // A match for IS NULL or IS NOT NULL should not have a DESC_SEPARATOR_BYTE as nulls sort first
-            byte sepByte = SchemaUtil.getSeparatorByte(schema.rowKeyOrderOptimizable(), bytes.length == 0 || range == KeyRange.IS_NULL_RANGE || range == KeyRange.IS_NOT_NULL_RANGE, field);
+            // A null or empty byte array is always represented as a zero byte
+            byte sepByte = SchemaUtil.getSeparatorByte(schema.rowKeyOrderOptimizable(), bytes.length == 0, field);
             
             if ( !isFixedWidth && ( sepByte == QueryConstants.DESC_SEPARATOR_BYTE 
                                     || ( !exclusiveUpper 
@@ -399,7 +409,7 @@ public class ScanUtil {
             // If we are setting the lower bound with an exclusive range key, we need to bump the
             // slot up for each key part. For an upper bound, we bump up an inclusive key, but
             // only after the last key part.
-            if (!range.isSingleKey() && exclusiveLower) {
+            if (exclusiveLower) {
                 if (!ByteUtil.nextKey(key, offset)) {
                     // Special case for not being able to increment.
                     // In this case we return a negative byteOffset to
@@ -408,6 +418,14 @@ public class ScanUtil {
                     // have an end key specified.
                     return -byteOffset;
                 }
+                // We're filtering on values being non null here, but we still need the 0xFF
+                // terminator, since DESC keys ignore the last byte as it's expected to be 
+                // the terminator. Without this, we'd ignore the separator byte that was
+                // just added and incremented.
+                if (!isFixedWidth && bytes.length == 0 
+                    && SchemaUtil.getSeparatorByte(schema.rowKeyOrderOptimizable(), false, field) == QueryConstants.DESC_SEPARATOR_BYTE) {
+                    key[offset++] = QueryConstants.DESC_SEPARATOR_BYTE;
+                }
             }
             
             fieldIndex += slotSpan[i] + 1;
@@ -427,7 +445,8 @@ public class ScanUtil {
         // byte.
         if (bound == Bound.LOWER) {
             while (--i >= schemaStartIndex && offset > byteOffset && 
-                    !schema.getField(--fieldIndex).getDataType().isFixedWidth() && 
+                    !(field=schema.getField(--fieldIndex)).getDataType().isFixedWidth() && 
+                    field.getSortOrder() == SortOrder.ASC &&
                     key[offset-1] == QueryConstants.SEPARATOR_BYTE) {
                 offset--;
                 fieldIndex -= slotSpan[i];
@@ -435,19 +454,47 @@ public class ScanUtil {
         }
         return offset - byteOffset;
     }
+    
+    public static interface BytesComparator {
+        public int compare(byte[] b1, int s1, int l1, byte[] b2, int s2, int l2);
+    };
 
+    private static final BytesComparator DESC_VAR_WIDTH_COMPARATOR = new BytesComparator() {
+
+        @Override
+        public int compare(byte[] b1, int s1, int l1, byte[] b2, int s2, int l2) {
+            return DescVarLengthFastByteComparisons.compareTo(b1, s1, l1, b2, s2, l2);
+        }
+        
+    };
+    
+    private static final BytesComparator ASC_FIXED_WIDTH_COMPARATOR = new BytesComparator() {
+
+        @Override
+        public int compare(byte[] b1, int s1, int l1, byte[] b2, int s2, int l2) {
+            return WritableComparator.compareBytes(b1, s1, l1, b2, s2, l2);
+        }
+        
+    };
+    public static BytesComparator getComparator(boolean isFixedWidth, SortOrder sortOrder) {
+        return isFixedWidth || sortOrder == SortOrder.ASC ? ASC_FIXED_WIDTH_COMPARATOR : DESC_VAR_WIDTH_COMPARATOR;
+    }
+    public static BytesComparator getComparator(Field field) {
+        return getComparator(field.getDataType().isFixedWidth(),field.getSortOrder());
+    }
     /**
      * Perform a binary lookup on the list of KeyRange for the tightest slot such that the slotBound
      * of the current slot is higher or equal than the slotBound of our range. 
      * @return  the index of the slot whose slot bound equals or are the tightest one that is 
      *          smaller than rangeBound of range, or slots.length if no bound can be found.
      */
-    public static int searchClosestKeyRangeWithUpperHigherThanPtr(List<KeyRange> slots, ImmutableBytesWritable ptr, int lower) {
+    public static int searchClosestKeyRangeWithUpperHigherThanPtr(List<KeyRange> slots, ImmutableBytesWritable ptr, int lower, Field field) {
         int upper = slots.size() - 1;
         int mid;
+        BytesComparator comparator = ScanUtil.getComparator(field.getDataType().isFixedWidth(), field.getSortOrder());
         while (lower <= upper) {
             mid = (lower + upper) / 2;
-            int cmp = slots.get(mid).compareUpperToLowerBound(ptr, true);
+            int cmp = slots.get(mid).compareUpperToLowerBound(ptr, true, comparator);
             if (cmp < 0) {
                 lower = mid + 1;
             } else if (cmp > 0) {
@@ -457,7 +504,7 @@ public class ScanUtil {
             }
         }
         mid = (lower + upper) / 2;
-        if (mid == 0 && slots.get(mid).compareUpperToLowerBound(ptr, true) > 0) {
+        if (mid == 0 && slots.get(mid).compareUpperToLowerBound(ptr, true, comparator) > 0) {
             return mid;
         } else {
             return ++mid;

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
index d01bf39..5414d4f 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
@@ -625,7 +625,7 @@ public class SchemaUtil {
     }
 
     public static int getMaxKeyLength(RowKeySchema schema, List<List<KeyRange>> slots) {
-        int maxKeyLength = getTerminatorCount(schema);
+        int maxKeyLength = getTerminatorCount(schema) * 2;
         for (List<KeyRange> slot : slots) {
             int maxSlotLength = 0;
             for (KeyRange range : slot) {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java b/phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java
index 93e7823..9344323 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/compile/WhereOptimizerTest.java
@@ -32,6 +32,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.math.BigDecimal;
 import java.sql.Connection;
 import java.sql.Date;
 import java.sql.DriverManager;
@@ -56,9 +57,12 @@ import org.apache.phoenix.query.BaseConnectionlessQueryTest;
 import org.apache.phoenix.query.KeyRange;
 import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.schema.ColumnNotFoundException;
+import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.types.PChar;
 import org.apache.phoenix.schema.types.PDate;
+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.PUnsignedLong;
 import org.apache.phoenix.schema.types.PVarchar;
 import org.apache.phoenix.util.ByteUtil;
@@ -144,6 +148,22 @@ public class WhereOptimizerTest extends BaseConnectionlessQueryTest {
         assertArrayEquals(tmpBytes2, scan.getStartRow());
         assertArrayEquals(tmpBytes3, scan.getStopRow());
     }
+    
+    @Test
+    public void testDescDecimalRange() throws SQLException {
+        String ddl = "create table t (k1 bigint not null, k2 decimal, constraint pk primary key (k1,k2 desc))";
+        Connection conn = DriverManager.getConnection(getUrl(), PropertiesUtil.deepCopy(TEST_PROPERTIES));
+        conn.createStatement().execute(ddl);
+        String query = "select * from t where k1 in (1,2) and k2>1.0";
+        Scan scan = compileStatement(query).getScan();
+
+        byte[] startRow = ByteUtil.concat(PLong.INSTANCE.toBytes(1), ByteUtil.nextKey(QueryConstants.SEPARATOR_BYTE_ARRAY), QueryConstants.DESC_SEPARATOR_BYTE_ARRAY);
+        byte[] upperValue = PDecimal.INSTANCE.toBytes(BigDecimal.valueOf(1.0));
+        byte[] stopRow = ByteUtil.concat(PLong.INSTANCE.toBytes(2), SortOrder.invert(upperValue,0,upperValue.length), QueryConstants.DESC_SEPARATOR_BYTE_ARRAY);
+        assertTrue(scan.getFilter() instanceof SkipScanFilter);
+        assertArrayEquals(startRow, scan.getStartRow());
+        assertArrayEquals(stopRow, scan.getStopRow());
+    }
 
     @Test
     public void testSingleCharPaddedKeyExpression() throws SQLException {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/test/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisonsTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisonsTest.java b/phoenix-core/src/test/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisonsTest.java
new file mode 100644
index 0000000..106471b
--- /dev/null
+++ b/phoenix-core/src/test/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisonsTest.java
@@ -0,0 +1,45 @@
+/*
+ * 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.execute;
+
+import static org.junit.Assert.assertTrue;
+
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.util.ByteUtil;
+import org.junit.Test;
+
+public class DescVarLengthFastByteComparisonsTest {
+    
+    @Test
+    public void testNullIsSmallest() {
+        byte[] b1 = ByteUtil.EMPTY_BYTE_ARRAY;
+        byte[] b2 = Bytes.toBytes("a");
+        int cmp = DescVarLengthFastByteComparisons.compareTo(b1, 0, b1.length, b2, 0, b2.length);
+        assertTrue(cmp < 0);
+        cmp = DescVarLengthFastByteComparisons.compareTo(b2, 0, b2.length, b1, 0, b1.length);
+        assertTrue(cmp > 0);
+    }
+    
+    @Test
+    public void testShorterSubstringIsBigger() {
+        byte[] b1 = Bytes.toBytes("ab");
+        byte[] b2 = Bytes.toBytes("a");
+        int cmp = DescVarLengthFastByteComparisons.compareTo(b1, 0, b1.length, b2, 0, b2.length);
+        assertTrue(cmp < 0);
+    }
+}

http://git-wip-us.apache.org/repos/asf/phoenix/blob/ac2b2083/phoenix-core/src/test/java/org/apache/phoenix/expression/SortOrderExpressionTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/expression/SortOrderExpressionTest.java b/phoenix-core/src/test/java/org/apache/phoenix/expression/SortOrderExpressionTest.java
index b9ee0eb..e2ab684 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/expression/SortOrderExpressionTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/expression/SortOrderExpressionTest.java
@@ -292,17 +292,20 @@ public class SortOrderExpressionTest {
     }
     
     private void runCompareTest(CompareOp op, boolean expectedResult, Object lhsValue, PDataType lhsDataType, Object rhsValue, PDataType rhsDataType) throws Exception {
-        List<Expression> args = Lists.newArrayList(getLiteral(lhsValue, lhsDataType), getLiteral(rhsValue, rhsDataType));
-        evaluateAndAssertResult(new ComparisonExpression(args, op), expectedResult, "lhsDataType: " + lhsDataType + " rhsDataType: " + rhsDataType);
+        List<Expression> args;
+        ImmutableBytesWritable ptr = new ImmutableBytesWritable();
+
+        args = Lists.newArrayList(getLiteral(lhsValue, lhsDataType), getLiteral(rhsValue, rhsDataType));
+        evaluateAndAssertResult(ComparisonExpression.create(op, args, ptr, true), expectedResult, "lhsDataType: " + lhsDataType + " rhsDataType: " + rhsDataType);
         
         args = Lists.newArrayList(getInvertedLiteral(lhsValue, lhsDataType), getLiteral(rhsValue, rhsDataType));
-        evaluateAndAssertResult(new ComparisonExpression(args, op), expectedResult, "lhs (inverted) dataType: " + lhsDataType + " rhsDataType: " + rhsDataType);
+        evaluateAndAssertResult(ComparisonExpression.create(op, args, ptr, true), expectedResult, "lhs (inverted) dataType: " + lhsDataType + " rhsDataType: " + rhsDataType);
         
         args = Lists.newArrayList(getLiteral(lhsValue, lhsDataType), getInvertedLiteral(rhsValue, rhsDataType));
-        evaluateAndAssertResult(new ComparisonExpression(args, op), expectedResult, "lhsDataType: " + lhsDataType + " rhs (inverted) dataType: " + rhsDataType);
+        evaluateAndAssertResult(ComparisonExpression.create(op, args, ptr, true), expectedResult, "lhsDataType: " + lhsDataType + " rhs (inverted) dataType: " + rhsDataType);
         
         args = Lists.newArrayList(getInvertedLiteral(lhsValue, lhsDataType), getInvertedLiteral(rhsValue, rhsDataType));
-        evaluateAndAssertResult(new ComparisonExpression(args, op), expectedResult, "lhs (inverted) dataType: " + lhsDataType + " rhs (inverted) dataType: " + rhsDataType);                
+        evaluateAndAssertResult(ComparisonExpression.create(op, args, ptr, true), expectedResult, "lhs (inverted) dataType: " + lhsDataType + " rhs (inverted) dataType: " + rhsDataType);                
     }
     
     private void evaluateAndAssertResult(Expression expression, Object expectedResult) {


[2/2] phoenix git commit: PHOENIX-2171 DOUBLE and FLOAT DESC are stored as ASC

Posted by ja...@apache.org.
PHOENIX-2171 DOUBLE and FLOAT DESC are stored as ASC


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

Branch: refs/heads/4.x-HBase-0.98
Commit: 18ec7d6a460b079f1e22fbb17a1e83ac5ac0164b
Parents: ac2b208
Author: James Taylor <jt...@salesforce.com>
Authored: Tue Aug 11 01:59:23 2015 -0700
Committer: James Taylor <jt...@salesforce.com>
Committed: Thu Sep 10 21:24:03 2015 -0700

----------------------------------------------------------------------
 .../org/apache/phoenix/end2end/SortOrderIT.java |  6 ++--
 .../UngroupedAggregateRegionObserver.java       |  6 ++++
 .../org/apache/phoenix/schema/PTableImpl.java   | 10 +++++-
 .../apache/phoenix/schema/types/PDataType.java  |  4 +--
 .../apache/phoenix/schema/types/PDouble.java    | 18 +++++++----
 .../org/apache/phoenix/schema/types/PFloat.java | 18 ++++++-----
 .../org/apache/phoenix/util/UpgradeUtil.java    |  5 +++
 .../phoenix/schema/types/PDataTypeTest.java     | 33 ++++++++++++++++++++
 8 files changed, 82 insertions(+), 18 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
index 25782ba..b906ae2 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
@@ -41,6 +41,8 @@ import org.apache.commons.lang.ArrayUtils;
 import org.apache.phoenix.schema.SortOrder;
 import org.apache.phoenix.schema.types.PDataType;
 import org.apache.phoenix.schema.types.PDecimal;
+import org.apache.phoenix.schema.types.PDouble;
+import org.apache.phoenix.schema.types.PFloat;
 import org.apache.phoenix.util.PropertiesUtil;
 import org.junit.Assert;
 import org.junit.Test;
@@ -431,7 +433,7 @@ public class SortOrderIT extends BaseHBaseManagedTimeIT {
     public void testNonPKCompare() throws Exception {
         List<Integer> expectedResults = Lists.newArrayList(2,3,4);
         Integer[] saltBuckets = new Integer[] {null,3};
-        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE};
+        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE, PDouble.INSTANCE, PFloat.INSTANCE};
         for (Integer saltBucket : saltBuckets) {
             for (PDataType dataType : dataTypes) {
                 for (SortOrder sortOrder : SortOrder.values()) {
@@ -447,7 +449,7 @@ public class SortOrderIT extends BaseHBaseManagedTimeIT {
         List<Integer> rExpectedResults = new ArrayList<>(expectedResults);
         Collections.reverse(rExpectedResults);
         Integer[] saltBuckets = new Integer[] {null,3};
-        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE};
+        PDataType[] dataTypes = new PDataType[] {PDecimal.INSTANCE, PDouble.INSTANCE, PFloat.INSTANCE};
         for (Integer saltBucket : saltBuckets) {
             for (PDataType dataType : dataTypes) {
                 for (SortOrder sortOrder : SortOrder.values()) {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/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 571d0d1..7316bb1 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
@@ -88,6 +88,8 @@ import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
 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.PDouble;
+import org.apache.phoenix.schema.types.PFloat;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.IndexUtil;
 import org.apache.phoenix.util.KeyValueUtil;
@@ -312,6 +314,10 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver{
                                                 len--;
                                             }
                                             ptr.set(ptr.get(), ptr.getOffset(), len);
+                                        // Special case for re-writing DESC FLOAT and DOUBLE, as they're not inverted like they should be (PHOENIX-2171)
+                                        } else if (field.getDataType() == PFloat.INSTANCE || field.getDataType() == PDouble.INSTANCE) {
+                                            byte[] invertedBytes = SortOrder.invert(ptr.get(), ptr.getOffset(), ptr.getLength());
+                                            ptr.set(invertedBytes);
                                         }
                                     } else if (field.getDataType() == PBinary.INSTANCE) {
                                         // Remove trailing space characters so that the setValues call below will replace them

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/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 610bd83..407ee90 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
@@ -57,6 +57,8 @@ 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.PDouble;
+import org.apache.phoenix.schema.types.PFloat;
 import org.apache.phoenix.schema.types.PVarchar;
 import org.apache.phoenix.util.ByteUtil;
 import org.apache.phoenix.util.SchemaUtil;
@@ -407,7 +409,13 @@ public class PTableImpl implements PTable {
         for (PColumn column : allColumns) {
             PName familyName = column.getFamilyName();
             if (familyName == null) {
-                hasColumnsRequiringUpgrade |= (column.getSortOrder() == SortOrder.DESC && (!column.getDataType().isFixedWidth() || column.getDataType() == PChar.INSTANCE || column.getDataType() == PBinary.INSTANCE))
+                hasColumnsRequiringUpgrade |= 
+                        ( column.getSortOrder() == SortOrder.DESC 
+                            && (!column.getDataType().isFixedWidth() 
+                                || column.getDataType() == PChar.INSTANCE 
+                                || column.getDataType() == PFloat.INSTANCE 
+                                || column.getDataType() == PDouble.INSTANCE 
+                                || column.getDataType() == PBinary.INSTANCE) )
                         || (column.getSortOrder() == SortOrder.ASC && column.getDataType() == PBinary.INSTANCE && column.getMaxLength() != null && column.getMaxLength() > 1);
             	pkColumns.add(column);
             }

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/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 d79de60..5d8852c 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
@@ -66,16 +66,15 @@ public abstract class PDataType<T> implements DataType<T>, Comparable<PDataType<
         this.ordinal = ordinal;
     }
 
-    @Deprecated
     public static PDataType[] values() {
         return PDataTypeFactory.getInstance().getOrderedTypes();
     }
 
-    @Deprecated
     public int ordinal() {
         return ordinal;
     }
 
+    @SuppressWarnings("unchecked")
     @Override
     public Class<T> encodedClass() {
         return getJavaClass();
@@ -942,6 +941,7 @@ public abstract class PDataType<T> implements DataType<T>, Comparable<PDataType<
     public abstract Object toObject(byte[] bytes, int offset, int length, PDataType actualType, SortOrder sortOrder,
             Integer maxLength, Integer scale);
 
+    @SuppressWarnings("unchecked")
     @Override
     public T decode(PositionedByteRange pbr) {
         // default implementation based on existing PDataType methods.

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDouble.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDouble.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDouble.java
index d11aedf..95a526e 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDouble.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDouble.java
@@ -236,15 +236,21 @@ public class PDouble extends PRealNumber<Double> {
     }
 
     @Override
-    public double decodeDouble(byte[] b, int o, SortOrder sortOrder) {
+    public double decodeDouble(byte[] bytes, int o, SortOrder sortOrder) {
       Preconditions.checkNotNull(sortOrder);
-      checkForSufficientLength(b, o, Bytes.SIZEOF_LONG);
+      checkForSufficientLength(bytes, o, Bytes.SIZEOF_LONG);
+      long l;
       if (sortOrder == SortOrder.DESC) {
-        for (int i = o; i < Bytes.SIZEOF_LONG; i++) {
-          b[i] = (byte) (b[i] ^ 0xff);
-        }
+          // Copied from Bytes.toLong(), but without using the toLongUnsafe
+          // TODO: would it be possible to use the toLongUnsafe?
+          l = 0;
+          for(int i = o; i < o + Bytes.SIZEOF_LONG; i++) {
+            l <<= 8;
+            l ^= (bytes[i] ^ 0xff) & 0xFF;
+          }
+      } else {
+          l = Bytes.toLong(bytes, o);
       }
-      long l = Bytes.toLong(b, o);
       l--;
       l ^= (~l >> Long.SIZE - 1) | Long.MIN_VALUE;
       return Double.longBitsToDouble(l);

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloat.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloat.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloat.java
index 67b7d9a..75f8efa 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloat.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloat.java
@@ -243,15 +243,19 @@ public class PFloat extends PRealNumber<Float> {
     public float decodeFloat(byte[] b, int o, SortOrder sortOrder) {
       Preconditions.checkNotNull(sortOrder);
       checkForSufficientLength(b, o, Bytes.SIZEOF_INT);
+      int value;
       if (sortOrder == SortOrder.DESC) {
-        for (int i = o; i < Bytes.SIZEOF_INT; i++) {
-          b[i] = (byte) (b[i] ^ 0xff);
-        }
+          value = 0;
+          for(int i = o; i < (o + Bytes.SIZEOF_INT); i++) {
+            value <<= 8;
+            value ^= (b[i] ^ 0xff) & 0xFF;
+          }
+      } else {
+          value = Bytes.toInt(b, o);
       }
-      int i = Bytes.toInt(b, o);
-      i--;
-      i ^= (~i >> Integer.SIZE - 1) | Integer.MIN_VALUE;
-      return Float.intBitsToFloat(i);
+      value--;
+      value ^= (~value >> Integer.SIZE - 1) | Integer.MIN_VALUE;
+      return Float.intBitsToFloat(value);
     }
 
     @Override

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/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 b303b09..f2b4516 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
@@ -84,6 +84,8 @@ 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.PDouble;
+import org.apache.phoenix.schema.types.PFloat;
 import org.apache.phoenix.schema.types.PInteger;
 import org.apache.phoenix.schema.types.PLong;
 import org.apache.phoenix.schema.types.PVarbinary;
@@ -884,12 +886,15 @@ public class UpgradeUtil {
     // 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)
+    // 3) float and double (PHOENIX-2171)
     // We exclude VARBINARY as we no longer support DESC for it.
     private static String getAffectedDataTypes() {
         StringBuilder buf = new StringBuilder("(" 
                 + PVarchar.INSTANCE.getSqlType() + "," +
                 + PChar.INSTANCE.getSqlType() + "," +
                 + PBinary.INSTANCE.getSqlType() + "," +
+                + PFloat.INSTANCE.getSqlType() + "," +
+                + PDouble.INSTANCE.getSqlType() + "," +
                 + PDecimal.INSTANCE.getSqlType() + ","
                 );
         for (PDataType type : PDataType.values()) {

http://git-wip-us.apache.org/repos/asf/phoenix/blob/18ec7d6a/phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
----------------------------------------------------------------------
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java b/phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
index 7ab9093..5657c22 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
@@ -41,6 +41,7 @@ import org.apache.phoenix.exception.SQLExceptionCode;
 import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.schema.ConstraintViolationException;
 import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.util.ScanUtil;
 import org.apache.phoenix.util.TestUtil;
 import org.junit.Test;
 
@@ -743,6 +744,38 @@ public class PDataTypeTest {
     }
     
     @Test
+    public void testDoubleComparison() {
+        testRealNumberComparison(PDouble.INSTANCE, new Double[] {0.99, 1.0, 1.001, 1.01, 2.0});
+    }
+    
+    @Test
+    public void testFloatComparison() {
+        testRealNumberComparison(PFloat.INSTANCE, new Float[] {0.99f, 1.0f, 1.001f, 1.01f, 2.0f});
+    }
+    
+    @Test
+    public void testDecimalComparison() {
+        testRealNumberComparison(PDecimal.INSTANCE, new BigDecimal[] {BigDecimal.valueOf(0.99), BigDecimal.valueOf(1.0), BigDecimal.valueOf(1.001), BigDecimal.valueOf(1.01), BigDecimal.valueOf(2.0)});
+    }
+    
+    private static void testRealNumberComparison(PDataType type, Object[] a) {
+        
+        for (SortOrder sortOrder : SortOrder.values()) {
+            int factor = (sortOrder == SortOrder.ASC ? 1 : -1);
+            byte[] prev_b = null;
+            Object prev_o = null;
+            for (Object o : a) {
+                byte[] b = type.toBytes(o, sortOrder);
+                if (prev_b != null) {
+                    assertTrue("Compare of " + o + " with " + prev_o + " " + sortOrder + " failed.", ScanUtil.getComparator(type.isFixedWidth(), sortOrder).compare(prev_b, 0, prev_b.length, b, 0, b.length) * factor < 0);
+                }
+                prev_b = b;
+                prev_o = o;
+            }
+        }
+    }
+    
+    @Test
     public void testDouble() {
         Double na = 0.005;
         byte[] b = PDouble.INSTANCE.toBytes(na);