You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2018/05/31 19:17:02 UTC
[GitHub] keith-turner closed pull request #501: Improve some comparators
using Java 8
keith-turner closed pull request #501: Improve some comparators using Java 8
URL: https://github.com/apache/accumulo/pull/501
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java b/core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
index e4c60c69b0..fb941ebeeb 100644
--- a/core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
+++ b/core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
@@ -778,38 +778,13 @@ private void convertMutations(TabletServerMutations<QCMutation> mutations, Map<L
}
}
- static class ConditionComparator implements Comparator<Condition> {
+ private static final Comparator<Long> TIMESTAMP_COMPARATOR = Comparator
+ .nullsFirst(Comparator.reverseOrder());
- private static final Long MAX = Long.MAX_VALUE;
-
- @Override
- public int compare(Condition c1, Condition c2) {
- int comp = c1.getFamily().compareTo(c2.getFamily());
- if (comp == 0) {
- comp = c1.getQualifier().compareTo(c2.getQualifier());
- if (comp == 0) {
- comp = c1.getVisibility().compareTo(c2.getVisibility());
- if (comp == 0) {
- Long l1 = c1.getTimestamp();
- Long l2 = c2.getTimestamp();
- if (l1 == null) {
- l1 = MAX;
- }
-
- if (l2 == null) {
- l2 = MAX;
- }
-
- comp = l2.compareTo(l1);
- }
- }
- }
-
- return comp;
- }
- }
-
- private static final ConditionComparator CONDITION_COMPARATOR = new ConditionComparator();
+ static final Comparator<Condition> CONDITION_COMPARATOR = Comparator
+ .comparing(Condition::getFamily).thenComparing(Condition::getQualifier)
+ .thenComparing(Condition::getVisibility)
+ .thenComparing(Condition::getTimestamp, TIMESTAMP_COMPARATOR);
private List<TCondition> convertConditions(ConditionalMutation cm,
CompressedIterators compressedIters) {
diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java b/core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java
index 6a296d612b..746ae5dda5 100644
--- a/core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java
+++ b/core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java
@@ -18,7 +18,6 @@
import static org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly;
-import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -57,40 +56,24 @@
private static final Logger log = LoggerFactory.getLogger(TabletLocatorImpl.class);
- // there seems to be a bug in TreeMap.tailMap related to
- // putting null in the treemap.. therefore instead of
- // putting null, put MAX_TEXT
+ // MAX_TEXT represents a TEXT object that is greater than all others. Attempted to use null for
+ // this purpose, but there seems to be a bug in TreeMap.tailMap with null. Therefore instead of
+ // using null, created MAX_TEXT.
static final Text MAX_TEXT = new Text();
- private static class EndRowComparator implements Comparator<Text>, Serializable {
-
- private static final long serialVersionUID = 1L;
-
- @Override
- public int compare(Text o1, Text o2) {
-
- int ret;
-
- if (o1 == MAX_TEXT)
- if (o2 == MAX_TEXT)
- ret = 0;
- else
- ret = 1;
- else if (o2 == MAX_TEXT)
- ret = -1;
- else
- ret = o1.compareTo(o2);
-
- return ret;
- }
-
- }
-
- static final EndRowComparator endRowComparator = new EndRowComparator();
+ static final Comparator<Text> END_ROW_COMPARATOR = (o1, o2) -> {
+ if (o1 == o2)
+ return 0;
+ if (o1 == MAX_TEXT)
+ return 1;
+ if (o2 == MAX_TEXT)
+ return -1;
+ return o1.compareTo(o2);
+ };
protected Table.ID tableId;
protected TabletLocator parent;
- protected TreeMap<Text,TabletLocation> metaCache = new TreeMap<>(endRowComparator);
+ protected TreeMap<Text,TabletLocation> metaCache = new TreeMap<>(END_ROW_COMPARATOR);
protected TabletLocationObtainer locationObtainer;
private TabletServerLockChecker lockChecker;
protected Text lastTabletRow;
diff --git a/core/src/main/java/org/apache/accumulo/core/data/Column.java b/core/src/main/java/org/apache/accumulo/core/data/Column.java
index a95f2cdeb6..6752f60a83 100644
--- a/core/src/main/java/org/apache/accumulo/core/data/Column.java
+++ b/core/src/main/java/org/apache/accumulo/core/data/Column.java
@@ -23,6 +23,7 @@
import java.io.DataOutput;
import java.io.IOException;
import java.nio.ByteBuffer;
+import java.util.Comparator;
import org.apache.accumulo.core.data.thrift.TColumn;
import org.apache.hadoop.io.WritableComparable;
@@ -33,15 +34,13 @@
*/
public class Column implements WritableComparable<Column> {
- static private int compareBytes(byte[] a, byte[] b) {
- if (a == null && b == null)
- return 0;
- if (a == null)
- return -1;
- if (b == null)
- return 1;
- return WritableComparator.compareBytes(a, 0, a.length, b, 0, b.length);
- }
+ private static final Comparator<byte[]> BYTE_COMPARATOR = Comparator
+ .nullsFirst((a, b) -> WritableComparator.compareBytes(a, 0, a.length, b, 0, b.length));
+
+ private static final Comparator<Column> COMPARATOR = Comparator
+ .comparing(Column::getColumnFamily, BYTE_COMPARATOR)
+ .thenComparing(Column::getColumnQualifier, BYTE_COMPARATOR)
+ .thenComparing(Column::getColumnVisibility, BYTE_COMPARATOR);
/**
* Compares this column to another. Column families are compared first, then qualifiers, then
@@ -53,14 +52,7 @@ static private int compareBytes(byte[] a, byte[] b) {
*/
@Override
public int compareTo(Column that) {
- int result;
- result = compareBytes(this.columnFamily, that.columnFamily);
- if (result != 0)
- return result;
- result = compareBytes(this.columnQualifier, that.columnQualifier);
- if (result != 0)
- return result;
- return compareBytes(this.columnVisibility, that.columnVisibility);
+ return COMPARATOR.compare(this, that);
}
@Override
diff --git a/core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java b/core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java
index d4833d517d..c20ad5246f 100644
--- a/core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java
+++ b/core/src/main/java/org/apache/accumulo/core/data/impl/KeyExtent.java
@@ -28,6 +28,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
+import java.util.Comparator;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Set;
@@ -383,32 +384,16 @@ public static Mutation getPrevRowUpdateMutation(KeyExtent ke) {
return m;
}
+ // The last tablet in a table has no end row, so null sorts last for end row; similarly, the first
+ // tablet has no previous end row, so null sorts first for previous end row
+ private static final Comparator<KeyExtent> COMPARATOR = Comparator
+ .comparing(KeyExtent::getTableId)
+ .thenComparing(KeyExtent::getEndRow, Comparator.nullsLast(Text::compareTo))
+ .thenComparing(KeyExtent::getPrevEndRow, Comparator.nullsFirst(Text::compareTo));
+
@Override
public int compareTo(KeyExtent other) {
-
- int result = getTableId().compareTo(other.getTableId());
- if (result != 0)
- return result;
-
- if (this.getEndRow() == null) {
- if (other.getEndRow() != null)
- return 1;
- } else {
- if (other.getEndRow() == null)
- return -1;
-
- result = getEndRow().compareTo(other.getEndRow());
- if (result != 0)
- return result;
- }
- if (this.getPrevEndRow() == null) {
- if (other.getPrevEndRow() == null)
- return 0;
- return -1;
- }
- if (other.getPrevEndRow() == null)
- return 1;
- return this.getPrevEndRow().compareTo(other.getPrevEndRow());
+ return COMPARATOR.compare(this, other);
}
private int hashCode = 0;
diff --git a/core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java b/core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java
index 1a37b7c1a3..d6e904c208 100644
--- a/core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java
+++ b/core/src/main/java/org/apache/accumulo/core/iterators/IteratorUtil.java
@@ -19,7 +19,6 @@
import static java.util.Objects.requireNonNull;
import java.io.IOException;
-import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -72,15 +71,8 @@
majc, minc, scan;
}
- public static class IterInfoComparator implements Comparator<IterInfo>, Serializable {
- private static final long serialVersionUID = 1L;
-
- @Override
- public int compare(IterInfo o1, IterInfo o2) {
- return (o1.priority < o2.priority ? -1 : (o1.priority == o2.priority ? 0 : 1));
- }
-
- }
+ private static Comparator<IterInfo> ITER_INFO_COMPARATOR = Comparator
+ .comparingInt(IterInfo::getPriority);
/**
* Fetch the correct configuration key prefix for the given scope. Throws an
@@ -130,7 +122,7 @@ public static void mergeIteratorConfig(List<IterInfo> destList,
Map<String,Map<String,String>> ssio) {
destList.addAll(tableIters);
destList.addAll(ssi);
- Collections.sort(destList, new IterInfoComparator());
+ Collections.sort(destList, ITER_INFO_COMPARATOR);
Set<Entry<String,Map<String,String>>> es = tableOpts.entrySet();
for (Entry<String,Map<String,String>> entry : es) {
@@ -176,7 +168,7 @@ public static void parseIterConf(IteratorScope scope, List<IterInfo> iters,
}
}
- Collections.sort(iters, new IterInfoComparator());
+ Collections.sort(iters, ITER_INFO_COMPARATOR);
}
// @formatter:off
diff --git a/core/src/main/java/org/apache/accumulo/core/iterators/system/HeapIterator.java b/core/src/main/java/org/apache/accumulo/core/iterators/system/HeapIterator.java
index 815b247704..43c15d928f 100644
--- a/core/src/main/java/org/apache/accumulo/core/iterators/system/HeapIterator.java
+++ b/core/src/main/java/org/apache/accumulo/core/iterators/system/HeapIterator.java
@@ -17,7 +17,6 @@
package org.apache.accumulo.core.iterators.system;
import java.io.IOException;
-import java.util.Comparator;
import java.util.PriorityQueue;
import org.apache.accumulo.core.data.Key;
@@ -33,14 +32,6 @@
private SortedKeyValueIterator<Key,Value> topIdx = null;
private Key nextKey;
- private static class SKVIComparator implements Comparator<SortedKeyValueIterator<Key,Value>> {
-
- @Override
- public int compare(SortedKeyValueIterator<Key,Value> o1, SortedKeyValueIterator<Key,Value> o2) {
- return o1.getTopKey().compareTo(o2.getTopKey());
- }
- }
-
protected HeapIterator() {
heap = null;
}
@@ -53,7 +44,8 @@ protected void createHeap(int maxSize) {
if (heap != null)
throw new IllegalStateException("heap already exist");
- heap = new PriorityQueue<>(maxSize == 0 ? 1 : maxSize, new SKVIComparator());
+ heap = new PriorityQueue<>(maxSize == 0 ? 1 : maxSize,
+ (si1, si2) -> si1.getTopKey().compareTo(si2.getTopKey()));
}
@Override
diff --git a/core/src/test/java/org/apache/accumulo/core/client/impl/ConditionalComparatorTest.java b/core/src/test/java/org/apache/accumulo/core/client/impl/ConditionalComparatorTest.java
index f44f929792..b8ba83c0bd 100644
--- a/core/src/test/java/org/apache/accumulo/core/client/impl/ConditionalComparatorTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/client/impl/ConditionalComparatorTest.java
@@ -17,7 +17,8 @@
package org.apache.accumulo.core.client.impl;
-import org.apache.accumulo.core.client.impl.ConditionalWriterImpl.ConditionComparator;
+import java.util.Comparator;
+
import org.apache.accumulo.core.data.Condition;
import org.apache.accumulo.core.security.ColumnVisibility;
import org.junit.Assert;
@@ -34,7 +35,7 @@ public void testComparator() {
Condition c6 = new Condition("a", "b").setVisibility(new ColumnVisibility("A&B"));
Condition c7 = new Condition("a", "b").setVisibility(new ColumnVisibility("A&C"));
- ConditionComparator comparator = new ConditionComparator();
+ Comparator<Condition> comparator = ConditionalWriterImpl.CONDITION_COMPARATOR;
Assert.assertEquals(0, comparator.compare(c1, c1));
Assert.assertTrue(comparator.compare(c1, c2) < 0);
diff --git a/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java b/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java
index fd571b06aa..30908f924f 100644
--- a/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java
@@ -124,7 +124,7 @@ static Range nr(String k1, String k2) {
static TreeMap<Text,TabletLocation> createMetaCache(Object... data) {
TreeMap<KeyExtent,TabletLocation> mcke = createMetaCacheKE(data);
- TreeMap<Text,TabletLocation> mc = new TreeMap<>(TabletLocatorImpl.endRowComparator);
+ TreeMap<Text,TabletLocation> mc = new TreeMap<>(TabletLocatorImpl.END_ROW_COMPARATOR);
for (Entry<KeyExtent,TabletLocation> entry : mcke.entrySet()) {
if (entry.getKey().getEndRow() == null)
diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionQueue.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionQueue.java
index 2b406160c2..1d90e70bec 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionQueue.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionQueue.java
@@ -36,20 +36,16 @@
private List<TraceRunnable> task = new LinkedList<>();
- private static final Comparator<TraceRunnable> comparator = new Comparator<TraceRunnable>() {
- @SuppressWarnings("unchecked")
- @Override
- public int compare(TraceRunnable o1, TraceRunnable o2) {
- return ((Comparable<Runnable>) o1.getRunnable()).compareTo(o2.getRunnable());
- }
- };
+ @SuppressWarnings("unchecked")
+ private static final Comparator<TraceRunnable> ELEMENT_COMPARATOR = (o1,
+ o2) -> ((Comparable<Runnable>) o1.getRunnable()).compareTo(o2.getRunnable());
@Override
public synchronized TraceRunnable poll() {
if (task.size() == 0)
return null;
- TraceRunnable min = Collections.min(task, comparator);
+ TraceRunnable min = Collections.min(task, ELEMENT_COMPARATOR);
Iterator<TraceRunnable> iterator = task.iterator();
while (iterator.hasNext()) {
if (iterator.next() == min) {
@@ -65,7 +61,7 @@ public synchronized TraceRunnable peek() {
if (task.size() == 0)
return null;
- return Collections.min(task, comparator);
+ return Collections.min(task, ELEMENT_COMPARATOR);
}
@Override
@@ -122,7 +118,7 @@ public synchronized int drainTo(Collection<? super TraceRunnable> c) {
@Override
public synchronized int drainTo(Collection<? super TraceRunnable> c, int maxElements) {
- Collections.sort(task, comparator);
+ Collections.sort(task, ELEMENT_COMPARATOR);
int num = Math.min(task.size(), maxElements);
@@ -137,7 +133,7 @@ public synchronized int drainTo(Collection<? super TraceRunnable> c, int maxElem
@Override
public synchronized Iterator<TraceRunnable> iterator() {
- Collections.sort(task, comparator);
+ Collections.sort(task, ELEMENT_COMPARATOR);
return task.iterator();
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services