diff --git a/docs/user/optimization/optimization.rst b/docs/user/optimization/optimization.rst index b168916029..e0fe943560 100644 --- a/docs/user/optimization/optimization.rst +++ b/docs/user/optimization/optimization.rst @@ -287,7 +287,7 @@ The Aggregation operator will merge into OpenSearch Aggregation:: { "name": "OpenSearchIndexScan", "description": { - "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" + "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" }, "children": [] } @@ -313,7 +313,7 @@ The Sort operator will merge into OpenSearch Aggregation.:: { "name": "OpenSearchIndexScan", "description": { - "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"order\":\"desc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" + "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"last\",\"order\":\"desc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" }, "children": [] } @@ -321,41 +321,6 @@ The Sort operator will merge into OpenSearch Aggregation.:: } } -Because the OpenSearch Composite Aggregation order doesn't support separate NULL_FIRST/NULL_LAST option. only the default sort option (ASC NULL_FIRST/DESC NULL_LAST) will be supported for push down to OpenSearch Aggregation, otherwise it will fall back to the default memory based operator:: - - sh$ curl -sS -H 'Content-Type: application/json' \ - ... -X POST localhost:9200/_plugins/_sql/_explain \ - ... -d '{"query" : "SELECT gender, avg(age) FROM accounts GROUP BY gender ORDER BY gender ASC NULLS LAST"}' - { - "root": { - "name": "ProjectOperator", - "description": { - "fields": "[gender, avg(age)]" - }, - "children": [ - { - "name": "SortOperator", - "description": { - "sortList": { - "gender": { - "sortOrder": "ASC", - "nullOrder": "NULL_LAST" - } - } - }, - "children": [ - { - "name": "OpenSearchIndexScan", - "description": { - "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" - }, - "children": [] - } - ] - } - ] - } - } Because the OpenSearch Composite Aggregation doesn't support order by metrics field, then if the sort list include fields which refer to metrics aggregation, then the sort operator can't be push down to OpenSearch Aggregation:: @@ -383,7 +348,7 @@ Because the OpenSearch Composite Aggregation doesn't support order by metrics fi { "name": "OpenSearchIndexScan", "description": { - "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" + "request": "OpenSearchQueryRequest(indexName=accounts, sourceBuilder={\"from\":0,\"size\":0,\"timeout\":\"1m\",\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"gender\":{\"terms\":{\"field\":\"gender.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg(age)\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone=false)" }, "children": [] } @@ -408,4 +373,4 @@ At the moment there is no optimization to merge similar sort operators to avoid Sort Push Down -------------- -Without sort push down optimization, the sort operator will sort the result from child operator. By default, only 200 docs will extracted from the source index, `you can change this value by using size_limit setting <../admin/settings.rst#opensearch-query-size-limit>`_. \ No newline at end of file +Without sort push down optimization, the sort operator will sort the result from child operator. By default, only 200 docs will extracted from the source index, `you can change this value by using size_limit setting <../admin/settings.rst#opensearch-query-size-limit>`_. diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_agg_push.json b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_agg_push.json index 23c79ac077..2d7f5f8c08 100644 --- a/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_agg_push.json +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_agg_push.json @@ -8,7 +8,7 @@ { "name": "OpenSearchIndexScan", "description": { - "request": "OpenSearchQueryRequest(indexName\u003dopensearch-sql_test_index_account, sourceBuilder\u003d{\"from\":0,\"size\":0,\"timeout\":\"1m\",\"query\":{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}],\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"state\":{\"terms\":{\"field\":\"state.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}},{\"city\":{\"terms\":{\"field\":\"city.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}}]},\"aggregations\":{\"avg_age\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone\u003dfalse)" + "request": "OpenSearchQueryRequest(indexName\u003dopensearch-sql_test_index_account, sourceBuilder\u003d{\"from\":0,\"size\":0,\"timeout\":\"1m\",\"query\":{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}],\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"state\":{\"terms\":{\"field\":\"state.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}},{\"city\":{\"terms\":{\"field\":\"city.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg_age\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone\u003dfalse)" }, "children": [] } diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_output.json b/integ-test/src/test/resources/expectedOutput/ppl/explain_output.json index 93a864beda..45988e35c7 100644 --- a/integ-test/src/test/resources/expectedOutput/ppl/explain_output.json +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_output.json @@ -31,7 +31,7 @@ { "name": "OpenSearchIndexScan", "description": { - "request": "OpenSearchQueryRequest(indexName\u003dopensearch-sql_test_index_account, sourceBuilder\u003d{\"from\":0,\"size\":0,\"timeout\":\"1m\",\"query\":{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}],\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"state\":{\"terms\":{\"field\":\"state.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}},{\"city\":{\"terms\":{\"field\":\"city.keyword\",\"missing_bucket\":true,\"order\":\"asc\"}}}]},\"aggregations\":{\"avg_age\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone\u003dfalse)" + "request": "OpenSearchQueryRequest(indexName\u003dopensearch-sql_test_index_account, sourceBuilder\u003d{\"from\":0,\"size\":0,\"timeout\":\"1m\",\"query\":{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}],\"aggregations\":{\"composite_buckets\":{\"composite\":{\"size\":1000,\"sources\":[{\"state\":{\"terms\":{\"field\":\"state.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}},{\"city\":{\"terms\":{\"field\":\"city.keyword\",\"missing_bucket\":true,\"missing_order\":\"first\",\"order\":\"asc\"}}}]},\"aggregations\":{\"avg_age\":{\"avg\":{\"field\":\"age\"}}}}}}, searchDone\u003dfalse)" }, "children": [] } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexAgg.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexAgg.java index d994cac6c3..57dac4dcf1 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexAgg.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/MergeSortAndIndexAgg.java @@ -47,7 +47,6 @@ public MergeSortAndIndexAgg() { this.pattern = typeOf(LogicalSort.class) .matching(OptimizationRuleUtils::sortByFieldsOnly) - .matching(OptimizationRuleUtils::sortByDefaultOptionOnly) .matching(sort -> { sortRef.set(sort); return true; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/OptimizationRuleUtils.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/OptimizationRuleUtils.java index 77f09f39d9..aa1ffa9e4c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/OptimizationRuleUtils.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/logical/rule/OptimizationRuleUtils.java @@ -11,7 +11,6 @@ import java.util.List; import java.util.Set; import lombok.experimental.UtilityClass; -import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.expression.ExpressionNodeVisitor; import org.opensearch.sql.expression.NamedExpression; import org.opensearch.sql.expression.ReferenceExpression; @@ -32,20 +31,6 @@ public static boolean sortByFieldsOnly(LogicalSort logicalSort) { .reduce(true, Boolean::logicalAnd); } - /** - * Does the sort list has option other than default options. - * The default sort options are (ASC NULL_FIRST) or (DESC NULL LAST) - * - * @param logicalSort LogicalSort. - * @return true sort list only option default options, otherwise false. - */ - public static boolean sortByDefaultOptionOnly(LogicalSort logicalSort) { - return logicalSort.getSortList().stream() - .map(sort -> Sort.SortOption.DEFAULT_ASC.equals(sort.getLeft()) - || Sort.SortOption.DEFAULT_DESC.equals(sort.getLeft())) - .reduce(true, Boolean::logicalAnd); - } - /** * Find reference expression from expression. * @param expressions a list of expression. diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java index 3f76799829..ae3239eea0 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java @@ -14,13 +14,14 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.function.Function; import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.apache.commons.lang3.tuple.Pair; +import org.apache.commons.lang3.tuple.Triple; import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.aggregations.AggregationBuilders; import org.opensearch.search.aggregations.AggregatorFactories; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.data.type.ExprType; @@ -92,7 +93,9 @@ public AggregationQueryBuilder( bucketBuilder.build( groupByList.stream() .sorted(groupSortOrder) - .map(expr -> Pair.of(expr, groupSortOrder.apply(expr))) + .map(expr -> Triple.of(expr, + groupSortOrder.sortOrder(expr), + groupSortOrder.missingOrder(expr))) .collect(Collectors.toList()))) .subAggregations(metrics.getLeft()) .size(AGGREGATION_BUCKET_SIZE)), @@ -112,27 +115,37 @@ public Map buildTypeMapping( return builder.build(); } + /** + * Group By field sort order. + */ @VisibleForTesting - public static class GroupSortOrder implements Comparator, - Function { + public static class GroupSortOrder implements Comparator { /** * The default order of group field. * The order is ASC NULL_FIRST. * The field should be the last one in the group list. */ - private static final Pair DEFAULT_ORDER = - Pair.of(SortOrder.ASC, Integer.MAX_VALUE); + private static final Pair DEFAULT_ORDER = + Pair.of(Sort.SortOption.DEFAULT_ASC, Integer.MAX_VALUE); + + /** + * The mapping between {@link Sort.SortOrder} and {@link SortOrder}. + */ + private static final Map SORT_MAP = + new ImmutableMap.Builder() + .put(Sort.SortOrder.ASC, SortOrder.ASC) + .put(Sort.SortOrder.DESC, SortOrder.DESC).build(); /** - * The mapping betwen {@link Sort.SortOption} and {@link SortOrder}. + * The mapping between {@link Sort.NullOrder} and {@link MissingOrder}. */ - private static final Map SORT_MAP = - new ImmutableMap.Builder() - .put(Sort.SortOption.DEFAULT_ASC, SortOrder.ASC) - .put(Sort.SortOption.DEFAULT_DESC, SortOrder.DESC).build(); + private static final Map NULL_MAP = + new ImmutableMap.Builder() + .put(Sort.NullOrder.NULL_FIRST, MissingOrder.FIRST) + .put(Sort.NullOrder.NULL_LAST, MissingOrder.LAST).build(); - private final Map> map = new HashMap<>(); + private final Map> map = new HashMap<>(); /** * Constructor of GroupSortOrder. @@ -144,7 +157,7 @@ public GroupSortOrder(List> sortList) { int pos = 0; for (Pair sortPair : sortList) { map.put(((ReferenceExpression) sortPair.getRight()).getAttr(), - Pair.of(SORT_MAP.getOrDefault(sortPair.getLeft(), SortOrder.ASC), pos++)); + Pair.of(sortPair.getLeft(), pos++)); } } @@ -161,9 +174,9 @@ public GroupSortOrder(List> sortList) { */ @Override public int compare(NamedExpression o1, NamedExpression o2) { - final Pair o1Value = + final Pair o1Value = map.getOrDefault(o1.getName(), DEFAULT_ORDER); - final Pair o2Value = + final Pair o2Value = map.getOrDefault(o2.getName(), DEFAULT_ORDER); return o1Value.getRight().compareTo(o2Value.getRight()); } @@ -172,8 +185,19 @@ public int compare(NamedExpression o1, NamedExpression o2) { * Get the {@link SortOrder} for expression. * By default, the {@link SortOrder} is ASC. */ - @Override - public SortOrder apply(NamedExpression expression) { + public SortOrder sortOrder(NamedExpression expression) { + return SORT_MAP.get(sortOption(expression).getSortOrder()); + } + + /** + * Get the {@link MissingOrder} for expression. + * By default, the {@link MissingOrder} is ASC missing first / DESC missing last. + */ + public MissingOrder missingOrder(NamedExpression expression) { + return NULL_MAP.get(sortOption(expression).getNullOrder()); + } + + private Sort.SortOption sortOption(NamedExpression expression) { return map.getOrDefault(expression.getName(), DEFAULT_ORDER).getLeft(); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java index 1dba5ac836..cf368c940d 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java @@ -7,12 +7,13 @@ import com.google.common.collect.ImmutableList; import java.util.List; -import org.apache.commons.lang3.tuple.Pair; +import org.apache.commons.lang3.tuple.Triple; import org.opensearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder; import org.opensearch.search.aggregations.bucket.composite.DateHistogramValuesSourceBuilder; import org.opensearch.search.aggregations.bucket.composite.HistogramValuesSourceBuilder; import org.opensearch.search.aggregations.bucket.composite.TermsValuesSourceBuilder; import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.ast.expression.SpanUnit; import org.opensearch.sql.expression.NamedExpression; @@ -35,50 +36,56 @@ public BucketAggregationBuilder( * Build the list of CompositeValuesSourceBuilder. */ public List> build( - List> groupList) { + List> groupList) { ImmutableList.Builder> resultBuilder = new ImmutableList.Builder<>(); - for (Pair groupPair : groupList) { + for (Triple groupPair : groupList) { resultBuilder.add( - buildCompositeValuesSourceBuilder(groupPair.getLeft(), groupPair.getRight())); + buildCompositeValuesSourceBuilder(groupPair.getLeft(), + groupPair.getMiddle(), groupPair.getRight())); } return resultBuilder.build(); } // todo, Expression should implement buildCompositeValuesSourceBuilder() interface. private CompositeValuesSourceBuilder buildCompositeValuesSourceBuilder( - NamedExpression expr, SortOrder order) { + NamedExpression expr, SortOrder sortOrder, MissingOrder missingOrder) { if (expr.getDelegated() instanceof SpanExpression) { SpanExpression spanExpr = (SpanExpression) expr.getDelegated(); return buildHistogram( expr.getNameOrAlias(), spanExpr.getField().toString(), spanExpr.getValue().valueOf(null).doubleValue(), - spanExpr.getUnit()); + spanExpr.getUnit(), + missingOrder); } else { CompositeValuesSourceBuilder sourceBuilder = - new TermsValuesSourceBuilder(expr.getNameOrAlias()).missingBucket(true).order(order); + new TermsValuesSourceBuilder(expr.getNameOrAlias()) + .missingBucket(true) + .missingOrder(missingOrder) + .order(sortOrder); return helper.build(expr.getDelegated(), sourceBuilder::field, sourceBuilder::script); } } private CompositeValuesSourceBuilder buildHistogram( - String name, String field, Double value, SpanUnit unit) { + String name, String field, Double value, SpanUnit unit, MissingOrder missingOrder) { switch (unit) { case NONE: return new HistogramValuesSourceBuilder(name) .field(field) .interval(value) - .missingBucket(true); + .missingBucket(true) + .missingOrder(missingOrder); case UNKNOWN: throw new IllegalStateException("Invalid span unit"); default: - return buildDateHistogram(name, field, value.intValue(), unit); + return buildDateHistogram(name, field, value.intValue(), unit, missingOrder); } } private CompositeValuesSourceBuilder buildDateHistogram( - String name, String field, Integer value, SpanUnit unit) { + String name, String field, Integer value, SpanUnit unit, MissingOrder missingOrder) { String spanValue = value + unit.getName(); switch (unit) { case MILLISECOND: @@ -94,11 +101,13 @@ private CompositeValuesSourceBuilder buildDateHistogram( return new DateHistogramValuesSourceBuilder(name) .field(field) .missingBucket(true) + .missingOrder(missingOrder) .fixedInterval(new DateHistogramInterval(spanValue)); default: return new DateHistogramValuesSourceBuilder(name) .field(field) .missingBucket(true) + .missingOrder(missingOrder) .calendarInterval(new DateHistogramInterval(spanValue)); } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicOptimizerTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicOptimizerTest.java index 767f398bf0..8085a2c0d4 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicOptimizerTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/planner/logical/OpenSearchLogicOptimizerTest.java @@ -305,29 +305,28 @@ void sort_refer_to_aggregator_should_not_merge_with_indexAgg() { } /** - * Can't Optimize the following query. * SELECT avg(intV) FROM schema GROUP BY stringV ORDER BY stringV ASC NULL_LAST. */ @Test - void sort_with_customized_option_should_not_merge_with_indexAgg() { + void sort_with_customized_option_should_merge_with_indexAgg() { assertEquals( - sort( - indexScanAgg("schema", - ImmutableList.of(DSL.named("AVG(intV)", dsl.avg(DSL.ref("intV", INTEGER)))), - ImmutableList.of(DSL.named("stringV", DSL.ref("stringV", STRING)))), - Pair.of(new Sort.SortOption(Sort.SortOrder.ASC, Sort.NullOrder.NULL_LAST), - DSL.ref("stringV", STRING)) - ), + indexScanAgg( + "schema", + ImmutableList.of(DSL.named("AVG(intV)", dsl.avg(DSL.ref("intV", INTEGER)))), + ImmutableList.of(DSL.named("stringV", DSL.ref("stringV", STRING))), + ImmutableList.of( + Pair.of( + new Sort.SortOption(Sort.SortOrder.ASC, Sort.NullOrder.NULL_LAST), + DSL.ref("stringV", STRING)))), optimize( sort( - indexScanAgg("schema", + indexScanAgg( + "schema", ImmutableList.of(DSL.named("AVG(intV)", dsl.avg(DSL.ref("intV", INTEGER)))), ImmutableList.of(DSL.named("stringV", DSL.ref("stringV", STRING)))), - Pair.of(new Sort.SortOption(Sort.SortOrder.ASC, Sort.NullOrder.NULL_LAST), - DSL.ref("stringV", STRING)) - ) - ) - ); + Pair.of( + new Sort.SortOption(Sort.SortOrder.ASC, Sort.NullOrder.NULL_LAST), + DSL.ref("stringV", STRING))))); } @Test diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilderTest.java index cd28aab2e7..04aedc0f01 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilderTest.java @@ -82,6 +82,7 @@ void should_build_composite_aggregation_for_field_reference() { + " \"terms\" : {\n" + " \"field\" : \"name\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + " }\n" @@ -114,6 +115,7 @@ void should_build_composite_aggregation_for_field_reference_with_order() { + " \"terms\" : {\n" + " \"field\" : \"name\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"last\",\n" + " \"order\" : \"desc\"\n" + " }\n" + " }\n" @@ -160,6 +162,7 @@ void should_build_composite_aggregation_for_field_reference_of_keyword() { + " \"terms\" : {\n" + " \"field\" : \"name.keyword\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + " }\n" @@ -211,6 +214,7 @@ void should_build_composite_aggregation_for_expression() { + " \"lang\" : \"opensearch_query_expression\"\n" + " },\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + " }\n" @@ -247,6 +251,7 @@ void should_build_composite_aggregation_follow_with_order_by_position() { + " \"terms\" : {\n" + " \"field\" : \"name\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"last\",\n" + " \"order\" : \"desc\"\n" + " }\n" + " }\n" @@ -255,6 +260,7 @@ void should_build_composite_aggregation_follow_with_order_by_position() { + " \"terms\" : {\n" + " \"field\" : \"age\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + " }\n" @@ -350,7 +356,8 @@ void should_build_filter_aggregation_group_by() { + " \"gender\" : {\n" + " \"terms\" : {\n" + " \"field\" : \"gender\",\n" - + " \"missing_bucket\" : true,\n" + + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + " }\n" @@ -411,6 +418,7 @@ void should_build_histogram() { + " \"histogram\" : {\n" + " \"field\" : \"age\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\",\n" + " \"interval\" : 10.0\n" + " }\n" @@ -444,6 +452,7 @@ void should_build_histogram_two_metrics() { + " \"histogram\" : {\n" + " \"field\" : \"age\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\",\n" + " \"interval\" : 10.0\n" + " }\n" @@ -483,6 +492,7 @@ void fixed_interval_time_span() { + " \"date_histogram\" : {\n" + " \"field\" : \"timestamp\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\",\n" + " \"fixed_interval\" : \"1h\"\n" + " }\n" @@ -516,6 +526,7 @@ void calendar_interval_time_span() { + " \"date_histogram\" : {\n" + " \"field\" : \"date\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\",\n" + " \"calendar_interval\" : \"1w\"\n" + " }\n" @@ -549,6 +560,7 @@ void general_span() { + " \"histogram\" : {\n" + " \"field\" : \"age\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\",\n" + " \"interval\" : 1.0\n" + " }\n" diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/GroupSortOrderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/GroupSortOrderTest.java index fbbb886c94..bff04604c1 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/GroupSortOrderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/GroupSortOrderTest.java @@ -17,6 +17,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.ast.tree.Sort; import org.opensearch.sql.expression.NamedExpression; @@ -36,31 +37,41 @@ class GroupSortOrderTest { void both_expression_in_sort_list() { assertEquals(-1, compare(named("name", ref), named("age", ref))); assertEquals(1, compare(named("age", ref), named("name", ref))); - assertEquals(SortOrder.DESC, order(named("name", ref))); - assertEquals(SortOrder.ASC, order(named("age", ref))); + assertEquals(SortOrder.DESC, sortOrder(named("name", ref))); + assertEquals(MissingOrder.LAST, missingOrder(named("name", ref))); + assertEquals(SortOrder.ASC, sortOrder(named("age", ref))); + assertEquals(MissingOrder.FIRST, missingOrder(named("age", ref))); } @Test void only_one_expression_in_sort_list() { assertEquals(-1, compare(named("name", ref), named("noSort", ref))); assertEquals(1, compare(named("noSort", ref), named("name", ref))); - assertEquals(SortOrder.DESC, order(named("name", ref))); - assertEquals(SortOrder.ASC, order(named("noSort", ref))); + assertEquals(SortOrder.DESC, sortOrder(named("name", ref))); + assertEquals(MissingOrder.LAST, missingOrder(named("name", ref))); + assertEquals(SortOrder.ASC, sortOrder(named("noSort", ref))); + assertEquals(MissingOrder.FIRST, missingOrder(named("noSort", ref))); } @Test void no_expression_in_sort_list() { assertEquals(0, compare(named("noSort1", ref), named("noSort2", ref))); assertEquals(0, compare(named("noSort2", ref), named("noSort1", ref))); - assertEquals(SortOrder.ASC, order(named("noSort1", ref))); - assertEquals(SortOrder.ASC, order(named("noSort2", ref))); + assertEquals(SortOrder.ASC, sortOrder(named("noSort1", ref))); + assertEquals(MissingOrder.FIRST, missingOrder(named("noSort1", ref))); + assertEquals(SortOrder.ASC, sortOrder(named("noSort2", ref))); + assertEquals(MissingOrder.FIRST, missingOrder(named("noSort2", ref))); } private int compare(NamedExpression e1, NamedExpression e2) { return groupSortOrder.compare(e1, e2); } - private SortOrder order(NamedExpression expr) { - return groupSortOrder.apply(expr); + private SortOrder sortOrder(NamedExpression expr) { + return groupSortOrder.sortOrder(expr); + } + + private MissingOrder missingOrder(NamedExpression expr) { + return groupSortOrder.missingOrder(expr); } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java index 24bc3170e5..a5d9bdf117 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java @@ -16,7 +16,7 @@ import java.util.Arrays; import java.util.List; import lombok.SneakyThrows; -import org.apache.commons.lang3.tuple.Pair; +import org.apache.commons.lang3.tuple.Triple; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; @@ -29,6 +29,7 @@ import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder; +import org.opensearch.search.aggregations.bucket.missing.MissingOrder; import org.opensearch.search.sort.SortOrder; import org.opensearch.sql.expression.NamedExpression; import org.opensearch.sql.opensearch.storage.serialization.ExpressionSerializer; @@ -54,6 +55,7 @@ void should_build_bucket_with_field() { + " \"terms\" : {\n" + " \"field\" : \"age\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + "}", @@ -69,6 +71,7 @@ void should_build_bucket_with_keyword_field() { + " \"terms\" : {\n" + " \"field\" : \"name.keyword\",\n" + " \"missing_bucket\" : true,\n" + + " \"missing_order\" : \"first\",\n" + " \"order\" : \"asc\"\n" + " }\n" + "}", @@ -78,7 +81,8 @@ void should_build_bucket_with_keyword_field() { } @SneakyThrows - private String buildQuery(List> groupByExpressions) { + private String buildQuery( + List> groupByExpressions) { XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON).prettyPrint(); builder.startObject(); CompositeValuesSourceBuilder sourceBuilder = @@ -88,7 +92,7 @@ private String buildQuery(List> groupByExpressi return BytesReference.bytes(builder).utf8ToString(); } - private Pair asc(NamedExpression expression) { - return Pair.of(expression, SortOrder.ASC); + private Triple asc(NamedExpression expression) { + return Triple.of(expression, SortOrder.ASC, MissingOrder.FIRST); } }