Fix bucket keys format for range aggregations on float field (#81801)

When dealing with float fields we actually store float values with lower precision. Later on, when loading float values into double values, the precision difference between float and double values surfaces as additional "spurious" digits in the string representation which is propagated to the client during serialisation. As a result of this, the JSON response returned to the client includes range values not matching values in the request. This results in clients, including Kibana, to break while trying to match ranges in the request with ranges in the response. With this change we use two new fields, `originalFrom` and `originalTo` to hold the original double values before manipulating the precision of float fields. Later we use these fields in the code dealing with serialisation. The effect is that precision issue resulting from float to double conversions are not propagated to clients.
elastic · Jan 11, 2022 · 3550370 · 3550370
1 parent e747898
commit 3550370
Show file tree

Hide file tree

Showing 13 changed files with 356 additions and 43 deletions.
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.yml
@@ -91,8 +91,84 @@ setup:
   - match: { aggregations.double_range.buckets.1.key: "0.0152-1.0" }
   - match: { aggregations.double_range.buckets.1.doc_count: 2 }
 
+---
+"Float range":
+  - skip:
+      version: " - 8.0.99"
+      reason: Bug fixed in 8.1.0
+  - do:
+      search:
+        body:
+          size: 0
+          aggs:
+            float_range:
+              range:
+                field: "float"
+                ranges:
+                  -
+                    to: 6.0
+                  -
+                    from: 6.0
+                    to: 10.6
+                  -
+                    from: 10.6
+
+  - match: { hits.total.relation: "eq" }
+  - match: { hits.total.value: 4 }
+  - length: { aggregations.float_range.buckets: 3 }
+  - match: { aggregations.float_range.buckets.0.key: "*-6.0" }
+  - is_false: aggregations.float_range.buckets.0.from
+  - match: { aggregations.float_range.buckets.0.to: 6.0 }
+  - match: { aggregations.float_range.buckets.0.doc_count: 3 }
+  - match: { aggregations.float_range.buckets.1.key: "6.0-10.6" }
+  - match: { aggregations.float_range.buckets.1.from: 6.0 }
+  - match: { aggregations.float_range.buckets.1.to: 10.6 }
+  - match: { aggregations.float_range.buckets.1.doc_count: 0 }
+  - match: { aggregations.float_range.buckets.2.key: "10.6-*" }
+  - match: { aggregations.float_range.buckets.2.from: 10.6 }
+  - is_false:  aggregations.float_range.buckets.2.to
+  - match: { aggregations.float_range.buckets.2.doc_count: 0 }
+
 ---
 "Double range":
+  - skip:
+      version: " - 8.0.99"
+      reason: Bug fixed in 8.1.0
+  - do:
+      search:
+        body:
+          size: 0
+          aggs:
+            float_range:
+              range:
+                field: "double"
+                ranges:
+                  -
+                    to: 6.0
+                  -
+                    from: 6.0
+                    to: 10.6
+                  -
+                    from: 10.6
+
+  - match: { hits.total.relation: "eq" }
+  - match: { hits.total.value: 4 }
+  - length: { aggregations.float_range.buckets: 3 }
+  - match: { aggregations.float_range.buckets.0.key: "*-6.0" }
+  - is_false: aggregations.float_range.buckets.0.from
+  - match: { aggregations.float_range.buckets.0.to: 6.0 }
+  - match: { aggregations.float_range.buckets.0.doc_count: 0 }
+  - match: { aggregations.float_range.buckets.1.key: "6.0-10.6" }
+  - match: { aggregations.float_range.buckets.1.from: 6.0 }
+  - match: { aggregations.float_range.buckets.1.to: 10.6 }
+  - match: { aggregations.float_range.buckets.1.doc_count: 0 }
+  - match: { aggregations.float_range.buckets.2.key: "10.6-*" }
+  - match: { aggregations.float_range.buckets.2.from: 10.6 }
+  - is_false:  aggregations.float_range.buckets.2.to
+  - match: { aggregations.float_range.buckets.2.doc_count: 3 }
+
+---
+"Double range no decimal":
   - do:
       search:
         body:

diff --git a/.../java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java b/.../java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java
@@ -364,7 +364,7 @@ protected DateRangeAggregatorFactory innerBuild(
             } else if (Double.isFinite(to)) {
                 to = parser.parseDouble(Long.toString((long) to), false, context::nowInMillis);
             }
-            return new RangeAggregator.Range(range.getKey(), from, fromAsString, to, toAsString);
+            return new RangeAggregator.Range(range.getKey(), from, from, fromAsString, to, to, toAsString);
         });
         if (ranges.length == 0) {
             throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation");

diff --git a/...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalDateRange.java b/...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalDateRange.java
@@ -28,25 +28,29 @@ public static class Bucket extends InternalRange.Bucket {
         public Bucket(
             String key,
             double from,
+            double originalFrom,
             double to,
+            double originalTo,
             long docCount,
             List<InternalAggregation> aggregations,
             boolean keyed,
             DocValueFormat formatter
         ) {
-            super(key, from, to, docCount, InternalAggregations.from(aggregations), keyed, formatter);
+            super(key, from, originalFrom, to, originalTo, docCount, InternalAggregations.from(aggregations), keyed, formatter);
         }
 
         public Bucket(
             String key,
             double from,
+            double originalFrom,
             double to,
+            double originalTo,
             long docCount,
             InternalAggregations aggregations,
             boolean keyed,
             DocValueFormat formatter
         ) {
-            super(key, from, to, docCount, aggregations, keyed, formatter);
+            super(key, from, originalFrom, to, originalTo, docCount, aggregations, keyed, formatter);
         }
 
         @Override
@@ -71,6 +75,14 @@ private Double internalGetTo() {
             return to;
         }
 
+        private Double internalGetOriginalFrom() {
+            return originalFrom;
+        }
+
+        private Double internalGetOriginalTo() {
+            return originalTo;
+        }
+
         @Override
         protected InternalRange.Factory<Bucket, ?> getFactory() {
             return FACTORY;
@@ -112,21 +124,25 @@ public InternalDateRange create(List<Bucket> ranges, InternalDateRange prototype
         public Bucket createBucket(
             String key,
             double from,
+            double originalFrom,
             double to,
+            double originalTo,
             long docCount,
             InternalAggregations aggregations,
             boolean keyed,
             DocValueFormat formatter
         ) {
-            return new Bucket(key, from, to, docCount, aggregations, keyed, formatter);
+            return new Bucket(key, from, originalFrom, to, originalTo, docCount, aggregations, keyed, formatter);
         }
 
         @Override
         public Bucket createBucket(InternalAggregations aggregations, Bucket prototype) {
             return new Bucket(
                 prototype.getKey(),
                 prototype.internalGetFrom(),
+                prototype.internalGetOriginalFrom(),
                 prototype.internalGetTo(),
+                prototype.internalGetOriginalTo(),
                 prototype.getDocCount(),
                 aggregations,
                 prototype.getKeyed(),

diff --git a/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalGeoDistance.java b/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalGeoDistance.java
@@ -23,8 +23,17 @@ public class InternalGeoDistance extends InternalRange<InternalGeoDistance.Bucke
 
     static class Bucket extends InternalRange.Bucket {
 
-        Bucket(String key, double from, double to, long docCount, InternalAggregations aggregations, boolean keyed) {
-            super(key, from, to, docCount, aggregations, keyed, DocValueFormat.RAW);
+        Bucket(
+            String key,
+            double from,
+            double originalFrom,
+            double to,
+            double originalTo,
+            long docCount,
+            InternalAggregations aggregations,
+            boolean keyed
+        ) {
+            super(key, from, originalFrom, to, originalTo, docCount, aggregations, keyed, DocValueFormat.RAW);
         }
 
         @Override
@@ -68,21 +77,25 @@ public InternalGeoDistance create(List<Bucket> ranges, InternalGeoDistance proto
         public Bucket createBucket(
             String key,
             double from,
+            double originalFrom,
             double to,
+            double originalTo,
             long docCount,
             InternalAggregations aggregations,
             boolean keyed,
             DocValueFormat format
         ) {
-            return new Bucket(key, from, to, docCount, aggregations, keyed);
+            return new Bucket(key, from, originalFrom, to, originalTo, docCount, aggregations, keyed);
         }
 
         @Override
         public Bucket createBucket(InternalAggregations aggregations, Bucket prototype) {
             return new Bucket(
                 prototype.getKey(),
                 ((Number) prototype.getFrom()).doubleValue(),
+                ((Number) prototype.getOriginalFrom()).doubleValue(),
                 ((Number) prototype.getTo()).doubleValue(),
+                ((Number) prototype.getOriginalTo()).doubleValue(),
                 prototype.getDocCount(),
                 aggregations,
                 prototype.getKeyed()