[Refactor] Use TreeSet instead of ArrayList

BinaryFieldMapper uses an ArrayList and sorts elements when returning binary values. Use a TreeMap instead. This also lets us remove utility function, SortAndDedup as we no longer need it. Signed-off-by: Kiran Reddy <[email protected]>
opensearch-project · Aug 18, 2023 · 7b6def9 · 7b6def9
1 parent 8e57a66
commit 7b6def9
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 64 deletions.
diff --git a/libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java b/libs/core/src/main/java/org/opensearch/core/common/util/CollectionUtils.java
@@ -41,12 +41,9 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashMap;
 import java.util.IdentityHashMap;
-import java.util.Iterator;
 import java.util.List;
-import java.util.ListIterator;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
@@ -91,30 +88,6 @@ public static <T> List<T> rotate(final List<T> list, int distance) {
         return new RotatedList<>(list, d);
     }
 
-    /**
-     * in place de-duplicates items in a list
-     */
-    public static <T> void sortAndDedup(final List<T> array, Comparator<T> comparator) {
-        // base case: one item
-        if (array.size() <= 1) {
-            return;
-        }
-        array.sort(comparator);
-        ListIterator<T> deduped = array.listIterator();
-        T cmp = deduped.next(); // return the first item and advance
-        Iterator<T> oldArray = array.iterator();
-        oldArray.next(); // advance to the old to the second item (advanced to third below)
-
-        do {
-            T old = oldArray.next(); // get the next item and advance iter
-            if (comparator.compare(cmp, old) != 0 && (cmp = deduped.next()) != old) {
-                deduped.set(old);
-            }
-        } while (oldArray.hasNext());
-        // in place update
-        array.subList(deduped.nextIndex(), array.size()).clear();
-    }
-
     public static int[] toArray(Collection<Integer> ints) {
         Objects.requireNonNull(ints);
         return ints.stream().mapToInt(s -> s).toArray();

diff --git a/server/src/main/java/org/opensearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/BinaryFieldMapper.java
@@ -53,12 +53,12 @@
 
 import java.io.IOException;
 import java.time.ZoneId;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Base64;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeSet;
 import java.util.function.Supplier;
 
 /**
@@ -242,28 +242,26 @@ protected String contentType() {
      */
     public static class CustomBinaryDocValuesField extends CustomDocValuesField {
 
-        private final ArrayList<byte[]> bytesList;
+        private final TreeSet<byte[]> bytesSet;
 
         public CustomBinaryDocValuesField(String name, byte[] bytes) {
             super(name);
-            bytesList = new ArrayList<>();
+            bytesSet = new TreeSet<>(Arrays::compareUnsigned);
             add(bytes);
         }
 
         public void add(byte[] bytes) {
-            bytesList.add(bytes);
+            bytesSet.add(bytes);
         }
 
         @Override
         public BytesRef binaryValue() {
             try {
-                // sort and dedup in place
-                CollectionUtils.sortAndDedup(bytesList, Arrays::compareUnsigned);
-                int size = bytesList.stream().map(b -> b.length).reduce(0, Integer::sum);
-                int length = bytesList.size();
+                int size = bytesSet.stream().map(b -> b.length).reduce(0, Integer::sum);
+                int length = bytesSet.size();
                 try (BytesStreamOutput out = new BytesStreamOutput(size + (length + 1) * 5)) {
                     out.writeVInt(length);  // write total number of values
-                    for (byte[] value : bytesList) {
+                    for (byte[] value : bytesSet) {
                         int valueLength = value.length;
                         out.writeVInt(valueLength);
                         out.writeBytes(value, 0, valueLength);

diff --git a/server/src/test/java/org/opensearch/common/util/CollectionUtilsTests.java b/server/src/test/java/org/opensearch/common/util/CollectionUtilsTests.java
@@ -38,10 +38,8 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 
@@ -79,32 +77,6 @@ public void testRotate() {
         }
     }
 
-    private <T> void assertDeduped(List<T> array, Comparator<T> cmp, int expectedLength) {
-        // test the dedup w/ ArrayLists and LinkedLists
-        List<List<T>> types = List.of(new ArrayList<T>(array), new LinkedList<>(array));
-        for (List<T> clone : types) {
-            // dedup the list
-            CollectionUtils.sortAndDedup(clone, cmp);
-            // verify unique elements
-            for (int i = 0; i < clone.size() - 1; ++i) {
-                assertNotEquals(cmp.compare(clone.get(i), clone.get(i + 1)), 0);
-            }
-            assertEquals(expectedLength, clone.size());
-        }
-    }
-
-    public void testSortAndDedup() {
-        // test no elements in a string array
-        assertDeduped(List.<String>of(), Comparator.naturalOrder(), 0);
-        // test no elements in an integer array
-        assertDeduped(List.<Integer>of(), Comparator.naturalOrder(), 0);
-        // test unsorted array
-        assertDeduped(List.of(-1, 0, 2, 1, -1, 19, -1), Comparator.naturalOrder(), 5);
-        // test sorted array
-        assertDeduped(List.of(-1, 0, 1, 2, 19, 19), Comparator.naturalOrder(), 5);
-        // test sorted
-    }
-
     public void testEmptyPartition() {
         assertEquals(Collections.emptyList(), eagerPartition(Collections.emptyList(), 1));
     }