refactor: use the original from/to values when creating the bucket (e…

…lastic#83262) The range bucket values should always be set to the original `from` and `to` values. These values are the ones used by the serialisation logic and are the values the client expects in the response. Also, the key for a Range is made optional, avoiding unnecessary serialisation and deserialisation and the actual key generation happens only if required (calling 'getKey' or 'getKeyAsString' directly or at key serialisation time). A Range does not actually need a key to accomplish its purpose ('from' and 'to' are the only required parameters), other than propagating a user-specified key for the bucket collecting documents for that range.
vijay2308 · Jan 31, 2022 · a03cd92 · a03cd92
1 parent 035fdd8
commit a03cd92
Show file tree

Hide file tree

Showing 13 changed files with 102 additions and 195 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
@@ -124,8 +124,8 @@ setup:
 ---
 "Float range":
   - skip:
-      version: " - 7.16.99"
-      reason: Bug fixed in 8.1.0 and backported to 7.17.0
+      version: " - 8.1.0"
+      reason: Bug fixed in 8.1.0
   - do:
       search:
         index: test

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, from, fromAsString, to, to, toAsString);
+            return new RangeAggregator.Range(range.getKey(), from, fromAsString, to, toAsString);
         });
         if (ranges.length == 0) {
             throw new IllegalArgumentException("No [ranges] specified for the [" + this.getName() + "] aggregation");

diff --git a/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java b/...src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.search.aggregations.bucket.range;
 
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.search.DocValueFormat;
@@ -54,7 +55,7 @@ public Bucket(
         ) {
             this.format = format;
             this.keyed = keyed;
-            this.key = key != null ? key : generateKey(from, to, format);
+            this.key = key;
             this.from = from;
             this.to = to;
             this.docCount = docCount;
@@ -69,7 +70,7 @@ private static String generateKey(BytesRef from, BytesRef to, DocValueFormat for
         }
 
         private static Bucket createFromStream(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
-            String key = in.readString();
+            String key = in.getVersion().onOrAfter(Version.V_8_1_0) ? in.readOptionalString() : in.readString();
             BytesRef from = in.readBoolean() ? in.readBytesRef() : null;
             BytesRef to = in.readBoolean() ? in.readBytesRef() : null;
             long docCount = in.readLong();
@@ -80,7 +81,11 @@ private static Bucket createFromStream(StreamInput in, DocValueFormat format, bo
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
-            out.writeString(key);
+            if (out.getVersion().onOrAfter(Version.V_8_1_0)) {
+                out.writeOptionalString(key);
+            } else {
+                out.writeString(key == null ? generateKey(from, to, format) : key);
+            }
             out.writeBoolean(from != null);
             if (from != null) {
                 out.writeBytesRef(from);
@@ -95,12 +100,12 @@ public void writeTo(StreamOutput out) throws IOException {
 
         @Override
         public Object getKey() {
-            return key;
+            return getKeyAsString();
         }
 
         @Override
         public String getKeyAsString() {
-            return key;
+            return this.key == null ? generateKey(this.from, this.to, this.format) : this.key;
         }
 
         @Override
@@ -115,7 +120,7 @@ public Aggregations getAggregations() {
 
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-            String key = this.key;
+            final String key = getKeyAsString();
             if (keyed) {
                 builder.startObject(key);
             } else {

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,29 +28,25 @@ 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, originalFrom, to, originalTo, docCount, InternalAggregations.from(aggregations), keyed, formatter);
+            super(key, from, to, 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, originalFrom, to, originalTo, docCount, aggregations, keyed, formatter);
+            super(key, from, to, docCount, aggregations, keyed, formatter);
         }
 
         @Override
@@ -75,14 +71,6 @@ private Double internalGetTo() {
             return to;
         }
 
-        private Double internalGetOriginalFrom() {
-            return originalFrom;
-        }
-
-        private Double internalGetOriginalTo() {
-            return originalTo;
-        }
-
         @Override
         protected InternalRange.Factory<Bucket, ?> getFactory() {
             return FACTORY;
@@ -124,25 +112,21 @@ 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, originalFrom, to, originalTo, docCount, aggregations, keyed, formatter);
+            return new Bucket(key, from, to, 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,17 +23,8 @@ public class InternalGeoDistance extends InternalRange<InternalGeoDistance.Bucke
 
     static class Bucket extends InternalRange.Bucket {
 
-        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);
+        Bucket(String key, double from, double to, long docCount, InternalAggregations aggregations, boolean keyed) {
+            super(key, from, to, docCount, aggregations, keyed, DocValueFormat.RAW);
         }
 
         @Override
@@ -77,25 +68,21 @@ 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, originalFrom, to, originalTo, docCount, aggregations, keyed);
+            return new Bucket(key, from, to, 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()