Fix max/min aggs for unsigned_long (#63904)

Max and min aggs were producing wrong results for unsigned_long field if field was indexed. If field is indexed for max/min aggs instead of field data, we use values from indexed Points, values of which are derived using method pointReaderIfPossible. Before UnsignedLongFieldType#pointReaderIfPossible was incorrectly producing values, as it failed to shift them back to original values. This patch fixes method pointReaderIfPossible to produce correct original values. Relates to #60050
elastic · Oct 19, 2020 · 0d453fd · 0d453fd
1 parent 62d8b13
commit 0d453fd
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 3 deletions.
diff --git a/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java b/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java
@@ -49,6 +49,8 @@
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import static org.elasticsearch.xpack.unsignedlong.UnsignedLongLeafFieldData.convertUnsignedLongToDouble;
+
 public class UnsignedLongFieldMapper extends ParametrizedFieldMapper {
     public static final String CONTENT_TYPE = "unsigned_long";
 
@@ -272,7 +274,8 @@ public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
         @Override
         public Function<byte[], Number> pointReaderIfPossible() {
             if (isSearchable()) {
-                return (value) -> LongPoint.decodeDimension(value, 0);
+                // convert from the shifted value back to the original value
+                return (value) -> convertUnsignedLongToDouble(LongPoint.decodeDimension(value, 0));
             }
             return null;
         }
@@ -525,7 +528,7 @@ private static long parseUnsignedLong(Object value) {
     }
 
     /**
-     * Convert an unsigned long to the singed long by subtract 2^63 from it
+     * Convert an unsigned long to the signed long by subtract 2^63 from it
      * @param value – unsigned long value in the range [0; 2^64-1], values greater than 2^63-1 are negative
      * @return signed long value in the range [-2^63; 2^63-1]
      */

diff --git a/...ed-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java b/...ed-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java
@@ -112,7 +112,7 @@ public Object nextValue() throws IOException {
         };
     }
 
-    private static double convertUnsignedLongToDouble(long value) {
+    static double convertUnsignedLongToDouble(long value) {
         if (value < 0L) {
             return sortableSignedLongToUnsigned(value); // add 2 ^ 63
         } else {

diff --git a/...r-unsigned-long/src/test/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongTests.java b/...r-unsigned-long/src/test/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongTests.java
@@ -21,7 +21,9 @@
 import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
 import org.elasticsearch.search.aggregations.bucket.range.Range;
 import org.elasticsearch.search.aggregations.bucket.terms.Terms;
+import org.elasticsearch.search.aggregations.metrics.Min;
 import org.elasticsearch.search.aggregations.metrics.Sum;
+import org.elasticsearch.search.aggregations.metrics.Max;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ESIntegTestCase;
 
@@ -36,6 +38,8 @@
 import static org.elasticsearch.search.aggregations.AggregationBuilders.histogram;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.range;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.sum;
+import static org.elasticsearch.search.aggregations.AggregationBuilders.max;
+import static org.elasticsearch.search.aggregations.AggregationBuilders.min;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
 import static org.hamcrest.Matchers.containsString;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.terms;
@@ -275,6 +279,20 @@ public void testAggs() {
             double expectedSum = Arrays.stream(values).mapToDouble(Number::doubleValue).sum();
             assertEquals(expectedSum, sum.getValue(), 0.001);
         }
+        // max agg
+        {
+            SearchResponse response = client().prepareSearch("idx").setSize(0).addAggregation(max("ul_max").field("ul_field")).get();
+            assertSearchResponse(response);
+            Max max = response.getAggregations().get("ul_max");
+            assertEquals(1.8446744073709551615E19, max.getValue(), 0.001);
+        }
+        // min agg
+        {
+            SearchResponse response = client().prepareSearch("idx").setSize(0).addAggregation(min("ul_min").field("ul_field")).get();
+            assertSearchResponse(response);
+            Min min = response.getAggregations().get("ul_min");
+            assertEquals(0, min.getValue(), 0.001);
+        }
     }
 
     public void testSortDifferentFormatsShouldFail() {