diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java index 3477c203d4887..44a29e2251b13 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java @@ -47,8 +47,10 @@ 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; /** @@ -68,10 +70,11 @@ class BinaryValuesSource extends SingleDimensionValuesSource { CheckedFunction docValuesFunc, DocValueFormat format, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { - super(bigArrays, format, fieldType, missingBucket, size, reverseMul); + super(bigArrays, format, fieldType, missingBucket, missingOrder, size, reverseMul); this.breakerConsumer = breakerConsumer; this.docValuesFunc = docValuesFunc; this.values = bigArrays.newObjectArray(Math.min(size, 100)); @@ -101,10 +104,9 @@ void copyCurrent(int slot) { @Override int compare(int from, int to) { if (missingBucket) { - if (values.get(from) == null) { - return values.get(to) == null ? 0 : -1 * reverseMul; - } else if (values.get(to) == null) { - return reverseMul; + int result = missingOrder.compare(() -> Objects.isNull(values.get(from)), () -> Objects.isNull(values.get(to)), reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(values.get(from), values.get(to)); @@ -113,10 +115,9 @@ int compare(int from, int to) { @Override int compareCurrent(int slot) { if (missingBucket) { - if (currentValue == null) { - return values.get(slot) == null ? 0 : -1 * reverseMul; - } else if (values.get(slot) == null) { - return reverseMul; + int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(values.get(slot)), reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, values.get(slot)); @@ -125,10 +126,9 @@ int compareCurrent(int slot) { @Override int compareCurrentWithAfter() { if (missingBucket) { - if (currentValue == null) { - return afterValue == null ? 0 : -1 * reverseMul; - } else if (afterValue == null) { - return reverseMul; + int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(afterValue), reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, afterValue); diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 7fc90f6c6d373..6d047197b38a4 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -32,6 +32,7 @@ package org.opensearch.search.aggregations.bucket.composite; +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; @@ -39,6 +40,7 @@ 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; @@ -49,6 +51,8 @@ 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} */ @@ -59,6 +63,7 @@ public abstract class CompositeValuesSourceBuilder 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; @@ -83,6 +85,7 @@ SingleDimensionValuesSource createValuesSource( DocValueFormat format, SortOrder order, boolean missingBucket, + MissingOrder missingOrder, boolean hasScript, SingleDimensionValuesSourceProvider singleDimensionValuesSourceProvider ) { @@ -94,6 +97,7 @@ SingleDimensionValuesSource createValuesSource( this.missingBucket = missingBucket; this.hasScript = hasScript; this.singleDimensionValuesSourceProvider = singleDimensionValuesSourceProvider; + this.missingOrder = missingOrder; } /** @@ -132,6 +136,13 @@ 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/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java index 888b16b348a14..c1e64f57c10cc 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java @@ -43,6 +43,7 @@ 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; @@ -54,6 +55,7 @@ public class CompositeValuesSourceParserHelper { static , T> void declareValuesSourceFields(AbstractObjectParser 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/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java index 51c014cf71ec1..acab1790bfa21 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java @@ -52,6 +52,7 @@ 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; @@ -81,6 +82,7 @@ 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 ); } @@ -288,7 +290,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, order) -> { + (valuesSourceConfig, rounding, name, hasScript, format, missingBucket, missingOrder, 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 @@ -304,6 +306,7 @@ public static void register(ValuesSourceRegistry.Builder builder) { docValueFormat, order, missingBucket, + missingOrder, hasScript, ( BigArrays bigArrays, @@ -319,6 +322,7 @@ public static void register(ValuesSourceRegistry.Builder builder) { roundingValuesSource::round, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -339,6 +343,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(), order()); + .apply(config, rounding, name, config.script() != null, format(), missingBucket(), missingOrder(), order()); } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java index 0af5c2a901093..fb94ff194b950 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java @@ -44,6 +44,7 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.LeafBucketCollector; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import java.io.IOException; @@ -63,10 +64,11 @@ class DoubleValuesSource extends SingleDimensionValuesSource { CheckedFunction docValuesFunc, DocValueFormat format, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { - super(bigArrays, format, fieldType, missingBucket, size, reverseMul); + super(bigArrays, format, fieldType, missingBucket, missingOrder, size, reverseMul); this.docValuesFunc = docValuesFunc; this.bits = missingBucket ? new BitArray(100, bigArrays) : null; this.values = bigArrays.newDoubleArray(Math.min(size, 100), false); @@ -89,10 +91,9 @@ void copyCurrent(int slot) { @Override int compare(int from, int to) { if (missingBucket) { - if (bits.get(from) == false) { - return bits.get(to) ? -1 * reverseMul : 0; - } else if (bits.get(to) == false) { - return reverseMul; + int result = missingOrder.compare(() -> bits.get(from) == false, () -> bits.get(to) == false, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(values.get(from), values.get(to)); @@ -101,10 +102,9 @@ int compare(int from, int to) { @Override int compareCurrent(int slot) { if (missingBucket) { - if (missingCurrentValue) { - return bits.get(slot) ? -1 * reverseMul : 0; - } else if (bits.get(slot) == false) { - return reverseMul; + int result = missingOrder.compare(() -> missingCurrentValue, () -> bits.get(slot) == false, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, values.get(slot)); @@ -113,10 +113,9 @@ int compareCurrent(int slot) { @Override int compareCurrentWithAfter() { if (missingBucket) { - if (missingCurrentValue) { - return afterValue != null ? -1 * reverseMul : 0; - } else if (afterValue == null) { - return reverseMul; + int result = missingOrder.compare(() -> missingCurrentValue, () -> afterValue == null, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, afterValue); diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index b4049568af4a8..5d47a44a26222 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -49,6 +49,7 @@ import org.opensearch.search.aggregations.bucket.geogrid.CellIdSource; import org.opensearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.opensearch.search.aggregations.bucket.geogrid.GeoTileUtils; +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; @@ -72,6 +73,7 @@ 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 ); } @@ -103,7 +105,7 @@ static void register(ValuesSourceRegistry.Builder builder) { builder.register( REGISTRY_KEY, CoreValuesSourceType.GEOPOINT, - (valuesSourceConfig, precision, boundingBox, name, hasScript, format, missingBucket, order) -> { + (valuesSourceConfig, precision, boundingBox, name, hasScript, format, missingBucket, missingOrder, order) -> { ValuesSource.GeoPoint geoPoint = (ValuesSource.GeoPoint) valuesSourceConfig.getValuesSource(); // is specified in the builder. final MappedFieldType fieldType = valuesSourceConfig.fieldType(); @@ -115,6 +117,7 @@ static void register(ValuesSourceRegistry.Builder builder) { DocValueFormat.GEOTILE, order, missingBucket, + missingOrder, hasScript, ( BigArrays bigArrays, @@ -132,6 +135,7 @@ static void register(ValuesSourceRegistry.Builder builder) { LongUnaryOperator.identity(), compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -220,7 +224,7 @@ protected ValuesSourceType getDefaultValuesSourceType() { protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { return queryShardContext.getValuesSourceRegistry() .getAggregator(REGISTRY_KEY, config) - .apply(config, precision, geoBoundingBox(), name, script() != null, format(), missingBucket(), order()); + .apply(config, precision, geoBoundingBox(), name, script() != null, format(), missingBucket(), missingOrder(), order()); } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileValuesSource.java index 7ad38ec1a3b38..2daddc9647f44 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GeoTileValuesSource.java @@ -39,6 +39,7 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.bucket.geogrid.GeoTileUtils; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import java.io.IOException; import java.util.function.LongUnaryOperator; @@ -57,10 +58,11 @@ class GeoTileValuesSource extends LongValuesSource { LongUnaryOperator rounding, DocValueFormat format, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { - super(bigArrays, fieldType, docValuesFunc, rounding, format, missingBucket, size, reverseMul); + super(bigArrays, fieldType, docValuesFunc, rounding, format, missingBucket, missingOrder, size, reverseMul); } @Override diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java index 8c000f7a6190c..b4db4d8dd2a36 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java @@ -46,6 +46,7 @@ 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; @@ -71,10 +72,11 @@ class GlobalOrdinalValuesSource extends SingleDimensionValuesSource { CheckedFunction docValuesFunc, DocValueFormat format, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { - super(bigArrays, format, type, missingBucket, size, reverseMul); + super(bigArrays, format, type, missingBucket, missingOrder, size, reverseMul); this.docValuesFunc = docValuesFunc; this.values = bigArrays.newLongArray(Math.min(size, 100), false); } @@ -87,16 +89,34 @@ void copyCurrent(int slot) { @Override int compare(int from, int to) { + if (missingBucket) { + int result = missingOrder.compare(() -> values.get(from) == -1, () -> values.get(to) == -1, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; + } + } return Long.compare(values.get(from), values.get(to)) * reverseMul; } @Override int compareCurrent(int slot) { + if (missingBucket) { + int result = missingOrder.compare(() -> currentValue == -1, () -> values.get(slot) == -1, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; + } + } return Long.compare(currentValue, values.get(slot)) * reverseMul; } @Override int compareCurrentWithAfter() { + if (missingBucket) { + int result = missingOrder.compare(() -> currentValue == -1, () -> afterValueGlobalOrd == -1, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; + } + } int cmp = Long.compare(currentValue, afterValueGlobalOrd); if (cmp == 0 && isTopValueInsertionPoint) { // the top value is missing in this shard, the comparison is against diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java index f30fde164f9c4..1f1ed3b6254ba 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java @@ -42,6 +42,7 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.query.QueryShardContext; 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; @@ -67,6 +68,7 @@ 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 ); } @@ -92,7 +94,7 @@ public static void register(ValuesSourceRegistry.Builder builder) { builder.register( REGISTRY_KEY, org.opensearch.common.collect.List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC), - (valuesSourceConfig, interval, name, hasScript, format, missingBucket, order) -> { + (valuesSourceConfig, interval, name, hasScript, format, missingBucket, missingOrder, order) -> { ValuesSource.Numeric numeric = (ValuesSource.Numeric) valuesSourceConfig.getValuesSource(); final HistogramValuesSource vs = new HistogramValuesSource(numeric, interval); final MappedFieldType fieldType = valuesSourceConfig.fieldType(); @@ -103,6 +105,7 @@ public static void register(ValuesSourceRegistry.Builder builder) { valuesSourceConfig.format(), order, missingBucket, + missingOrder, hasScript, ( BigArrays bigArrays, @@ -117,6 +120,7 @@ public static void register(ValuesSourceRegistry.Builder builder) { numericValuesSource::doubleValues, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -194,6 +198,6 @@ protected ValuesSourceType getDefaultValuesSourceType() { protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { return queryShardContext.getValuesSourceRegistry() .getAggregator(REGISTRY_KEY, config) - .apply(config, interval, name, script() != null, format(), missingBucket(), order()); + .apply(config, interval, name, script() != null, format(), missingBucket(), missingOrder(), order()); } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java index e964283e2e362..0ce147a138a96 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java @@ -54,8 +54,10 @@ import org.opensearch.index.mapper.NumberFieldMapper; 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.LongUnaryOperator; import java.util.function.ToLongFunction; @@ -79,10 +81,11 @@ class LongValuesSource extends SingleDimensionValuesSource { LongUnaryOperator rounding, DocValueFormat format, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { - super(bigArrays, format, fieldType, missingBucket, size, reverseMul); + super(bigArrays, format, fieldType, missingBucket, missingOrder, size, reverseMul); this.bigArrays = bigArrays; this.docValuesFunc = docValuesFunc; this.rounding = rounding; @@ -107,10 +110,9 @@ void copyCurrent(int slot) { @Override int compare(int from, int to) { if (missingBucket) { - if (bits.get(from) == false) { - return bits.get(to) ? -1 * reverseMul : 0; - } else if (bits.get(to) == false) { - return reverseMul; + int result = missingOrder.compare(() -> bits.get(from) == false, () -> bits.get(to) == false, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(values.get(from), values.get(to)); @@ -119,10 +121,9 @@ int compare(int from, int to) { @Override int compareCurrent(int slot) { if (missingBucket) { - if (missingCurrentValue) { - return bits.get(slot) ? -1 * reverseMul : 0; - } else if (bits.get(slot) == false) { - return reverseMul; + int result = missingOrder.compare(() -> missingCurrentValue, () -> bits.get(slot) == false, reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, values.get(slot)); @@ -131,10 +132,9 @@ int compareCurrent(int slot) { @Override int compareCurrentWithAfter() { if (missingBucket) { - if (missingCurrentValue) { - return afterValue != null ? -1 * reverseMul : 0; - } else if (afterValue == null) { - return reverseMul; + int result = missingOrder.compare(() -> missingCurrentValue, () -> Objects.isNull(afterValue), reverseMul); + if (MissingOrder.unknownOrder(result) == false) { + return result; } } return compareValues(currentValue, afterValue); diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java index faa94963ae5c9..94b108e863e3d 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java @@ -41,10 +41,13 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.LeafBucketCollector; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import java.io.IOException; +import static org.opensearch.search.aggregations.bucket.missing.MissingOrder.LAST; + /** * A source that can record and compare values of similar type. */ @@ -54,6 +57,7 @@ abstract class SingleDimensionValuesSource> implements R @Nullable protected final MappedFieldType fieldType; protected final boolean missingBucket; + protected final MissingOrder missingOrder; protected final int size; protected final int reverseMul; @@ -67,6 +71,7 @@ abstract class SingleDimensionValuesSource> implements R * @param format The format of the source. * @param fieldType The field type or null if the source is a script. * @param missingBucket If true, an explicit `null bucket represents documents with missing values. + * @param missingOrder The `null bucket's position. * @param size The number of values to record. * @param reverseMul -1 if the natural order ({@link SortOrder#ASC} should be reversed. */ @@ -75,6 +80,7 @@ abstract class SingleDimensionValuesSource> implements R DocValueFormat format, @Nullable MappedFieldType fieldType, boolean missingBucket, + MissingOrder missingOrder, int size, int reverseMul ) { @@ -82,6 +88,7 @@ abstract class SingleDimensionValuesSource> implements R this.format = format; this.fieldType = fieldType; this.missingBucket = missingBucket; + this.missingOrder = missingOrder; this.size = size; this.reverseMul = reverseMul; this.afterValue = null; @@ -167,8 +174,11 @@ T getAfter() { * Returns true if a {@link SortedDocsProducer} should be used to optimize the execution. */ protected boolean checkIfSortedDocsIsApplicable(IndexReader reader, MappedFieldType fieldType) { - if (fieldType == null || (missingBucket && afterValue == null) || fieldType.isSearchable() == false || - // inverse of the natural order + if (fieldType == null + || (missingBucket && (afterValue == null || reverseMul == 1 && missingOrder == LAST)) + || fieldType.isSearchable() == false + || + // inverse of the natural order reverseMul == -1) { return false; } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java index d70b9dece82c0..c871e8d0c7d5b 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java @@ -43,6 +43,7 @@ import org.opensearch.index.query.QueryShardContext; import org.opensearch.script.Script; import org.opensearch.search.DocValueFormat; +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; @@ -68,6 +69,7 @@ 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 ); } @@ -111,7 +113,7 @@ static void register(ValuesSourceRegistry.Builder builder) { builder.register( REGISTRY_KEY, org.opensearch.common.collect.List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC, CoreValuesSourceType.BOOLEAN), - (valuesSourceConfig, name, hasScript, format, missingBucket, order) -> { + (valuesSourceConfig, name, hasScript, format, missingBucket, missingOrder, order) -> { final DocValueFormat docValueFormat; if (format == null && valuesSourceConfig.valueSourceType() == CoreValuesSourceType.DATE) { // defaults to the raw format on date fields (preserve timestamp as longs). @@ -126,6 +128,7 @@ static void register(ValuesSourceRegistry.Builder builder) { docValueFormat, order, missingBucket, + missingOrder, hasScript, ( BigArrays bigArrays, @@ -142,6 +145,7 @@ static void register(ValuesSourceRegistry.Builder builder) { vs::doubleValues, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -156,6 +160,7 @@ static void register(ValuesSourceRegistry.Builder builder) { rounding, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -170,13 +175,14 @@ static void register(ValuesSourceRegistry.Builder builder) { builder.register( REGISTRY_KEY, org.opensearch.common.collect.List.of(CoreValuesSourceType.BYTES, CoreValuesSourceType.IP), - (valuesSourceConfig, name, hasScript, format, missingBucket, order) -> new CompositeValuesSourceConfig( + (valuesSourceConfig, name, hasScript, format, missingBucket, missingOrder, order) -> new CompositeValuesSourceConfig( name, valuesSourceConfig.fieldType(), valuesSourceConfig.getValuesSource(), valuesSourceConfig.format(), order, missingBucket, + missingOrder, hasScript, ( BigArrays bigArrays, @@ -193,6 +199,7 @@ static void register(ValuesSourceRegistry.Builder builder) { vs::globalOrdinalsValues, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -205,6 +212,7 @@ static void register(ValuesSourceRegistry.Builder builder) { vs::bytesValues, compositeValuesSourceConfig.format(), compositeValuesSourceConfig.missingBucket(), + compositeValuesSourceConfig.missingOrder(), size, compositeValuesSourceConfig.reverseMul() ); @@ -224,6 +232,6 @@ protected ValuesSourceType getDefaultValuesSourceType() { protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { return queryShardContext.getValuesSourceRegistry() .getAggregator(REGISTRY_KEY, config) - .apply(config, name, script() != null, format(), missingBucket(), order()); + .apply(config, name, script() != null, format(), missingBucket(), missingOrder(), order()); } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/missing/MissingOrder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/missing/MissingOrder.java new file mode 100644 index 0000000000000..06086f507f665 --- /dev/null +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/missing/MissingOrder.java @@ -0,0 +1,109 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.search.aggregations.bucket.missing; + +import org.opensearch.common.inject.Provider; +import org.opensearch.common.io.stream.StreamInput; +import org.opensearch.common.io.stream.StreamOutput; +import org.opensearch.common.io.stream.Writeable; + +import java.io.IOException; +import java.util.Locale; + +/** + * Composite Aggregation Missing bucket order. + */ +public enum MissingOrder implements Writeable { + /** + * missing first. + */ + FIRST { + @Override + public int compare(Provider leftIsMissing, Provider rightIsMissing, int reverseMul) { + if (leftIsMissing.get()) { + return rightIsMissing.get() ? 0 : -1; + } else if (rightIsMissing.get()) { + return 1; + } + return MISSING_ORDER_UNKNOWN; + } + + @Override + public String toString() { + return "first"; + } + }, + + /** + * missing last. + */ + LAST { + @Override + public int compare(Provider leftIsMissing, Provider rightIsMissing, int reverseMul) { + if (leftIsMissing.get()) { + return rightIsMissing.get() ? 0 : 1; + } else if (rightIsMissing.get()) { + return -1; + } + return MISSING_ORDER_UNKNOWN; + } + + @Override + public String toString() { + return "last"; + } + }, + + /** + * Default: ASC missing first / DESC missing last + */ + DEFAULT { + @Override + public int compare(Provider leftIsMissing, Provider rightIsMissing, int reverseMul) { + if (leftIsMissing.get()) { + return rightIsMissing.get() ? 0 : -1 * reverseMul; + } else if (rightIsMissing.get()) { + return reverseMul; + } + return MISSING_ORDER_UNKNOWN; + } + + @Override + public String toString() { + return "default"; + } + }; + + public static final String NAME = "missing_order"; + + private static int MISSING_ORDER_UNKNOWN = Integer.MIN_VALUE; + + public static MissingOrder readFromStream(StreamInput in) throws IOException { + return in.readEnum(MissingOrder.class); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeEnum(this); + } + + public static boolean isDefault(MissingOrder order) { + return order == DEFAULT; + } + + public static MissingOrder fromString(String order) { + return valueOf(order.toUpperCase(Locale.ROOT)); + } + + public static boolean unknownOrder(int v) { + return v == MISSING_ORDER_UNKNOWN; + } + + public abstract int compare(Provider leftIsMissing, Provider rightIsMissing, int reverseMul); +} diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java index 4316acc8c2b85..c4a87f3993bb4 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java @@ -37,6 +37,7 @@ import org.opensearch.search.aggregations.BaseAggregationTestCase; import org.opensearch.search.aggregations.bucket.geogrid.GeoTileUtils; import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import java.util.ArrayList; @@ -69,6 +70,7 @@ private DateHistogramValuesSourceBuilder randomDateHistogramSourceBuilder() { if (randomBoolean()) { histo.missingBucket(true); } + histo.missingOrder(randomFrom(MissingOrder.values())); return histo; } @@ -94,6 +96,7 @@ private TermsValuesSourceBuilder randomTermsSourceBuilder() { if (randomBoolean()) { terms.missingBucket(true); } + terms.missingOrder(randomFrom(MissingOrder.values())); return terms; } @@ -108,6 +111,7 @@ private HistogramValuesSourceBuilder randomHistogramSourceBuilder() { histo.missingBucket(true); } histo.interval(randomDoubleBetween(Math.nextUp(0), Double.MAX_VALUE, false)); + histo.missingOrder(randomFrom(MissingOrder.values())); return histo; } diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java index e8df1753a79b8..f81bd012bfa63 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java @@ -82,6 +82,7 @@ import org.opensearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.opensearch.search.aggregations.bucket.geogrid.GeoTileUtils; import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.aggregations.bucket.terms.StringTerms; import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.opensearch.search.aggregations.metrics.InternalMax; @@ -586,6 +587,84 @@ public void testWithKeywordAndMissingBucket() throws Exception { assertEquals(0, result.getBuckets().size()); assertNull(result.afterKey()); }); + + // sort ascending, null bucket is first, same as default. + testSearchCase(Arrays.asList(new MatchAllDocsQuery()), dataset, () -> { + TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword").field("keyword") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST); + return new CompositeAggregationBuilder("name", Collections.singletonList(terms)); + }, (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{keyword=d}", result.afterKey().toString()); + assertEquals("{keyword=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=a}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=c}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + assertEquals("{keyword=d}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(3).getDocCount()); + }); + + // sort ascending, null bucket is last. + testSearchCase(Arrays.asList(new MatchAllDocsQuery()), dataset, () -> { + TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword").field("keyword") + .missingBucket(true) + .missingOrder(MissingOrder.LAST); + return new CompositeAggregationBuilder("name", Collections.singletonList(terms)); + }, (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{keyword=null}", result.afterKey().toString()); + assertEquals("{keyword=a}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=c}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=d}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(2).getDocCount()); + assertEquals("{keyword=null}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(3).getDocCount()); + }); + + // sort descending, null bucket is last, same as default + testSearchCase(Arrays.asList(new MatchAllDocsQuery()), dataset, () -> { + TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword").field("keyword") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .order(SortOrder.DESC); + return new CompositeAggregationBuilder("name", Collections.singletonList(terms)); + }, (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{keyword=null}", result.afterKey().toString()); + assertEquals("{keyword=null}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(3).getDocCount()); + assertEquals("{keyword=a}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + assertEquals("{keyword=c}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=d}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + }); + + // sort descending, null bucket is first + testSearchCase(Arrays.asList(new MatchAllDocsQuery()), dataset, () -> { + TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword").field("keyword") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .order(SortOrder.DESC); + return new CompositeAggregationBuilder("name", Collections.singletonList(terms)); + }, (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{keyword=a}", result.afterKey().toString()); + assertEquals("{keyword=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=d}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=c}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + assertEquals("{keyword=a}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(3).getDocCount()); + }); } public void testWithKeywordMissingAfter() throws Exception { @@ -901,14 +980,14 @@ public void testWithKeywordLongAndMissingBucket() throws Exception { final List>> dataset = new ArrayList<>(); dataset.addAll( Arrays.asList( - createDocument("keyword", "a", "long", 100L), + createDocument("double", 0d, "keyword", "a", "long", 100L), createDocument("double", 0d), - createDocument("keyword", "c", "long", 100L), - createDocument("keyword", "a", "long", 0L), - createDocument("keyword", "d", "long", 10L), - createDocument("keyword", "c"), - createDocument("keyword", "c", "long", 100L), - createDocument("long", 100L), + createDocument("double", 0d, "keyword", "c", "long", 100L), + createDocument("double", 0d, "keyword", "a", "long", 0L), + createDocument("double", 0d, "keyword", "d", "long", 10L), + createDocument("double", 0d, "keyword", "c"), + createDocument("double", 0d, "keyword", "c", "long", 100L), + createDocument("double", 0d, "long", 100L), createDocument("double", 0d) ) ); @@ -961,6 +1040,112 @@ public void testWithKeywordLongAndMissingBucket() throws Exception { assertEquals(1L, result.getBuckets().get(1).getDocCount()); } ); + + // keyword null bucket is last, long null bucket is last + testSearchCase( + Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("double")), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword").missingBucket(true).missingOrder(MissingOrder.LAST), + new TermsValuesSourceBuilder("long").field("long").missingBucket(true).missingOrder(MissingOrder.LAST) + ) + ), + (result) -> { + assertEquals(7, result.getBuckets().size()); + assertEquals("{keyword=null, long=null}", result.afterKey().toString()); + assertEquals("{keyword=null, long=null}", result.getBuckets().get(6).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(6).getDocCount()); + assertEquals("{keyword=null, long=100}", result.getBuckets().get(5).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(5).getDocCount()); + } + ); + + // keyword null bucket is last, long null bucket is first + testSearchCase( + Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("double")), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword").missingBucket(true).missingOrder(MissingOrder.LAST), + new TermsValuesSourceBuilder("long").field("long").missingBucket(true).missingOrder(MissingOrder.FIRST) + ) + ), + (result) -> { + assertEquals(7, result.getBuckets().size()); + assertEquals("{keyword=null, long=100}", result.afterKey().toString()); + assertEquals("{keyword=null, long=100}", result.getBuckets().get(6).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(6).getDocCount()); + assertEquals("{keyword=null, long=null}", result.getBuckets().get(5).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(5).getDocCount()); + } + ); + + // asc, null bucket is last, search after non null value + testSearchCase( + Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("double")), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList(new TermsValuesSourceBuilder("keyword").field("keyword").missingBucket(true).missingOrder(MissingOrder.LAST)) + ).aggregateAfter(createAfterKey("keyword", "c")), + (result) -> { + assertEquals(2, result.getBuckets().size()); + assertEquals("{keyword=null}", result.afterKey().toString()); + assertEquals("{keyword=d}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=null}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(3L, result.getBuckets().get(1).getDocCount()); + } + ); + + // desc, null bucket is last, search after non null value + testSearchCase( + Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("double")), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .order(SortOrder.DESC) + ) + ).aggregateAfter(createAfterKey("keyword", "c")), + (result) -> { + assertEquals(2, result.getBuckets().size()); + assertEquals("{keyword=null}", result.afterKey().toString()); + assertEquals("{keyword=a}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=null}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(3L, result.getBuckets().get(1).getDocCount()); + } + ); + + // keyword null bucket is last, long null bucket is last + testSearchCase( + Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery("double")), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new TermsValuesSourceBuilder("keyword").field("keyword").missingBucket(true).missingOrder(MissingOrder.LAST), + new TermsValuesSourceBuilder("long").field("long").missingBucket(true).missingOrder(MissingOrder.LAST) + ) + ).aggregateAfter(createAfterKey("keyword", "c", "long", null)), + (result) -> { + assertEquals(3, result.getBuckets().size()); + assertEquals("{keyword=null, long=null}", result.afterKey().toString()); + assertEquals("{keyword=d, long=10}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{keyword=null, long=100}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(1).getDocCount()); + assertEquals("{keyword=null, long=null}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + } + ); } public void testMultiValuedWithKeywordAndLong() throws Exception { @@ -1719,6 +1904,240 @@ public void testWithHistogramAndKeyword() throws IOException { ); } + public void testWithHistogramBucketMissing() throws IOException { + final List>> dataset = new ArrayList<>(); + dataset.addAll( + Arrays.asList( + createDocument("price", 50L, "long", 1L), + createDocument("price", 60L, "long", 2L), + createDocument("price", 70L, "long", 3L), + createDocument("price", 62L, "long", 4L), + createDocument("long", 5L) + ) + ); + + // asc, null bucket is first + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .interval(10) + ) + ), + (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{price=70.0}", result.afterKey().toString()); + assertEquals("{price=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{price=50.0}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(1).getDocCount()); + assertEquals("{price=60.0}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + assertEquals("{price=70.0}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(3).getDocCount()); + } + ); + + // asc, null bucket is first, after 50.0 + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .interval(10) + ) + ).aggregateAfter(createAfterKey("price", 60.0d)), + (result) -> { + assertEquals(1, result.getBuckets().size()); + assertEquals("{price=70.0}", result.afterKey().toString()); + assertEquals("{price=70.0}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + } + ); + + // asc, null bucket is first, after null + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .interval(10) + ) + ).aggregateAfter(createAfterKey("price", null)), + (result) -> { + assertEquals(3, result.getBuckets().size()); + assertEquals("{price=70.0}", result.afterKey().toString()); + assertEquals("{price=50.0}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{price=60.0}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{price=70.0}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(2).getDocCount()); + } + ); + + // asc, null bucket is last + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .interval(10) + ) + ), + (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{price=null}", result.afterKey().toString()); + assertEquals("{price=50.0}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{price=60.0}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{price=70.0}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(2).getDocCount()); + assertEquals("{price=null}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(3).getDocCount()); + } + ); + + // asc, null bucket is last, after 70.0 + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .interval(10) + ) + ).aggregateAfter(createAfterKey("price", 70.0)), + (result) -> { + assertEquals(1, result.getBuckets().size()); + assertEquals("{price=null}", result.afterKey().toString()); + assertEquals("{price=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + } + ); + + // desc, null bucket is first + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .order(SortOrder.DESC) + .interval(10) + ) + ), + (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{price=50.0}", result.afterKey().toString()); + assertEquals("{price=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{price=70.0}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(1).getDocCount()); + assertEquals("{price=60.0}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(2).getDocCount()); + assertEquals("{price=50.0}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(3).getDocCount()); + } + ); + + // desc, null bucket is first, after 60.0 + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.FIRST) + .order(SortOrder.DESC) + .interval(10) + ) + ).aggregateAfter(createAfterKey("price", 60.0)), + (result) -> { + assertEquals(1, result.getBuckets().size()); + assertEquals("{price=50.0}", result.afterKey().toString()); + assertEquals("{price=50.0}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + } + ); + + // desc, null bucket is last + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .order(SortOrder.DESC) + .interval(10) + ) + ), + (result) -> { + assertEquals(4, result.getBuckets().size()); + assertEquals("{price=null}", result.afterKey().toString()); + assertEquals("{price=70.0}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + assertEquals("{price=60.0}", result.getBuckets().get(1).getKeyAsString()); + assertEquals(2L, result.getBuckets().get(1).getDocCount()); + assertEquals("{price=50.0}", result.getBuckets().get(2).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(2).getDocCount()); + assertEquals("{price=null}", result.getBuckets().get(3).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(3).getDocCount()); + } + ); + + // desc, null bucket is last, after 50.0 + testSearchCase( + Arrays.asList(new MatchAllDocsQuery()), + dataset, + () -> new CompositeAggregationBuilder( + "name", + Arrays.asList( + new HistogramValuesSourceBuilder("price").field("price") + .missingBucket(true) + .missingOrder(MissingOrder.LAST) + .order(SortOrder.DESC) + .interval(10) + ) + ).aggregateAfter(createAfterKey("price", 50.0)), + (result) -> { + assertEquals(1, result.getBuckets().size()); + assertEquals("{price=null}", result.afterKey().toString()); + assertEquals("{price=null}", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); + } + ); + } + public void testWithKeywordAndDateHistogram() throws IOException { final List>> dataset = new ArrayList<>(); dataset.addAll( diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueueTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueueTests.java index a256a37814d62..0ad6d30df337f 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueueTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesCollectorQueueTests.java @@ -63,6 +63,7 @@ import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.AggregatorTestCase; import org.opensearch.search.aggregations.LeafBucketCollector; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import java.io.IOException; import java.util.ArrayList; @@ -277,6 +278,7 @@ private void testRandomCase(boolean forceMerge, boolean missingBucket, int index value -> value, DocValueFormat.RAW, missingBucket, + MissingOrder.DEFAULT, size, 1 ); @@ -287,6 +289,7 @@ private void testRandomCase(boolean forceMerge, boolean missingBucket, int index context -> FieldData.sortableLongBitsToDoubles(DocValues.getSortedNumeric(context.reader(), fieldType.name())), DocValueFormat.RAW, missingBucket, + MissingOrder.DEFAULT, size, 1 ); @@ -300,6 +303,7 @@ private void testRandomCase(boolean forceMerge, boolean missingBucket, int index context -> DocValues.getSortedSet(context.reader(), fieldType.name()), DocValueFormat.RAW, missingBucket, + MissingOrder.DEFAULT, size, 1 ); @@ -311,6 +315,7 @@ private void testRandomCase(boolean forceMerge, boolean missingBucket, int index context -> FieldData.toString(DocValues.getSortedSet(context.reader(), fieldType.name())), DocValueFormat.RAW, missingBucket, + MissingOrder.DEFAULT, size, 1 ); diff --git a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSourceTests.java b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSourceTests.java index 19cf1ef7044c9..6569a269169eb 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSourceTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/bucket/composite/SingleDimensionValuesSourceTests.java @@ -47,6 +47,7 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.search.DocValueFormat; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.test.OpenSearchTestCase; import static org.mockito.Mockito.mock; @@ -62,6 +63,7 @@ public void testBinarySorted() { context -> null, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, 1 ); @@ -79,6 +81,7 @@ public void testBinarySorted() { context -> null, DocValueFormat.RAW, true, + MissingOrder.DEFAULT, 1, 1 ); @@ -92,13 +95,24 @@ public void testBinarySorted() { context -> null, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 0, -1 ); assertNull(source.createSortedDocsProducerOrNull(reader, null)); MappedFieldType ip = new IpFieldMapper.IpFieldType("ip"); - source = new BinaryValuesSource(BigArrays.NON_RECYCLING_INSTANCE, (b) -> {}, ip, context -> null, DocValueFormat.RAW, false, 1, 1); + source = new BinaryValuesSource( + BigArrays.NON_RECYCLING_INSTANCE, + (b) -> {}, + ip, + context -> null, + DocValueFormat.RAW, + false, + MissingOrder.DEFAULT, + 1, + 1 + ); assertNull(source.createSortedDocsProducerOrNull(reader, null)); } @@ -110,6 +124,7 @@ public void testGlobalOrdinalsSorted() { context -> null, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, 1 ); @@ -120,7 +135,16 @@ public void testGlobalOrdinalsSorted() { assertNull(source.createSortedDocsProducerOrNull(reader, new TermQuery(new Term("foo", "bar")))); assertNull(source.createSortedDocsProducerOrNull(reader, new TermQuery(new Term("keyword", "toto)")))); - source = new GlobalOrdinalValuesSource(BigArrays.NON_RECYCLING_INSTANCE, keyword, context -> null, DocValueFormat.RAW, true, 1, 1); + source = new GlobalOrdinalValuesSource( + BigArrays.NON_RECYCLING_INSTANCE, + keyword, + context -> null, + DocValueFormat.RAW, + true, + MissingOrder.DEFAULT, + 1, + 1 + ); assertNull(source.createSortedDocsProducerOrNull(reader, new MatchAllDocsQuery())); assertNull(source.createSortedDocsProducerOrNull(reader, null)); assertNull(source.createSortedDocsProducerOrNull(reader, new TermQuery(new Term("foo", "bar")))); @@ -131,6 +155,7 @@ public void testGlobalOrdinalsSorted() { context -> null, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, -1 ); @@ -138,7 +163,16 @@ public void testGlobalOrdinalsSorted() { assertNull(source.createSortedDocsProducerOrNull(reader, new TermQuery(new Term("foo", "bar")))); final MappedFieldType ip = new IpFieldMapper.IpFieldType("ip"); - source = new GlobalOrdinalValuesSource(BigArrays.NON_RECYCLING_INSTANCE, ip, context -> null, DocValueFormat.RAW, false, 1, 1); + source = new GlobalOrdinalValuesSource( + BigArrays.NON_RECYCLING_INSTANCE, + ip, + context -> null, + DocValueFormat.RAW, + false, + MissingOrder.DEFAULT, + 1, + 1 + ); assertNull(source.createSortedDocsProducerOrNull(reader, null)); assertNull(source.createSortedDocsProducerOrNull(reader, new TermQuery(new Term("foo", "bar")))); } @@ -159,6 +193,7 @@ public void testNumericSorted() { value -> value, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, 1 ); @@ -192,6 +227,7 @@ public void testNumericSorted() { value -> value, DocValueFormat.RAW, true, + MissingOrder.DEFAULT, 1, 1 ); @@ -213,6 +249,7 @@ public void testNumericSorted() { value -> value, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, -1 ); @@ -231,6 +268,7 @@ public void testNumericSorted() { context -> null, DocValueFormat.RAW, false, + MissingOrder.DEFAULT, 1, 1 );