Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
opensearch-project · Apr 21, 2023 · e90e6ec · e90e6ec
1 parent f4f136f
commit e90e6ec
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 24 deletions.
diff --git a/libs/common/src/main/java/org/opensearch/common/Numbers.java b/libs/common/src/main/java/org/opensearch/common/Numbers.java
@@ -119,9 +119,7 @@ public static BigInteger toBigIntegerExact(Number n) {
         } else if (n instanceof BigInteger) {
             return ((BigInteger) n);
         } else {
-            throw new IllegalArgumentException(
-                "Cannot check whether [" + n + "] of class [" + n.getClass().getName() + "] is actually a long"
-            );
+            throw new IllegalArgumentException("Cannot convert [" + n + "] of class [" + n.getClass().getName() + "] to a BigInteger");
         }
     }
 
@@ -131,7 +129,7 @@ public static BigInteger toBigIntegerExact(Number n) {
     public static BigInteger toUnsignedLongExact(Number value) {
         final BigInteger v = Numbers.toBigIntegerExact(value);
 
-        if (v.compareTo(Numbers.MAX_UNSIGNED_LONG_VALUE) > 0 || v.compareTo(BigInteger.ZERO) < 0) {
+        if (v.compareTo(MAX_UNSIGNED_LONG_VALUE) > 0 || v.compareTo(MIN_UNSIGNED_LONG_VALUE) < 0) {
             throw new IllegalArgumentException("Value [" + value + "] is out of range for an unsigned long");
         }
 
@@ -192,7 +190,7 @@ public static BigInteger toUnsignedLong(String stringValue, boolean coerce) {
             throw new IllegalArgumentException("For input string: \"" + stringValue + "\"");
         }
 
-        if (bigIntegerValue.compareTo(MAX_UNSIGNED_LONG_VALUE) > 0 || bigIntegerValue.compareTo(BigInteger.ZERO) < 0) {
+        if (bigIntegerValue.compareTo(MAX_UNSIGNED_LONG_VALUE) > 0 || bigIntegerValue.compareTo(MIN_UNSIGNED_LONG_VALUE) < 0) {
             throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for an unsigned long");
         }
 

diff --git a/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java b/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java
@@ -31,7 +31,6 @@
 
 import java.io.IOException;
 import java.math.BigInteger;
-import java.util.function.Function;
 
 /**
  * Comparator source for unsigned long values.
@@ -41,27 +40,15 @@
 public class UnsignedLongValuesComparatorSource extends IndexFieldData.XFieldComparatorSource {
 
     private final IndexNumericFieldData indexFieldData;
-    private final Function<SortedNumericDocValues, SortedNumericDocValues> converter;
 
     public UnsignedLongValuesComparatorSource(
         IndexNumericFieldData indexFieldData,
         @Nullable Object missingValue,
         MultiValueMode sortMode,
         Nested nested
-    ) {
-        this(indexFieldData, missingValue, sortMode, nested, null);
-    }
-
-    public UnsignedLongValuesComparatorSource(
-        IndexNumericFieldData indexFieldData,
-        @Nullable Object missingValue,
-        MultiValueMode sortMode,
-        Nested nested,
-        Function<SortedNumericDocValues, SortedNumericDocValues> converter
     ) {
         super(missingValue, sortMode, nested);
         this.indexFieldData = indexFieldData;
-        this.converter = converter;
     }
 
     @Override
@@ -72,7 +59,7 @@ public SortField.Type reducedType() {
     private SortedNumericDocValues loadDocValues(LeafReaderContext context) {
         final LeafNumericFieldData data = indexFieldData.load(context);
         SortedNumericDocValues values = data.getLongValues();
-        return converter != null ? converter.apply(values) : values;
+        return values;
     }
 
     private NumericDocValues getNumericDocValues(LeafReaderContext context, BigInteger missingValue) throws IOException {
@@ -104,14 +91,14 @@ public Object missingObject(Object missingValue, boolean reversed) {
     public FieldComparator<?> newComparator(String fieldname, int numHits, boolean enableSkipping, boolean reversed) {
         assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName());
 
-        final BigInteger lMissingValue = (BigInteger) missingObject(missingValue, reversed);
+        final BigInteger ulMissingValue = (BigInteger) missingObject(missingValue, reversed);
         return new UnsignedLongComparator(numHits, null, null, reversed, false) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new UnsignedLongLeafComparator(context) {
                     @Override
                     protected NumericDocValues getNumericDocValues(LeafReaderContext context, String field) throws IOException {
-                        return UnsignedLongValuesComparatorSource.this.getNumericDocValues(context, lMissingValue);
+                        return UnsignedLongValuesComparatorSource.this.getNumericDocValues(context, ulMissingValue);
                     }
                 };
             }
@@ -127,12 +114,12 @@ public BucketedSort newBucketedSort(
         BucketedSort.ExtraData extra
     ) {
         return new BucketedSort.ForUnsignedLongs(bigArrays, sortOrder, format, bucketSize, extra) {
-            private final BigInteger lMissingValue = (BigInteger) missingObject(missingValue, sortOrder == SortOrder.DESC);
+            private final BigInteger ulMissingValue = (BigInteger) missingObject(missingValue, sortOrder == SortOrder.DESC);
 
             @Override
             public Leaf forLeaf(LeafReaderContext ctx) throws IOException {
                 return new Leaf(ctx) {
-                    private final NumericDocValues docValues = getNumericDocValues(ctx, lMissingValue);
+                    private final NumericDocValues docValues = getNumericDocValues(ctx, ulMissingValue);
                     private long docValue;
 
                     @Override

diff --git a/server/src/test/java/org/opensearch/common/NumbersTests.java b/server/src/test/java/org/opensearch/common/NumbersTests.java
@@ -212,7 +212,7 @@ public void testToUnsignedLongExact() {
         e = expectThrows(IllegalArgumentException.class, () -> Numbers.toUnsignedLongExact(3.1f));
         assertEquals("3.1 is not an integer value", e.getMessage());
         e = expectThrows(IllegalArgumentException.class, () -> Numbers.toUnsignedLongExact(new AtomicInteger(3))); // not supported
-        assertEquals("Cannot check whether [3] of class [java.util.concurrent.atomic.AtomicInteger] is actually a long", e.getMessage());
+        assertEquals("Cannot convert [3] of class [java.util.concurrent.atomic.AtomicInteger] to a BigInteger", e.getMessage());
     }
 
     public void testToUnsignedBigInteger() {