Revert "Support first and last parameter for missing bucket ordering …

…in composite aggregation (#1942) (#2049)" This reverts commit 5b27136. Signed-off-by: Andrew Ross <[email protected]>
opensearch-project · Feb 16, 2022 · 42cc5f0 · 42cc5f0
1 parent bfe128c
commit 42cc5f0
Show file tree

Hide file tree

Showing 21 changed files with 77 additions and 850 deletions.
diff --git a/...src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java b/...src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java
@@ -47,10 +47,8 @@
 import org.opensearch.index.mapper.StringFieldType;
 import org.opensearch.search.DocValueFormat;
 import org.opensearch.search.aggregations.LeafBucketCollector;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 
 import java.io.IOException;
-import java.util.Objects;
 import java.util.function.LongConsumer;
 
 /**
@@ -70,11 +68,10 @@ class BinaryValuesSource extends SingleDimensionValuesSource<BytesRef> {
         CheckedFunction<LeafReaderContext, SortedBinaryDocValues, IOException> docValuesFunc,
         DocValueFormat format,
         boolean missingBucket,
-        MissingOrder missingOrder,
         int size,
         int reverseMul
     ) {
-        super(bigArrays, format, fieldType, missingBucket, missingOrder, size, reverseMul);
+        super(bigArrays, format, fieldType, missingBucket, size, reverseMul);
         this.breakerConsumer = breakerConsumer;
         this.docValuesFunc = docValuesFunc;
         this.values = bigArrays.newObjectArray(Math.min(size, 100));
@@ -104,9 +101,10 @@ void copyCurrent(int slot) {
     @Override
     int compare(int from, int to) {
         if (missingBucket) {
-            int result = missingOrder.compare(() -> Objects.isNull(values.get(from)), () -> Objects.isNull(values.get(to)), reverseMul);
-            if (MissingOrder.unknownOrder(result) == false) {
-                return result;
+            if (values.get(from) == null) {
+                return values.get(to) == null ? 0 : -1 * reverseMul;
+            } else if (values.get(to) == null) {
+                return reverseMul;
             }
         }
         return compareValues(values.get(from), values.get(to));
@@ -115,9 +113,10 @@ int compare(int from, int to) {
     @Override
     int compareCurrent(int slot) {
         if (missingBucket) {
-            int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(values.get(slot)), reverseMul);
-            if (MissingOrder.unknownOrder(result) == false) {
-                return result;
+            if (currentValue == null) {
+                return values.get(slot) == null ? 0 : -1 * reverseMul;
+            } else if (values.get(slot) == null) {
+                return reverseMul;
             }
         }
         return compareValues(currentValue, values.get(slot));
@@ -126,9 +125,10 @@ int compareCurrent(int slot) {
     @Override
     int compareCurrentWithAfter() {
         if (missingBucket) {
-            int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(afterValue), reverseMul);
-            if (MissingOrder.unknownOrder(result) == false) {
-                return result;
+            if (currentValue == null) {
+                return afterValue == null ? 0 : -1 * reverseMul;
+            } else if (afterValue == null) {
+                return reverseMul;
             }
         }
         return compareValues(currentValue, afterValue);

diff --git a/...rc/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java b/...rc/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregator.java
@@ -70,7 +70,6 @@
 import org.opensearch.search.aggregations.MultiBucketCollector;
 import org.opensearch.search.aggregations.MultiBucketConsumerService;
 import org.opensearch.search.aggregations.bucket.BucketsAggregator;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.internal.SearchContext;
 import org.opensearch.search.searchafter.SearchAfterBuilder;
 import org.opensearch.search.sort.SortAndFormats;
@@ -90,7 +89,6 @@ final class CompositeAggregator extends BucketsAggregator {
     private final int size;
     private final List<String> sourceNames;
     private final int[] reverseMuls;
-    private final MissingOrder[] missingOrders;
     private final List<DocValueFormat> formats;
     private final CompositeKey rawAfterKey;
 
@@ -119,7 +117,6 @@ final class CompositeAggregator extends BucketsAggregator {
         this.size = size;
         this.sourceNames = Arrays.stream(sourceConfigs).map(CompositeValuesSourceConfig::name).collect(Collectors.toList());
         this.reverseMuls = Arrays.stream(sourceConfigs).mapToInt(CompositeValuesSourceConfig::reverseMul).toArray();
-        this.missingOrders = Arrays.stream(sourceConfigs).map(CompositeValuesSourceConfig::missingOrder).toArray(MissingOrder[]::new);
         this.formats = Arrays.stream(sourceConfigs).map(CompositeValuesSourceConfig::format).collect(Collectors.toList());
         this.sources = new SingleDimensionValuesSource[sourceConfigs.length];
         // check that the provided size is not greater than the search.max_buckets setting
@@ -192,15 +189,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
             CompositeKey key = queue.toCompositeKey(slot);
             InternalAggregations aggs = subAggsForBuckets[slot];
             int docCount = queue.getDocCount(slot);
-            buckets[queue.size()] = new InternalComposite.InternalBucket(
-                sourceNames,
-                formats,
-                key,
-                reverseMuls,
-                missingOrders,
-                docCount,
-                aggs
-            );
+            buckets[queue.size()] = new InternalComposite.InternalBucket(sourceNames, formats, key, reverseMuls, docCount, aggs);
         }
         CompositeKey lastBucket = num > 0 ? buckets[num - 1].getRawKey() : null;
         return new InternalAggregation[] {
@@ -212,26 +201,14 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
                 Arrays.asList(buckets),
                 lastBucket,
                 reverseMuls,
-                missingOrders,
                 earlyTerminated,
                 metadata()
             ) };
     }
 
     @Override
     public InternalAggregation buildEmptyAggregation() {
-        return new InternalComposite(
-            name,
-            size,
-            sourceNames,
-            formats,
-            Collections.emptyList(),
-            null,
-            reverseMuls,
-            missingOrders,
-            false,
-            metadata()
-        );
+        return new InternalComposite(name, size, sourceNames, formats, Collections.emptyList(), null, reverseMuls, false, metadata());
     }
 
     private void finishLeaf() {

diff --git a/...ava/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/...ava/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java
@@ -33,15 +33,13 @@
 package org.opensearch.search.aggregations.bucket.composite;
 
 import org.opensearch.LegacyESVersion;
-import org.opensearch.Version;
 import org.opensearch.common.io.stream.StreamInput;
 import org.opensearch.common.io.stream.StreamOutput;
 import org.opensearch.common.io.stream.Writeable;
 import org.opensearch.common.xcontent.ToXContentFragment;
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.script.Script;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.aggregations.support.ValueType;
 import org.opensearch.search.aggregations.support.ValuesSource;
 import org.opensearch.search.aggregations.support.ValuesSourceConfig;
@@ -52,8 +50,6 @@
 import java.time.ZoneId;
 import java.util.Objects;
 
-import static org.opensearch.search.aggregations.bucket.missing.MissingOrder.fromString;
-
 /**
  * A {@link ValuesSource} builder for {@link CompositeAggregationBuilder}
  */
@@ -64,7 +60,6 @@ public abstract class CompositeValuesSourceBuilder<AB extends CompositeValuesSou
     private Script script = null;
     private ValueType userValueTypeHint = null;
     private boolean missingBucket = false;
-    private MissingOrder missingOrder = MissingOrder.DEFAULT;
     private SortOrder order = SortOrder.ASC;
     private String format = null;
 
@@ -90,9 +85,6 @@ public abstract class CompositeValuesSourceBuilder<AB extends CompositeValuesSou
             // skip missing value for BWC
             in.readGenericValue();
         }
-        if (in.getVersion().onOrAfter(Version.V_1_3_0)) {
-            this.missingOrder = MissingOrder.readFromStream(in);
-        }
         this.order = SortOrder.readFromStream(in);
         if (in.getVersion().onOrAfter(LegacyESVersion.V_6_3_0)) {
             this.format = in.readOptionalString();
@@ -122,9 +114,6 @@ public final void writeTo(StreamOutput out) throws IOException {
             // write missing value for BWC
             out.writeGenericValue(null);
         }
-        if (out.getVersion().onOrAfter(Version.V_1_3_0)) {
-            missingOrder.writeTo(out);
-        }
         order.writeTo(out);
         if (out.getVersion().onOrAfter(LegacyESVersion.V_6_3_0)) {
             out.writeOptionalString(format);
@@ -152,9 +141,6 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
         if (format != null) {
             builder.field("format", format);
         }
-        if (MissingOrder.isDefault(missingOrder) == false) {
-            builder.field(MissingOrder.NAME, missingOrder.toString());
-        }
         builder.field("order", order);
         doXContentBody(builder, params);
         builder.endObject();
@@ -177,7 +163,6 @@ public boolean equals(Object o) {
             && Objects.equals(script, that.script())
             && Objects.equals(userValueTypeHint, that.userValuetypeHint())
             && Objects.equals(missingBucket, that.missingBucket())
-            && Objects.equals(missingOrder, that.missingOrder())
             && Objects.equals(order, that.order())
             && Objects.equals(format, that.format());
     }
@@ -262,29 +247,6 @@ public boolean missingBucket() {
         return missingBucket;
     }
 
-    /**
-     * Sets the {@link MissingOrder} to use to order missing value.
-     */
-    public AB missingOrder(MissingOrder missingOrder) {
-        this.missingOrder = missingOrder;
-        return (AB) this;
-    }
-
-    /**
-     * Sets the {@link MissingOrder} to use to order missing value.
-     * @param missingOrder "first", "last" or "default".
-     */
-    public AB missingOrder(String missingOrder) {
-        return missingOrder(fromString(missingOrder));
-    }
-
-    /**
-     * Missing value order. {@link MissingOrder}.
-     */
-    public MissingOrder missingOrder() {
-        return missingOrder;
-    }
-
     /**
      * Sets the {@link SortOrder} to use to sort values produced this source
      */
@@ -345,9 +307,6 @@ protected abstract CompositeValuesSourceConfig innerBuild(QueryShardContext quer
     protected abstract ValuesSourceType getDefaultValuesSourceType();
 
     public final CompositeValuesSourceConfig build(QueryShardContext queryShardContext) throws IOException {
-        if (missingBucket == false && missingOrder != MissingOrder.DEFAULT) {
-            throw new IllegalArgumentException(MissingOrder.NAME + " require missing_bucket is true");
-        }
         ValuesSourceConfig config = ValuesSourceConfig.resolve(
             queryShardContext,
             userValueTypeHint,

diff --git a/...java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java b/...java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java
@@ -37,7 +37,6 @@
 import org.opensearch.common.util.BigArrays;
 import org.opensearch.index.mapper.MappedFieldType;
 import org.opensearch.search.DocValueFormat;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.aggregations.support.ValuesSource;
 import org.opensearch.search.sort.SortOrder;
 
@@ -63,7 +62,6 @@ SingleDimensionValuesSource<?> createValuesSource(
     private final DocValueFormat format;
     private final int reverseMul;
     private final boolean missingBucket;
-    private final MissingOrder missingOrder;
     private final boolean hasScript;
     private final SingleDimensionValuesSourceProvider singleDimensionValuesSourceProvider;
 
@@ -85,7 +83,6 @@ SingleDimensionValuesSource<?> createValuesSource(
         DocValueFormat format,
         SortOrder order,
         boolean missingBucket,
-        MissingOrder missingOrder,
         boolean hasScript,
         SingleDimensionValuesSourceProvider singleDimensionValuesSourceProvider
     ) {
@@ -97,7 +94,6 @@ SingleDimensionValuesSource<?> createValuesSource(
         this.missingBucket = missingBucket;
         this.hasScript = hasScript;
         this.singleDimensionValuesSourceProvider = singleDimensionValuesSourceProvider;
-        this.missingOrder = missingOrder;
     }
 
     /**
@@ -136,13 +132,6 @@ boolean missingBucket() {
         return missingBucket;
     }
 
-    /**
-     * Return the {@link MissingOrder} for the config.
-     */
-    MissingOrder missingOrder() {
-        return missingOrder;
-    }
-
     /**
      * Returns true if the source contains a script that can change the value.
      */

diff --git a/...rg/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java b/...rg/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java
@@ -43,7 +43,6 @@
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.script.Script;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.aggregations.support.ValueType;
 
 import java.io.IOException;
@@ -55,7 +54,6 @@ public class CompositeValuesSourceParserHelper {
     static <VB extends CompositeValuesSourceBuilder<VB>, T> void declareValuesSourceFields(AbstractObjectParser<VB, T> objectParser) {
         objectParser.declareField(VB::field, XContentParser::text, new ParseField("field"), ObjectParser.ValueType.STRING);
         objectParser.declareBoolean(VB::missingBucket, new ParseField("missing_bucket"));
-        objectParser.declareString(VB::missingOrder, new ParseField(MissingOrder.NAME));
 
         objectParser.declareField(VB::userValuetypeHint, p -> {
             ValueType valueType = ValueType.lenientParse(p.text());

diff --git a/...org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java b/...org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java
@@ -52,7 +52,6 @@
 import org.opensearch.search.aggregations.bucket.histogram.DateIntervalConsumer;
 import org.opensearch.search.aggregations.bucket.histogram.DateIntervalWrapper;
 import org.opensearch.search.aggregations.bucket.histogram.Histogram;
-import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.aggregations.support.CoreValuesSourceType;
 import org.opensearch.search.aggregations.support.ValuesSource;
 import org.opensearch.search.aggregations.support.ValuesSourceConfig;
@@ -82,7 +81,6 @@ CompositeValuesSourceConfig apply(
             boolean hasScript, // probably redundant with the config, but currently we check this two different ways...
             String format,
             boolean missingBucket,
-            MissingOrder missingOrder,
             SortOrder order
         );
     }
@@ -290,7 +288,7 @@ public static void register(ValuesSourceRegistry.Builder builder) {
         builder.register(
             REGISTRY_KEY,
             org.opensearch.common.collect.List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC),
-            (valuesSourceConfig, rounding, name, hasScript, format, missingBucket, missingOrder, order) -> {
+            (valuesSourceConfig, rounding, name, hasScript, format, missingBucket, order) -> {
                 ValuesSource.Numeric numeric = (ValuesSource.Numeric) valuesSourceConfig.getValuesSource();
                 // TODO once composite is plugged in to the values source registry or at least understands Date values source types use it
                 // here
@@ -306,7 +304,6 @@ public static void register(ValuesSourceRegistry.Builder builder) {
                     docValueFormat,
                     order,
                     missingBucket,
-                    missingOrder,
                     hasScript,
                     (
                         BigArrays bigArrays,
@@ -322,7 +319,6 @@ public static void register(ValuesSourceRegistry.Builder builder) {
                             roundingValuesSource::round,
                             compositeValuesSourceConfig.format(),
                             compositeValuesSourceConfig.missingBucket(),
-                            compositeValuesSourceConfig.missingOrder(),
                             size,
                             compositeValuesSourceConfig.reverseMul()
                         );
@@ -343,6 +339,6 @@ protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardCon
         Rounding rounding = dateHistogramInterval.createRounding(timeZone(), offset);
         return queryShardContext.getValuesSourceRegistry()
             .getAggregator(REGISTRY_KEY, config)
-            .apply(config, rounding, name, config.script() != null, format(), missingBucket(), missingOrder(), order());
+            .apply(config, rounding, name, config.script() != null, format(), missingBucket(), order());
     }
 }