Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/12490#discussion_r60526773
--- Diff:
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java
---
@@ -28,88 +28,84 @@
public class PrefixComparators {
private PrefixComparators() {}
- public static final StringPrefixComparator STRING = new
StringPrefixComparator();
- public static final StringPrefixComparatorDesc STRING_DESC = new
StringPrefixComparatorDesc();
- public static final BinaryPrefixComparator BINARY = new
BinaryPrefixComparator();
- public static final BinaryPrefixComparatorDesc BINARY_DESC = new
BinaryPrefixComparatorDesc();
- public static final LongPrefixComparator LONG = new
LongPrefixComparator();
- public static final LongPrefixComparatorDesc LONG_DESC = new
LongPrefixComparatorDesc();
- public static final DoublePrefixComparator DOUBLE = new
DoublePrefixComparator();
- public static final DoublePrefixComparatorDesc DOUBLE_DESC = new
DoublePrefixComparatorDesc();
-
- public static final class StringPrefixComparator extends
PrefixComparator {
- @Override
- public int compare(long aPrefix, long bPrefix) {
- return UnsignedLongs.compare(aPrefix, bPrefix);
- }
-
+ public static final PrefixComparator STRING = new
UnsignedPrefixComparator();
+ public static final PrefixComparator STRING_DESC = new
UnsignedPrefixComparatorDesc();
+ public static final PrefixComparator BINARY = new
UnsignedPrefixComparator();
+ public static final PrefixComparator BINARY_DESC = new
UnsignedPrefixComparatorDesc();
+ public static final PrefixComparator LONG = new SignedPrefixComparator();
+ public static final PrefixComparator LONG_DESC = new
SignedPrefixComparatorDesc();
+ public static final PrefixComparator DOUBLE = new
SignedPrefixComparator();
+ public static final PrefixComparator DOUBLE_DESC = new
SignedPrefixComparatorDesc();
+
+ public static final class StringPrefixComparator {
public static long computePrefix(UTF8String value) {
return value == null ? 0L : value.getPrefix();
}
}
- public static final class StringPrefixComparatorDesc extends
PrefixComparator {
- @Override
- public int compare(long bPrefix, long aPrefix) {
- return UnsignedLongs.compare(aPrefix, bPrefix);
+ public static final class BinaryPrefixComparator {
+ public static long computePrefix(byte[] bytes) {
+ return ByteArray.getPrefix(bytes);
}
}
- public static final class BinaryPrefixComparator extends
PrefixComparator {
- @Override
- public int compare(long aPrefix, long bPrefix) {
- return UnsignedLongs.compare(aPrefix, bPrefix);
+ public static final class DoublePrefixComparator {
+ public static long computePrefix(double value) {
+ // Java's doubleToLongBits already canonicalizes all NaN values to
the lowest possible NaN,
+ // so there's nothing special we need to do here.
+ return Double.doubleToLongBits(value);
}
+ }
- public static long computePrefix(byte[] bytes) {
- return ByteArray.getPrefix(bytes);
- }
+ /**
+ * Provides radix sort parameters. Comparators implementing this also
are indicating that the
+ * ordering they define is compatible with radix sort.
+ */
+ public static abstract class RadixSortSupport extends PrefixComparator {
+ /** @return Whether the sort should be descending in binary sort
order. */
+ public abstract boolean sortDescending();
+
+ /** @return Whether the sort should take into account the sign bit. */
+ public abstract boolean sortSigned();
}
- public static final class BinaryPrefixComparatorDesc extends
PrefixComparator {
+ //
+ // Standard prefix comparator implementations
+ //
+
+ public static final class UnsignedPrefixComparator extends
RadixSortSupport {
+ @Override public final boolean sortDescending() { return false; }
+ @Override public final boolean sortSigned() { return false; }
@Override
- public int compare(long bPrefix, long aPrefix) {
+ public final int compare(long aPrefix, long bPrefix) {
return UnsignedLongs.compare(aPrefix, bPrefix);
}
}
- public static final class LongPrefixComparator extends PrefixComparator {
+ public static final class UnsignedPrefixComparatorDesc extends
RadixSortSupport {
+ @Override public final boolean sortDescending() { return true; }
+ @Override public final boolean sortSigned() { return false; }
@Override
- public int compare(long a, long b) {
- return (a < b) ? -1 : (a > b) ? 1 : 0;
+ public final int compare(long bPrefix, long aPrefix) {
+ return UnsignedLongs.compare(aPrefix, bPrefix);
--- End diff --
I think so, we just remove that from public API
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
