From 9009198ccfe95c23d0cd4628ecdbcac29321f04c Mon Sep 17 00:00:00 2001 From: penghuo Date: Fri, 20 Nov 2020 15:18:30 -0800 Subject: [PATCH 1/6] init --- ...sticsearchLogicalPlanOptimizerFactory.java | 6 +- .../logical/rule/MergeSortAndIndexScan.java | 81 ++++++++++++++ .../logical/rule/MergeSortAndRelation.java | 64 +++++++++++ .../logical/rule/OptimizationRuleUtils.java | 38 +++++++ .../storage/ElasticsearchIndex.java | 17 ++- .../storage/ElasticsearchIndexScan.java | 14 +++ .../ExpressionAggregationScript.java | 2 +- .../storage/script/core/ExpressionScript.java | 6 +- .../script/filter/ExpressionFilterScript.java | 2 +- .../script/filter/FilterQueryBuilder.java | 3 + .../storage/script/sort/SortQueryBuilder.java | 74 +++++++++++++ .../ElasticsearchLogicOptimizerTest.java | 100 ++++++++++++++---- .../storage/ElasticsearchIndexTest.java | 22 ++++ .../script/sort/SortQueryBuilderTest.java | 49 +++++++++ .../sql/elasticsearch/utils/Utils.java | 36 +++++++ 15 files changed, 485 insertions(+), 29 deletions(-) create mode 100644 elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndIndexScan.java create mode 100644 elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndRelation.java create mode 100644 elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/OptimizationRuleUtils.java create mode 100644 elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilder.java create mode 100644 elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilderTest.java diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicalPlanOptimizerFactory.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicalPlanOptimizerFactory.java index 4799bd1587..18646c96fa 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicalPlanOptimizerFactory.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicalPlanOptimizerFactory.java @@ -20,6 +20,8 @@ import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule.MergeAggAndIndexScan; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule.MergeAggAndRelation; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule.MergeFilterAndRelation; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule.MergeSortAndIndexScan; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule.MergeSortAndRelation; import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.LogicalPlanOptimizer; import java.util.Arrays; import lombok.experimental.UtilityClass; @@ -37,6 +39,8 @@ public static LogicalPlanOptimizer create() { return new LogicalPlanOptimizer(Arrays.asList( new MergeFilterAndRelation(), new MergeAggAndIndexScan(), - new MergeAggAndRelation())); + new MergeAggAndRelation(), + new MergeSortAndRelation(), + new MergeSortAndIndexScan())); } } diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndIndexScan.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndIndexScan.java new file mode 100644 index 0000000000..80e007d1ad --- /dev/null +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndIndexScan.java @@ -0,0 +1,81 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule; + +import static com.amazon.opendistroforelasticsearch.sql.planner.optimizer.pattern.Patterns.source; +import static com.facebook.presto.matching.Pattern.typeOf; + +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.ElasticsearchLogicalIndexScan; +import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalSort; +import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.Rule; +import com.facebook.presto.matching.Capture; +import com.facebook.presto.matching.Captures; +import com.facebook.presto.matching.Pattern; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.commons.lang3.tuple.Pair; + +/** + * Merge Sort with IndexScan only when Sort by fields. + */ +public class MergeSortAndIndexScan implements Rule { + + private final Capture indexScanCapture; + private final Pattern pattern; + + /** + * Constructor of MergeSortAndRelation. + */ + public MergeSortAndIndexScan() { + this.indexScanCapture = Capture.newCapture(); + this.pattern = typeOf(LogicalSort.class).matching(OptimizationRuleUtils::sortByFieldsOnly) + .with(source() + .matching(typeOf(ElasticsearchLogicalIndexScan.class).capturedAs(indexScanCapture))); + } + + @Override + public Pattern pattern() { + return pattern; + } + + @Override + public LogicalPlan apply(LogicalSort sort, + Captures captures) { + ElasticsearchLogicalIndexScan indexScan = captures.get(indexScanCapture); + + return ElasticsearchLogicalIndexScan + .builder() + .relationName(indexScan.getRelationName()) + .filter(indexScan.getFilter()) + .sortList(mergeSortList(indexScan.getSortList(), sort.getSortList())) + .build(); + } + + private List> mergeSortList(List> l1, List> l2) { + if (null == l1) { + return l2; + } else { + return Stream.concat(l1.stream(), l2.stream()).collect(Collectors.toList()); + } + } +} diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndRelation.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndRelation.java new file mode 100644 index 0000000000..6aef755e2c --- /dev/null +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/MergeSortAndRelation.java @@ -0,0 +1,64 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule; + +import static com.amazon.opendistroforelasticsearch.sql.planner.optimizer.pattern.Patterns.source; +import static com.facebook.presto.matching.Pattern.typeOf; + +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.ElasticsearchLogicalIndexScan; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalRelation; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalSort; +import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.Rule; +import com.facebook.presto.matching.Capture; +import com.facebook.presto.matching.Captures; +import com.facebook.presto.matching.Pattern; + +/** + * Merge Sort with Relation only when Sort by fields. + */ +public class MergeSortAndRelation implements Rule { + + private final Capture relationCapture; + private final Pattern pattern; + + /** + * Constructor of MergeSortAndRelation. + */ + public MergeSortAndRelation() { + this.relationCapture = Capture.newCapture(); + this.pattern = typeOf(LogicalSort.class).matching(OptimizationRuleUtils::sortByFieldsOnly) + .with(source().matching(typeOf(LogicalRelation.class).capturedAs(relationCapture))); + } + + @Override + public Pattern pattern() { + return pattern; + } + + @Override + public LogicalPlan apply(LogicalSort sort, + Captures captures) { + LogicalRelation relation = captures.get(relationCapture); + return ElasticsearchLogicalIndexScan + .builder() + .relationName(relation.getRelationName()) + .sortList(sort.getSortList()) + .build(); + } +} diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/OptimizationRuleUtils.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/OptimizationRuleUtils.java new file mode 100644 index 0000000000..8330db9a74 --- /dev/null +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/rule/OptimizationRuleUtils.java @@ -0,0 +1,38 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.rule; + +import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalSort; +import lombok.experimental.UtilityClass; + +@UtilityClass +public class OptimizationRuleUtils { + + /** + * Does the sort list only contain {@link ReferenceExpression}. + * + * @param logicalSort LogicalSort. + * @return true only contain ReferenceExpression, otherwise false. + */ + public static boolean sortByFieldsOnly(LogicalSort logicalSort) { + return logicalSort.getSortList().stream() + .map(sort -> sort.getRight() instanceof ReferenceExpression) + .reduce(true, Boolean::logicalAnd); + } +} diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndex.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndex.java index 3274b9d5ac..7da924c908 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndex.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndex.java @@ -29,6 +29,7 @@ import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.ElasticsearchLogicalPlanOptimizerFactory; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.aggregation.AggregationQueryBuilder; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.filter.FilterQueryBuilder; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.sort.SortQueryBuilder; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.serialization.DefaultExpressionSerializer; import com.amazon.opendistroforelasticsearch.sql.planner.DefaultImplementor; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; @@ -40,6 +41,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.search.aggregations.AggregationBuilder; @@ -145,9 +147,18 @@ public PhysicalPlan visitNode(LogicalPlan plan, ElasticsearchIndexScan context) */ public PhysicalPlan visitIndexScan(ElasticsearchLogicalIndexScan node, ElasticsearchIndexScan context) { - FilterQueryBuilder queryBuilder = new FilterQueryBuilder(new DefaultExpressionSerializer()); - QueryBuilder query = queryBuilder.build(node.getFilter()); - context.pushDown(query); + if (null != node.getSortList()) { + final SortQueryBuilder builder = new SortQueryBuilder(); + context.pushDownSort(node.getSortList().stream() + .map(sort -> builder.build(sort.getValue(), sort.getKey())) + .collect(Collectors.toList())); + } + + if (null != node.getFilter()) { + FilterQueryBuilder queryBuilder = new FilterQueryBuilder(new DefaultExpressionSerializer()); + QueryBuilder query = queryBuilder.build(node.getFilter()); + context.pushDown(query); + } return indexScan; } diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java index c60ac55ac9..fe78e278a7 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java @@ -41,6 +41,7 @@ import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.builder.SearchSourceBuilder; +import org.elasticsearch.search.sort.SortBuilder; /** * Elasticsearch index scan operator. @@ -103,6 +104,7 @@ public ExprValue next() { public void pushDown(QueryBuilder query) { SearchSourceBuilder source = request.getSourceBuilder(); QueryBuilder current = source.query(); + if (current == null) { source.query(query); } else { @@ -130,6 +132,18 @@ public void pushDownAggregation(List aggregationBuilderList) source.size(0); } + /** + * Push down sort to DSL request. + * + * @param sortBuilders sortBuilders. + */ + public void pushDownSort(List> sortBuilders) { + SearchSourceBuilder source = request.getSourceBuilder(); + for (SortBuilder sortBuilder : sortBuilders) { + source.sort(sortBuilder); + } + } + public void pushTypeMapping(Map typeMapping) { request.getExprValueFactory().setTypeMapping(typeMapping); } diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/aggregation/ExpressionAggregationScript.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/aggregation/ExpressionAggregationScript.java index 64f7538e0d..8125cc6f56 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/aggregation/ExpressionAggregationScript.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/aggregation/ExpressionAggregationScript.java @@ -53,7 +53,7 @@ public ExpressionAggregationScript( @Override public Object execute() { - return expressionScript.execute(this::getDoc, this::evaluateExpression); + return expressionScript.execute(this::getDoc, this::evaluateExpression).value(); } private ExprValue evaluateExpression(Expression expression, Environment>> docProvider, + public ExprValue execute(Supplier>> docProvider, BiFunction, ExprValue> evaluator) { - return AccessController.doPrivileged((PrivilegedAction) () -> { + return AccessController.doPrivileged((PrivilegedAction) () -> { Environment valueEnv = buildValueEnv(fields, valueFactory, docProvider); ExprValue result = evaluator.apply(expression, valueEnv); - return result.value(); + return result; }); } diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java index 003969f7c6..315a6068a4 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java @@ -50,7 +50,7 @@ public ExpressionFilterScript(Expression expression, @Override public boolean execute() { - return (Boolean) expressionScript.execute(this::getDoc, this::evaluateExpression); + return expressionScript.execute(this::getDoc, this::evaluateExpression).booleanValue(); } private ExprValue evaluateExpression(Expression expression, diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilder.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilder.java index 8146328a87..4cc8be3512 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilder.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilder.java @@ -40,6 +40,9 @@ import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.ScriptQueryBuilder; import org.elasticsearch.script.Script; +import org.elasticsearch.search.sort.FieldSortBuilder; +import org.elasticsearch.search.sort.SortBuilder; +import org.elasticsearch.search.sort.SortBuilders; @RequiredArgsConstructor public class FilterQueryBuilder extends ExpressionNodeVisitor { diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilder.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilder.java new file mode 100644 index 0000000000..07cb6f05c8 --- /dev/null +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilder.java @@ -0,0 +1,74 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.sort; + +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.ScriptUtils; +import com.amazon.opendistroforelasticsearch.sql.expression.Expression; +import com.amazon.opendistroforelasticsearch.sql.expression.ReferenceExpression; +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.elasticsearch.search.sort.FieldSortBuilder; +import org.elasticsearch.search.sort.SortBuilder; +import org.elasticsearch.search.sort.SortBuilders; +import org.elasticsearch.search.sort.SortOrder; + +/** + * Builder of {@link SortBuilder}. + */ +public class SortQueryBuilder { + + /** + * The mapping between Core Engine sort order and Elasticsearch sort order. + */ + private Map sortOrderMap = + new ImmutableMap.Builder() + .put(Sort.SortOrder.ASC, SortOrder.ASC) + .put(Sort.SortOrder.DESC, SortOrder.DESC) + .build(); + + /** + * The mapping between Core Engine null order and Elasticsearch null order. + */ + private Map missingMap = + new ImmutableMap.Builder() + .put(Sort.NullOrder.NULL_FIRST, "_first") + .put(Sort.NullOrder.NULL_LAST, "_last") + .build(); + + /** + * Build {@link SortBuilder}. + * + * @param expression expression + * @param option sort option + * @return SortBuilder. + */ + public SortBuilder build(Expression expression, Sort.SortOption option) { + if (expression instanceof ReferenceExpression) { + return fieldBuild((ReferenceExpression) expression, option); + } else { + throw new IllegalStateException("unsupported expression " + expression.getClass()); + } + } + + private FieldSortBuilder fieldBuild(ReferenceExpression ref, Sort.SortOption option) { + return SortBuilders.fieldSort(ScriptUtils.convertTextToKeyword(ref.getAttr(), ref.type())) + .order(sortOrderMap.get(option.getSortOrder())) + .missing(missingMap.get(option.getNullOrder())); + } +} diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java index 1bb943aee2..95a56e5cf4 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java @@ -21,22 +21,23 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.DOUBLE; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.LONG; +import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.utils.Utils.indexScan; +import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.utils.Utils.indexScanAgg; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.aggregation; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.filter; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.project; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.relation; +import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.sort; import static org.junit.jupiter.api.Assertions.assertEquals; +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; -import com.amazon.opendistroforelasticsearch.sql.expression.Expression; -import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; -import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.NamedAggregator; import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.LogicalPlanOptimizer; import com.google.common.collect.ImmutableList; import java.util.Collections; -import java.util.List; +import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -155,27 +156,86 @@ void aggregation_cant_merge_indexScan_with_project() { ); } - private LogicalPlan optimize(LogicalPlan plan) { - final LogicalPlanOptimizer optimizer = ElasticsearchLogicalPlanOptimizerFactory.create(); - final LogicalPlan optimize = optimizer.optimize(plan); - return optimize; + /** + * Sort - Relation --> IndexScan. + */ + @Test + void sort_merge_with_relation() { + assertEquals( + indexScan("schema", Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER))), + optimize( + sort( + relation("schema"), + 0, + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER)) + ) + ) + ); } - public static LogicalPlan indexScan(String tableName, Expression filter) { - return ElasticsearchLogicalIndexScan.builder().relationName(tableName).filter(filter).build(); + /** + * Sort - IndexScan --> IndexScan. + */ + @Test + void sort_merge_with_indexScan() { + assertEquals( + indexScan("schema", + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER)), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG))), + optimize( + sort( + indexScan("schema", Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER))), + 0, + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ) + ) + ); + } + + /** + * Sort - Filter - Relation --> IndexScan. + */ + @Test + void sort_filter_merge_with_relation() { + assertEquals( + indexScan("schema", + dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ), + optimize( + sort( + filter( + relation("schema"), + dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) + ), + 0, + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ) + ) + ); } - public static LogicalPlan indexScanAgg(String tableName, List aggregators, - List groupByList) { - return ElasticsearchLogicalIndexAgg.builder().relationName(tableName) - .aggregatorList(aggregators).groupByList(groupByList).build(); + @Test + void sort_with_expression_cannot_merge_with_relation() { + assertEquals( + sort( + relation("schema"), + 0, + Pair.of(Sort.SortOption.DEFAULT_ASC, dsl.abs(DSL.ref("intV", INTEGER))) + ), + optimize( + sort( + relation("schema"), + 0, + Pair.of(Sort.SortOption.DEFAULT_ASC, dsl.abs(DSL.ref("intV", INTEGER))) + ) + ) + ); } - public static LogicalPlan indexScanAgg(String tableName, - Expression filter, - List aggregators, - List groupByList) { - return ElasticsearchLogicalIndexAgg.builder().relationName(tableName).filter(filter) - .aggregatorList(aggregators).groupByList(groupByList).build(); + private LogicalPlan optimize(LogicalPlan plan) { + final LogicalPlanOptimizer optimizer = ElasticsearchLogicalPlanOptimizerFactory.create(); + final LogicalPlan optimize = optimizer.optimize(plan); + return optimize; } } \ No newline at end of file diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java index af985aab7c..a54695b374 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java @@ -321,4 +321,26 @@ void shouldNotPushDownAggregationFarFromRelation() { groupByExprs)); assertTrue(plan instanceof AggregationOperator); } + + @Test + void shouldImplIndexScanWithSort() { + when(settings.getSettingValue(Settings.Key.QUERY_SIZE_LIMIT)).thenReturn(200); + + ReferenceExpression field = ref("name", STRING); + NamedExpression named = named("n", field); + Expression sortExpr = ref("name", STRING); + + String indexName = "test"; + ElasticsearchIndex index = new ElasticsearchIndex(client, settings, indexName); + PhysicalPlan plan = index.implement( + project( + indexScan( + indexName, + Pair.of(Sort.SortOption.DEFAULT_ASC, sortExpr) + ), + named)); + + assertTrue(plan instanceof ProjectOperator); + assertTrue(((ProjectOperator) plan).getInput() instanceof ElasticsearchIndexScan); + } } diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilderTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilderTest.java new file mode 100644 index 0000000000..2ad0146a34 --- /dev/null +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/sort/SortQueryBuilderTest.java @@ -0,0 +1,49 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.sort; + +import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; +import com.amazon.opendistroforelasticsearch.sql.expression.DSL; +import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +class SortQueryBuilderTest { + + private final DSL dsl = new ExpressionConfig().dsl(new ExpressionConfig().functionRepository()); + + private SortQueryBuilder sortQueryBuilder = new SortQueryBuilder(); + + @Test + void build_sortbuilder_from_reference() { + assertNotNull(sortQueryBuilder.build(DSL.ref("intV", INTEGER), Sort.SortOption.DEFAULT_ASC)); + } + + @Test + void build_sortbuilder_from_function_should_throw_exception() { + final IllegalStateException exception = + assertThrows(IllegalStateException.class, () -> sortQueryBuilder.build(dsl.equal(DSL.ref( + "intV", INTEGER), DSL.literal(1)), Sort.SortOption.DEFAULT_ASC)); + assertThat(exception.getMessage(), Matchers.containsString("unsupported expression")); + } +} \ No newline at end of file diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/utils/Utils.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/utils/Utils.java index 9298999a32..3a67e96b23 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/utils/Utils.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/utils/Utils.java @@ -17,26 +17,62 @@ package com.amazon.opendistroforelasticsearch.sql.elasticsearch.utils; +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.ElasticsearchLogicalIndexAgg; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.planner.logical.ElasticsearchLogicalIndexScan; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.expression.NamedExpression; import com.amazon.opendistroforelasticsearch.sql.expression.aggregation.NamedAggregator; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import java.util.Arrays; import java.util.List; +import lombok.experimental.UtilityClass; +import org.apache.commons.lang3.tuple.Pair; +@UtilityClass public class Utils { + /** + * Build ElasticsearchLogicalIndexScan. + */ public static LogicalPlan indexScan(String tableName, Expression filter) { return ElasticsearchLogicalIndexScan.builder().relationName(tableName).filter(filter).build(); } + /** + * Build ElasticsearchLogicalIndexScan. + */ + public static LogicalPlan indexScan(String tableName, + Pair... sorts) { + return ElasticsearchLogicalIndexScan.builder().relationName(tableName) + .sortList(Arrays.asList(sorts)) + .build(); + } + + /** + * Build ElasticsearchLogicalIndexScan. + */ + public static LogicalPlan indexScan(String tableName, + Expression filter, + Pair... sorts) { + return ElasticsearchLogicalIndexScan.builder().relationName(tableName) + .filter(filter) + .sortList(Arrays.asList(sorts)) + .build(); + } + + /** + * Build ElasticsearchLogicalIndexAgg. + */ public static LogicalPlan indexScanAgg(String tableName, List aggregators, List groupByList) { return ElasticsearchLogicalIndexAgg.builder().relationName(tableName) .aggregatorList(aggregators).groupByList(groupByList).build(); } + /** + * Build ElasticsearchLogicalIndexAgg. + */ public static LogicalPlan indexScanAgg(String tableName, Expression filter, List aggregators, From 5a8a231e416198219b8715a0cab3432f192315a9 Mon Sep 17 00:00:00 2001 From: penghuo Date: Fri, 20 Nov 2020 22:57:52 -0800 Subject: [PATCH 2/6] remove the count option from sort --- .../sql/analysis/Analyzer.java | 5 +- .../analysis/WindowExpressionAnalyzer.java | 2 +- .../sql/ast/dsl/AstDSL.java | 12 ++--- .../sql/ast/tree/Sort.java | 1 - .../sql/executor/Explain.java | 1 - .../sql/planner/DefaultImplementor.java | 2 +- .../sql/planner/logical/LogicalPlanDSL.java | 5 +- .../sql/planner/logical/LogicalSort.java | 4 +- .../sql/planner/physical/PhysicalPlanDSL.java | 4 +- .../sql/planner/physical/SortOperator.java | 13 ++--- .../sql/analysis/AnalyzerTest.java | 4 -- .../WindowExpressionAnalyzerTest.java | 1 - .../sql/executor/ExplainTest.java | 2 - .../sql/planner/DefaultImplementorTest.java | 4 -- .../logical/LogicalPlanNodeVisitorTest.java | 2 +- .../sql/planner/logical/LogicalSortTest.java | 6 --- .../physical/PhysicalPlanNodeVisitorTest.java | 2 +- .../planner/physical/SortOperatorTest.java | 49 +++---------------- .../planner/physical/WindowOperatorTest.java | 2 +- docs/experiment/ppl/cmd/sort.rst | 24 ++------- .../ElasticsearchExecutionProtector.java | 1 - .../ElasticsearchExecutionProtectorTest.java | 4 -- .../ElasticsearchLogicOptimizerTest.java | 5 -- .../storage/ElasticsearchIndexTest.java | 2 - .../expectedOutput/ppl/explain_output.json | 1 - ppl/src/main/antlr/OpenDistroPPLParser.g4 | 2 +- .../sql/ppl/parser/AstBuilder.java | 5 -- .../sql/ppl/utils/ArgumentFactory.java | 17 ------- .../sql/ppl/utils/PPLQueryDataAnonymizer.java | 3 +- .../sql/ppl/parser/AstBuilderTest.java | 6 +-- .../ppl/parser/AstExpressionBuilderTest.java | 8 --- .../sql/ppl/utils/ArgumentFactoryTest.java | 19 +------ .../ppl/utils/PPLQueryDataAnonymizerTest.java | 4 +- .../sql/sql/parser/AstSortBuilder.java | 2 - .../sql/sql/parser/AstBuilderTest.java | 5 -- .../sql/sql/parser/AstSortBuilderTest.java | 1 - 36 files changed, 35 insertions(+), 195 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java index d5ddb0ef98..f24902fe53 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/Analyzer.java @@ -310,8 +310,6 @@ public LogicalPlan visitSort(Sort node, AnalysisContext context) { ExpressionReferenceOptimizer optimizer = new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child); - // the first options is {"count": "integer"} - Integer count = (Integer) node.getOptions().get(0).getValue().getValue(); List> sortList = node.getSortList().stream() .map( @@ -324,8 +322,7 @@ public LogicalPlan visitSort(Sort node, AnalysisContext context) { asc ? SortOption.DEFAULT_ASC : SortOption.DEFAULT_DESC, expression); }) .collect(Collectors.toList()); - - return new LogicalSort(child, count, sortList); + return new LogicalSort(child, sortList); } /** diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzer.java index 8c13076b0f..67440045ca 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzer.java @@ -77,7 +77,7 @@ public LogicalPlan visitWindowFunction(WindowFunction node, AnalysisContext cont WindowDefinition windowDefinition = new WindowDefinition(partitionByList, sortList); return new LogicalWindow( - new LogicalSort(child, 0, windowDefinition.getAllSortItems()), + new LogicalSort(child,windowDefinition.getAllSortItems()), windowFunction, windowDefinition); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java index 0b1a2b1a00..35a6e35f00 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/dsl/AstDSL.java @@ -332,20 +332,16 @@ public static List defaultDedupArgs() { argument("consecutive", booleanLiteral(false))); } - public static List defaultSortOptions() { - return exprList(argument("count", intLiteral(1000)), argument("desc", booleanLiteral(false))); - } - - public static List sortOptions(int count) { - return exprList(argument("count", intLiteral(count)), argument("desc", booleanLiteral(false))); + public static List sortOptions() { + return exprList(argument("desc", booleanLiteral(false))); } public static List defaultSortFieldArgs() { return exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral())); } - public static Sort sort(UnresolvedPlan input, List options, Field... sorts) { - return new Sort(input, options, Arrays.asList(sorts)); + public static Sort sort(UnresolvedPlan input, Field... sorts) { + return new Sort(input, Arrays.asList(sorts)); } public static Dedupe dedupe(UnresolvedPlan input, List options, Field... fields) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java index 530bb3aeec..485048746d 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java @@ -42,7 +42,6 @@ @AllArgsConstructor public class Sort extends UnresolvedPlan { private UnresolvedPlan child; - private final List options; private final List sortList; @Override diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/Explain.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/Explain.java index 241b81857f..1d3554ef34 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/Explain.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/Explain.java @@ -70,7 +70,6 @@ public ExplainResponseNode visitFilter(FilterOperator node, Object context) { @Override public ExplainResponseNode visitSort(SortOperator node, Object context) { return explain(node, context, explainNode -> explainNode.setDescription(ImmutableMap.of( - "count", node.getCount(), "sortList", describeSortList(node.getSortList())))); } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementor.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementor.java index 05cb775cb9..d49fa70540 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementor.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementor.java @@ -112,7 +112,7 @@ public PhysicalPlan visitEval(LogicalEval node, C context) { @Override public PhysicalPlan visitSort(LogicalSort node, C context) { - return new SortOperator(visitChild(node, context), node.getCount(), node.getSortList()); + return new SortOperator(visitChild(node, context), node.getSortList()); } @Override diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java index 8742c5be4c..4e5b7a60c0 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanDSL.java @@ -73,9 +73,8 @@ public static LogicalPlan eval( return new LogicalEval(input, Arrays.asList(expressions)); } - public static LogicalPlan sort( - LogicalPlan input, Integer count, Pair... sorts) { - return new LogicalSort(input, count, Arrays.asList(sorts)); + public static LogicalPlan sort(LogicalPlan input, Pair... sorts) { + return new LogicalSort(input, Arrays.asList(sorts)); } public static LogicalPlan dedupe(LogicalPlan input, Expression... fields) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java index 1a4efcb95d..2354eb128c 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java @@ -34,17 +34,15 @@ @EqualsAndHashCode(callSuper = true) public class LogicalSort extends LogicalPlan { - private final Integer count; private final List> sortList; /** * Constructor of LogicalSort. */ public LogicalSort( - LogicalPlan child, Integer count, + LogicalPlan child, List> sortList) { super(Collections.singletonList(child)); - this.count = count; this.sortList = sortList; } diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java index 5d139c866d..f1a0244787 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanDSL.java @@ -63,9 +63,9 @@ public static EvalOperator eval( return new EvalOperator(input, Arrays.asList(expressions)); } - public static SortOperator sort(PhysicalPlan input, Integer count, Pair... sorts) { - return new SortOperator(input, count, Arrays.asList(sorts)); + return new SortOperator(input, Arrays.asList(sorts)); } public static DedupeOperator dedupe(PhysicalPlan input, Expression... expressions) { diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java index 4e63ad15ab..a1b639bd75 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java @@ -46,11 +46,7 @@ public class SortOperator extends PhysicalPlan { @Getter private final PhysicalPlan input; - /** - * How many sorted result should been return. If count = 0, all the resulted will be returned. - */ - @Getter - private final Integer count; + @Getter private final List> sortList; @EqualsAndHashCode.Exclude @@ -61,14 +57,12 @@ public class SortOperator extends PhysicalPlan { /** * Sort Operator Constructor. * @param input input {@link PhysicalPlan} - * @param count how many sorted result should been return * @param sortList list of sort sort field. * The sort field is specified by the {@link Expression} with {@link SortOption} */ public SortOperator( - PhysicalPlan input, Integer count, List> sortList) { + PhysicalPlan input, List> sortList) { this.input = input; - this.count = count; this.sortList = sortList; SorterBuilder sorterBuilder = Sorter.builder(); for (Pair pair : sortList) { @@ -97,8 +91,7 @@ public void open() { sorted.add(input.next()); } - Iterator sortedIterator = iterator(sorted); - iterator = count == 0 ? sortedIterator : Iterators.limit(sortedIterator, count); + iterator = iterator(sorted); } @Override diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java index a7115c3d71..d6f7ce95fd 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java @@ -45,7 +45,6 @@ import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import com.amazon.opendistroforelasticsearch.sql.expression.window.WindowDefinition; -import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -296,7 +295,6 @@ public void sort_with_aggregator() { "avg(integer_value)", dsl.avg(DSL.ref("integer_value", INTEGER)))), ImmutableList.of(DSL.named("string_value", DSL.ref("string_value", STRING)))), - 0, // Aggregator in Sort AST node is replaced with reference by expression optimizer Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("avg(integer_value)", DOUBLE))), DSL.named("string_value", DSL.ref("string_value", STRING))), @@ -312,7 +310,6 @@ public void sort_with_aggregator() { ImmutableList.of(AstDSL.alias("string_value", qualifiedName("string_value"))), emptyList() ), - ImmutableList.of(argument("count", intLiteral(0))), field( function("avg", qualifiedName("integer_value")), argument("asc", booleanLiteral(true)))), @@ -327,7 +324,6 @@ public void window_function() { LogicalPlanDSL.window( LogicalPlanDSL.sort( LogicalPlanDSL.relation("test"), - 0, ImmutablePair.of(DEFAULT_ASC, DSL.ref("string_value", STRING)), ImmutablePair.of(DEFAULT_ASC, DSL.ref("integer_value", INTEGER))), dsl.rowNumber(), diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzerTest.java index e1409ce7df..a13be922ce 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/WindowExpressionAnalyzerTest.java @@ -62,7 +62,6 @@ void should_wrap_child_with_window_and_sort_operator_if_project_item_windowed() LogicalPlanDSL.window( LogicalPlanDSL.sort( LogicalPlanDSL.relation("test"), - 0, ImmutablePair.of(DEFAULT_ASC, DSL.ref("string_value", STRING)), ImmutablePair.of(DEFAULT_DESC, DSL.ref("integer_value", INTEGER))), dsl.rowNumber(), diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/executor/ExplainTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/executor/ExplainTest.java index e5383f2d29..23d233f49c 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/executor/ExplainTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/executor/ExplainTest.java @@ -205,7 +205,6 @@ void can_explain_other_operators() { dedupe( sort( values(values), - 1000, sortList), dedupeList), evalExprs), @@ -233,7 +232,6 @@ void can_explain_other_operators() { singletonList(new ExplainResponseNode( "SortOperator", ImmutableMap.of( - "count", 1000, "sortList", ImmutableMap.of( "age", ImmutableMap.of( "sortOrder", "ASC", diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java index 6a7267e7ae..8e1016c237 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/DefaultImplementorTest.java @@ -119,7 +119,6 @@ public void visitShouldReturnDefaultPhysicalOperator() { mappings), exclude), newEvalField), - sortCount, sortField), CommandType.TOP, topByExprs, @@ -150,7 +149,6 @@ public void visitShouldReturnDefaultPhysicalOperator() { mappings), exclude), newEvalField), - sortCount, sortField), CommandType.TOP, topByExprs, @@ -192,7 +190,6 @@ public void visitWindowOperatorShouldReturnPhysicalWindowOperator() { window( sort( values(), - 0, sortList), windowFunction, windowDefinition), @@ -203,7 +200,6 @@ public void visitWindowOperatorShouldReturnPhysicalWindowOperator() { PhysicalPlanDSL.window( PhysicalPlanDSL.sort( PhysicalPlanDSL.values(), - 0, sortList), windowFunction, windowDefinition), diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java index fec3c8920e..e7a7ed590e 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java @@ -105,7 +105,7 @@ public void testAbstractPlanNodeVisitorShouldReturnNull() { assertNull(eval.accept(new LogicalPlanNodeVisitor() { }, null)); - LogicalPlan sort = LogicalPlanDSL.sort(relation, 100, + LogicalPlan sort = LogicalPlanDSL.sort(relation, Pair.of(SortOption.DEFAULT_ASC, expression)); assertNull(sort.accept(new LogicalPlanNodeVisitor() { }, null)); diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSortTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSortTest.java index bb8326d947..11b310a3aa 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSortTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSortTest.java @@ -18,13 +18,11 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.argument; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.booleanLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortFieldArgs; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortOptions; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.exprList; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.field; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.nullLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.sort; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.sortOptions; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.DOUBLE; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; @@ -48,12 +46,10 @@ public void analyze_sort_with_two_field_with_default_option() { assertAnalyzeEqual( LogicalPlanDSL.sort( LogicalPlanDSL.relation("schema"), - 1000, ImmutablePair.of(SortOption.DEFAULT_ASC, DSL.ref("integer_value", INTEGER)), ImmutablePair.of(SortOption.DEFAULT_ASC, DSL.ref("double_value", DOUBLE))), sort( relation("schema"), - defaultSortOptions(), field("integer_value", defaultSortFieldArgs()), field("double_value", defaultSortFieldArgs()))); } @@ -63,12 +59,10 @@ public void analyze_sort_with_two_field() { assertAnalyzeEqual( LogicalPlanDSL.sort( LogicalPlanDSL.relation("schema"), - 100, ImmutablePair.of(SortOption.DEFAULT_DESC, DSL.ref("integer_value", INTEGER)), ImmutablePair.of(SortOption.DEFAULT_ASC, DSL.ref("double_value", DOUBLE))), sort( relation("schema"), - sortOptions(100), field( "integer_value", exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index 901fa330c7..97b54f3526 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -127,7 +127,7 @@ public void test_PhysicalPlanVisitor_should_return_null() { assertNull(eval.accept(new PhysicalPlanNodeVisitor() { }, null)); - PhysicalPlan sort = PhysicalPlanDSL.sort(plan, 100, Pair.of(SortOption.DEFAULT_ASC, ref)); + PhysicalPlan sort = PhysicalPlanDSL.sort(plan, Pair.of(SortOption.DEFAULT_ASC, ref)); assertNull(sort.accept(new PhysicalPlanNodeVisitor() { }, null)); diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperatorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperatorTest.java index e598198b23..34ce94cf08 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperatorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperatorTest.java @@ -60,23 +60,7 @@ public void sort_one_field_asc() { .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), - contains( - tupleValue(ImmutableMap.of("size", 320, "response", 200)), - tupleValue(ImmutableMap.of("size", 499, "response", 404)), - tupleValue(ImmutableMap.of("size", 399, "response", 503)))); - } - - @Test - public void sort_one_field_with_count_equal_to_zero_should_return_all() { - when(inputPlan.hasNext()).thenReturn(true, true, true, false); - when(inputPlan.next()) - .thenReturn(tupleValue(ImmutableMap.of("size", 499, "response", 404))) - .thenReturn(tupleValue(ImmutableMap.of("size", 320, "response", 200))) - .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); - - assertThat( - execute(sort(inputPlan, 0, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 320, "response", 200)), tupleValue(ImmutableMap.of("size", 499, "response", 404)), @@ -92,7 +76,7 @@ public void sort_one_field_with_duplication() { .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 499, "response", 404)), tupleValue(ImmutableMap.of("size", 320, "response", 404)), @@ -109,7 +93,7 @@ public void sort_one_field_asc_with_null_value() { .thenReturn(tupleValue(NULL_MAP)); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( tupleValue(NULL_MAP), tupleValue(ImmutableMap.of("size", 320, "response", 200)), @@ -127,7 +111,7 @@ public void sort_one_field_asc_with_missing_value() { .thenReturn(tupleValue(ImmutableMap.of("size", 399))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 399)), tupleValue(ImmutableMap.of("size", 320, "response", 200)), @@ -144,7 +128,7 @@ public void sort_one_field_desc() { .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 399, "response", 503)), tupleValue(ImmutableMap.of("size", 499, "response", 404)), @@ -161,7 +145,7 @@ public void sort_one_field_desc_with_null_value() { .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 399, "response", 503)), tupleValue(ImmutableMap.of("size", 499, "response", 404)), @@ -179,7 +163,7 @@ public void sort_one_field_with_duplicate_value() { .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); assertThat( - execute(sort(inputPlan, 100, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( tupleValue(ImmutableMap.of("size", 320, "response", 200)), tupleValue(ImmutableMap.of("size", 499, "response", 404)), @@ -201,7 +185,6 @@ public void sort_two_fields_both_asc() { execute( sort( inputPlan, - 100, Pair.of(SortOption.DEFAULT_ASC, ref("size", INTEGER)), Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( @@ -226,7 +209,6 @@ public void sort_two_fields_both_desc() { execute( sort( inputPlan, - 100, Pair.of(SortOption.DEFAULT_DESC, ref("size", INTEGER)), Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), contains( @@ -251,7 +233,6 @@ public void sort_two_fields_asc_and_desc() { execute( sort( inputPlan, - 100, Pair.of(SortOption.DEFAULT_ASC, ref("size", INTEGER)), Pair.of(SortOption.DEFAULT_DESC, ref("response", INTEGER)))), contains( @@ -276,7 +257,6 @@ public void sort_two_fields_desc_and_asc() { execute( sort( inputPlan, - 100, Pair.of(SortOption.DEFAULT_DESC, ref("size", INTEGER)), Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), contains( @@ -287,26 +267,13 @@ public void sort_two_fields_desc_and_asc() { tupleValue(ImmutableMap.of("size", 320, "response", 200)))); } - @Test - public void sort_one_field_asc_with_count() { - when(inputPlan.hasNext()).thenReturn(true, true, true, false); - when(inputPlan.next()) - .thenReturn(tupleValue(ImmutableMap.of("size", 499, "response", 404))) - .thenReturn(tupleValue(ImmutableMap.of("size", 320, "response", 200))) - .thenReturn(tupleValue(ImmutableMap.of("size", 399, "response", 503))); - - assertThat( - execute(sort(inputPlan, 1, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))), - contains(tupleValue(ImmutableMap.of("size", 320, "response", 200)))); - } - @Test public void sort_one_field_without_input() { when(inputPlan.hasNext()).thenReturn(false); assertEquals( 0, - execute(sort(inputPlan, 1, + execute(sort(inputPlan, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))).size()); } } diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/WindowOperatorTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/WindowOperatorTest.java index 2f6a242f88..9063b0d1e2 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/WindowOperatorTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/WindowOperatorTest.java @@ -95,7 +95,7 @@ WindowOperatorAssertion expectNext(Map expected) { if (windowOperator == null) { WindowDefinition definition = new WindowDefinition(partitionByList, sortList); windowOperator = new WindowOperator( - new SortOperator(new TestScan(), 10000, definition.getAllSortItems()), + new SortOperator(new TestScan(), definition.getAllSortItems()), windowFunction, definition); windowOperator.open(); diff --git a/docs/experiment/ppl/cmd/sort.rst b/docs/experiment/ppl/cmd/sort.rst index 9df4bcfdcf..2c83c15b7d 100644 --- a/docs/experiment/ppl/cmd/sort.rst +++ b/docs/experiment/ppl/cmd/sort.rst @@ -16,10 +16,9 @@ Description Syntax ============ -sort [count] <[+|-] sort-field>... +sort <[+|-] sort-field>... -* count: optional. The maximum number results to return from the sorted result. if count=0, all the result will be returned. **Default:** 1000 * [+|-]: optional. The plus [+] for ascending order and a minus [-] for descending order. **Default:** ascending order. * sort-field: mandatory. The field used to sort. @@ -50,7 +49,7 @@ The example show sort all the document with age field in ascending order. PPL query:: - od> source=accounts | sort 0 age | fields account_number, age; + od> source=accounts | sort age | fields account_number, age; fetched rows / total rows = 4/4 +------------------+-------+ | account_number | age | @@ -80,24 +79,7 @@ PPL query:: | 13 | 28 | +------------------+-------+ - -Example 4: Specify the number of sorted documents to return -============================================================ - -The example show sort all the document and return 2 documents. - -PPL query:: - - od> source=accounts | sort 2 age | fields account_number, age; - fetched rows / total rows = 2/2 - +------------------+-------+ - | account_number | age | - |------------------+-------| - | 13 | 28 | - | 1 | 32 | - +------------------+-------+ - -Example 5: Sort by multiple field +Example 4: Sort by multiple field ============================= The example show sort all the document with gender field in ascending order and age field in descending. diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/protector/ElasticsearchExecutionProtector.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/protector/ElasticsearchExecutionProtector.java index 8d4f765b69..929e218179 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/protector/ElasticsearchExecutionProtector.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/protector/ElasticsearchExecutionProtector.java @@ -126,7 +126,6 @@ public PhysicalPlan visitSort(SortOperator node, Object context) { return new ResourceMonitorPlan( new SortOperator( visitInput(node.getInput(), context), - node.getCount(), node.getSortList()), resourceMonitor); } diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java index 83212d3e50..d94f68ada9 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/executor/ElasticsearchExecutionProtectorTest.java @@ -139,7 +139,6 @@ public void testProtectIndexScan() { mappings), exclude), newEvalField), - sortCount, sortField)), CommandType.TOP, topExprs, @@ -169,7 +168,6 @@ public void testProtectIndexScan() { mappings), exclude), newEvalField), - sortCount, sortField), CommandType.TOP, topExprs, @@ -192,7 +190,6 @@ public void testProtectSortForWindowOperator() { resourceMonitor( sort( values(emptyList()), - 0, sortItem)), rank, windowDefinition), @@ -200,7 +197,6 @@ public void testProtectSortForWindowOperator() { window( sort( values(emptyList()), - 0, sortItem ), rank, diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java index 95a56e5cf4..445334298b 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/planner/logical/ElasticsearchLogicOptimizerTest.java @@ -166,7 +166,6 @@ void sort_merge_with_relation() { optimize( sort( relation("schema"), - 0, Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER)) ) ) @@ -185,7 +184,6 @@ void sort_merge_with_indexScan() { optimize( sort( indexScan("schema", Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("intV", INTEGER))), - 0, Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) ) ) @@ -208,7 +206,6 @@ void sort_filter_merge_with_relation() { relation("schema"), dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) ), - 0, Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) ) ) @@ -220,13 +217,11 @@ void sort_with_expression_cannot_merge_with_relation() { assertEquals( sort( relation("schema"), - 0, Pair.of(Sort.SortOption.DEFAULT_ASC, dsl.abs(DSL.ref("intV", INTEGER))) ), optimize( sort( relation("schema"), - 0, Pair.of(Sort.SortOption.DEFAULT_ASC, dsl.abs(DSL.ref("intV", INTEGER))) ) ) diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java index a54695b374..e4835201f2 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexTest.java @@ -190,7 +190,6 @@ void implementOtherLogicalOperators() { mappings), exclude), newEvalField), - sortCount, sortField), dedupeField), include); @@ -208,7 +207,6 @@ void implementOtherLogicalOperators() { mappings), exclude), newEvalField), - sortCount, sortField), dedupeField), include), 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 7e133bb24e..d60aa5f853 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,6 @@ { "name": "SortOperator", "description": { - "count": 1000, "sortList": { "state": { "sortOrder": "ASC", diff --git a/ppl/src/main/antlr/OpenDistroPPLParser.g4 b/ppl/src/main/antlr/OpenDistroPPLParser.g4 index f04b21b2da..cba8b99287 100644 --- a/ppl/src/main/antlr/OpenDistroPPLParser.g4 +++ b/ppl/src/main/antlr/OpenDistroPPLParser.g4 @@ -68,7 +68,7 @@ dedupCommand ; sortCommand - : SORT (count=integerLiteral)? sortbyClause (D | DESC)? + : SORT sortbyClause ; evalCommand diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java index 3f094bfab7..abe8680175 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilder.java @@ -32,11 +32,8 @@ import static com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.WhereCommandContext; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.DataType; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Let; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.Literal; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Map; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Aggregation; @@ -52,7 +49,6 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; -import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser; import com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.ByClauseContext; import com.amazon.opendistroforelasticsearch.sql.ppl.antlr.parser.OpenDistroPPLParser.FieldListContext; @@ -203,7 +199,6 @@ public UnresolvedPlan visitHeadCommand(HeadCommandContext ctx) { @Override public UnresolvedPlan visitSortCommand(SortCommandContext ctx) { return new Sort( - ArgumentFactory.getArgumentList(ctx), ctx.sortbyClause() .sortField() .stream() diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java index 872e14c9dc..b4dbf841db 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactory.java @@ -122,23 +122,6 @@ public static List getArgumentList(HeadCommandContext ctx, ); } - /** - * Get list of {@link Argument}. - * - * @param ctx SortCommandContext instance - * @return the list of arguments fetched from the sort command - */ - public static List getArgumentList(SortCommandContext ctx) { - return Arrays.asList( - ctx.count != null - ? new Argument("count", getArgumentValue(ctx.count)) - : new Argument("count", new Literal(1000, DataType.INTEGER)), - ctx.D() != null || ctx.DESC() != null - ? new Argument("desc", new Literal(true, DataType.BOOLEAN)) - : new Argument("desc", new Literal(false, DataType.BOOLEAN)) - ); - } - /** * Get list of {@link Argument}. * diff --git a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizer.java b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizer.java index 5a20beab77..4595e017c9 100644 --- a/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizer.java +++ b/ppl/src/main/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizer.java @@ -186,9 +186,8 @@ public String visitEval(Eval node, String context) { public String visitSort(Sort node, String context) { String child = node.getChild().get(0).accept(this, context); // the first options is {"count": "integer"} - Integer count = (Integer) node.getOptions().get(0).getValue().getValue(); String sortList = visitFieldList(node.getSortList()); - return StringUtils.format("%s | sort %d %s", child, count, sortList); + return StringUtils.format("%s | sort %s", child, sortList); } /** diff --git a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java index ac174d9019..edb3df6eeb 100644 --- a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstBuilderTest.java @@ -26,7 +26,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultFieldsArgs; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultHeadArgs; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortFieldArgs; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortOptions; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultStatsArgs; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.eval; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.exprList; @@ -43,7 +42,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.rename; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.sort; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.sortOptions; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.stringLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.unresolvedArg; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.unresolvedArgList; @@ -331,7 +329,6 @@ public void testSortCommand() { assertEqual("source=t | sort f1, f2", sort( relation("t"), - defaultSortOptions(), field("f1", defaultSortFieldArgs()), field("f2", defaultSortFieldArgs()) )); @@ -339,10 +336,9 @@ public void testSortCommand() { @Test public void testSortCommandWithOptions() { - assertEqual("source=t | sort 100 - f1, + f2", + assertEqual("source=t | sort - f1, + f2", sort( relation("t"), - sortOptions(100), field("f1", exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), field("f2", defaultSortFieldArgs()) diff --git a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilderTest.java index c8a119d9eb..b8961a9054 100644 --- a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilderTest.java +++ b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/parser/AstExpressionBuilderTest.java @@ -24,7 +24,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.compare; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultFieldsArgs; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortFieldArgs; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortOptions; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultStatsArgs; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.doubleLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.equalTo; @@ -40,7 +39,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.not; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.nullLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.or; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.project; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.projectWithArg; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.qualifiedName; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.relation; @@ -216,7 +214,6 @@ public void testFieldExpr() { assertEqual("source=t | sort + f", sort( relation("t"), - defaultSortOptions(), field("f", defaultSortFieldArgs()) )); } @@ -226,7 +223,6 @@ public void testSortFieldWithMinusKeyword() { assertEqual("source=t | sort - f", sort( relation("t"), - defaultSortOptions(), field( "f", argument("asc", booleanLiteral(false)), @@ -240,7 +236,6 @@ public void testSortFieldWithAutoKeyword() { assertEqual("source=t | sort auto(f)", sort( relation("t"), - defaultSortOptions(), field( "f", argument("asc", booleanLiteral(true)), @@ -254,7 +249,6 @@ public void testSortFieldWithIpKeyword() { assertEqual("source=t | sort ip(f)", sort( relation("t"), - defaultSortOptions(), field( "f", argument("asc", booleanLiteral(true)), @@ -268,7 +262,6 @@ public void testSortFieldWithNumKeyword() { assertEqual("source=t | sort num(f)", sort( relation("t"), - defaultSortOptions(), field( "f", argument("asc", booleanLiteral(true)), @@ -282,7 +275,6 @@ public void testSortFieldWithStrKeyword() { assertEqual("source=t | sort str(f)", sort( relation("t"), - defaultSortOptions(), field( "f", argument("asc", booleanLiteral(true)), diff --git a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactoryTest.java b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactoryTest.java index 9e8512fce8..953af4933e 100644 --- a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactoryTest.java +++ b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/ArgumentFactoryTest.java @@ -21,8 +21,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.argument; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.booleanLiteral; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.dedupe; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortFieldArgs; -import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.defaultSortOptions; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.exprList; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.field; import static com.amazon.opendistroforelasticsearch.sql.ast.dsl.AstDSL.intLiteral; @@ -103,24 +101,10 @@ public void testDedupCommandDefaultArgument() { ); } - @Test - public void testSortCommandArgument() { - assertEqual("source=t | sort 3 field0 desc", - sort( - relation("t"), - exprList( - argument("count", intLiteral(3)), - argument("desc", booleanLiteral(true)) - ), - field("field0", defaultSortFieldArgs()) - )); - assertEqual("source=t | sort 3 field0 d", "source=t | sort 3 field0 desc"); - } - @Test public void testSortCommandDefaultArgument() { assertEqual( - "source=t | sort 1000 field0", + "source=t | sort field0", "source=t | sort field0" ); } @@ -130,7 +114,6 @@ public void testSortFieldArgument() { assertEqual("source=t | sort - auto(field0)", sort( relation("t"), - defaultSortOptions(), field( "field0", exprList( diff --git a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java index 403c234c0b..91b14172c6 100644 --- a/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java +++ b/ppl/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java @@ -99,8 +99,8 @@ public void testHeadCommandWithWhileExpr() { //todo, sort order is ignored, it doesn't impact the log analysis. @Test public void testSortCommandWithOptions() { - assertEquals("source=t | sort 100 f1,f2", - anonymize("source=t | sort 100 - f1, + f2")); + assertEquals("source=t | sort f1,f2", + anonymize("source=t | sort - f1, + f2")); } @Test diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java index 880c8a9bb8..e14c3abd26 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstSortBuilder.java @@ -17,7 +17,6 @@ package com.amazon.opendistroforelasticsearch.sql.sql.parser; import static com.amazon.opendistroforelasticsearch.sql.ast.expression.DataType.BOOLEAN; -import static com.amazon.opendistroforelasticsearch.sql.ast.expression.DataType.INTEGER; import static com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.OrderByClauseContext; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; @@ -46,7 +45,6 @@ public class AstSortBuilder extends OpenDistroSQLParserBaseVisitor Date: Mon, 23 Nov 2020 09:09:13 -0800 Subject: [PATCH 3/6] add filter push down rule --- .../optimizer/LogicalPlanOptimizer.java | 4 +- .../optimizer/rule/PushFilterUnderSort.java | 63 +++++++++++++++++++ .../optimizer/LogicalPlanOptimizerTest.java | 59 +++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/rule/PushFilterUnderSort.java diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizer.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizer.java index 254dd1b1ad..daae3dfaf9 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizer.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizer.java @@ -22,6 +22,7 @@ import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.rule.MergeFilterAndFilter; +import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.rule.PushFilterUnderSort; import com.facebook.presto.matching.Match; import java.util.Arrays; import java.util.List; @@ -50,7 +51,8 @@ public LogicalPlanOptimizer(List> rules) { */ public static LogicalPlanOptimizer create(DSL dsl) { return new LogicalPlanOptimizer(Arrays.asList( - new MergeFilterAndFilter(dsl))); + new MergeFilterAndFilter(dsl), + new PushFilterUnderSort())); } /** diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/rule/PushFilterUnderSort.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/rule/PushFilterUnderSort.java new file mode 100644 index 0000000000..44738b518d --- /dev/null +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/rule/PushFilterUnderSort.java @@ -0,0 +1,63 @@ +/* + * + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.planner.optimizer.rule; + +import static com.amazon.opendistroforelasticsearch.sql.planner.optimizer.pattern.Patterns.source; +import static com.facebook.presto.matching.Pattern.typeOf; + +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalFilter; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalSort; +import com.amazon.opendistroforelasticsearch.sql.planner.optimizer.Rule; +import com.facebook.presto.matching.Capture; +import com.facebook.presto.matching.Captures; +import com.facebook.presto.matching.Pattern; +import lombok.Getter; +import lombok.experimental.Accessors; + +/** + * Push Filter under Sort. + * Filter - Sort - Child --> Sort - Filter - Child + */ +public class PushFilterUnderSort implements Rule { + + private final Capture capture; + + @Accessors(fluent = true) + @Getter + private final Pattern pattern; + + /** + * Constructor of PushFilterUnderSort. + */ + public PushFilterUnderSort() { + this.capture = Capture.newCapture(); + this.pattern = typeOf(LogicalFilter.class) + .with(source().matching(typeOf(LogicalSort.class).capturedAs(capture))); + } + + @Override + public LogicalPlan apply(LogicalFilter filter, + Captures captures) { + LogicalSort sort = captures.get(capture); + return new LogicalSort( + filter.replaceChildPlans(sort.getChild()), + sort.getSortList() + ); + } +} diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java index e68d17b0ff..fcf861f46b 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java @@ -18,15 +18,20 @@ package com.amazon.opendistroforelasticsearch.sql.planner.optimizer; import static com.amazon.opendistroforelasticsearch.sql.data.model.ExprValueUtils.integerValue; +import static com.amazon.opendistroforelasticsearch.sql.data.model.ExprValueUtils.longValue; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; +import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.LONG; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.filter; import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.relation; +import static com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlanDSL.sort; import static org.junit.jupiter.api.Assertions.assertEquals; import com.amazon.opendistroforelasticsearch.sql.analysis.AnalyzerTestBase; +import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import com.amazon.opendistroforelasticsearch.sql.planner.logical.LogicalPlan; +import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.context.annotation.Configuration; @@ -60,6 +65,60 @@ void filter_merge_filter() { ); } + /** + * Filter - Sort --> Sort - Filter. + */ + @Test + void push_filter_under_sort() { + assertEquals( + sort( + filter( + relation("schema"), + dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) + ), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ), + optimize( + filter( + sort( + relation("schema"), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ), + dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) + ) + ) + ); + } + + /** + * Filter - Sort --> Sort - Filter. + */ + @Test + void multiple_filter_should_eventually_be_merged() { + assertEquals( + sort( + filter( + relation("schema"), + dsl.and(dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))), + dsl.less(DSL.ref("longV", INTEGER), DSL.literal(longValue(1L)))) + ), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ), + optimize( + filter( + sort( + filter( + relation("schema"), + dsl.less(DSL.ref("longV", INTEGER), DSL.literal(longValue(1L))) + ), + Pair.of(Sort.SortOption.DEFAULT_ASC, DSL.ref("longV", LONG)) + ), + dsl.equal(DSL.ref("intV", INTEGER), DSL.literal(integerValue(1))) + ) + ) + ); + } + private LogicalPlan optimize(LogicalPlan plan) { final LogicalPlanOptimizer optimizer = LogicalPlanOptimizer.create(dsl); final LogicalPlan optimize = optimizer.optimize(plan); From cd68598218646f3bdedb4b0fd080679d5086a611 Mon Sep 17 00:00:00 2001 From: penghuo Date: Mon, 23 Nov 2020 14:14:02 -0800 Subject: [PATCH 4/6] fix compile issue --- .../opendistroforelasticsearch/sql/analysis/AnalyzerTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java index 20f2bdfbe9..0548cb683d 100644 --- a/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/com/amazon/opendistroforelasticsearch/sql/analysis/AnalyzerTest.java @@ -351,13 +351,11 @@ public void sort_with_options() { LogicalPlanDSL.project( LogicalPlanDSL.sort( LogicalPlanDSL.relation("test"), - 0, Pair.of(expectOption, DSL.ref("integer_value", INTEGER))), DSL.named("string_value", DSL.ref("string_value", STRING))), AstDSL.project( AstDSL.sort( AstDSL.relation("test"), - ImmutableList.of(argument("count", intLiteral(0))), field(qualifiedName("integer_value"), args)), AstDSL.alias("string_value", qualifiedName("string_value"))))); } From b1373771762c3f775b893840bb102f5b5593931b Mon Sep 17 00:00:00 2001 From: penghuo Date: Mon, 23 Nov 2020 14:33:58 -0800 Subject: [PATCH 5/6] add integ-test --- .../sql/ppl/ExplainIT.java | 17 +++++++++++++++++ .../expectedOutput/ppl/explain_sort_push.json | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 integ-test/src/test/resources/expectedOutput/ppl/explain_sort_push.json diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java index 4970ee1a63..79b95f9f05 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java @@ -76,6 +76,23 @@ public void testFilterAndAggPushDownExplain() throws Exception { ); } + @Test + public void testSortPushDownExplain() throws Exception { + String expected = loadFromFile("expectedOutput/ppl/explain_filter_agg_push.json"); + + String actual = explainQueryToString( + "source=elasticsearch-sql_test_index_account" + + "| sort age " + + "| where age > 30"); + assertJsonEquals( + expected, + explainQueryToString( + "source=elasticsearch-sql_test_index_account" + + "| sort age " + + "| where age > 30") + ); + } + String loadFromFile(String filename) throws Exception { URI uri = Resources.getResource(filename).toURI(); return new String(Files.readAllBytes(Paths.get(uri))); diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_sort_push.json b/integ-test/src/test/resources/expectedOutput/ppl/explain_sort_push.json new file mode 100644 index 0000000000..8b15ab493d --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_sort_push.json @@ -0,0 +1,17 @@ +{ + "root": { + "name": "ProjectOperator", + "description": { + "fields": "[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname]" + }, + "children": [ + { + "name": "ElasticsearchIndexScan", + "description": { + "request": "ElasticsearchQueryRequest(indexName\u003delasticsearch-sql_test_index_account, sourceBuilder\u003d{\"from\":0,\"size\":200,\"timeout\":\"1m\",\"query\":{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},\"sort\":[{\"age\":{\"order\":\"asc\",\"missing\":\"_first\"}}]}, searchDone\u003dfalse)" + }, + "children": [] + } + ] + } +} \ No newline at end of file From f537a5ee9d7dc3c3f200205d265b1b0a76c48896 Mon Sep 17 00:00:00 2001 From: penghuo Date: Mon, 23 Nov 2020 15:35:16 -0800 Subject: [PATCH 6/6] update --- .../amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java | 1 - .../sql/planner/logical/LogicalSort.java | 2 -- .../sql/planner/physical/SortOperator.java | 1 - .../elasticsearch/storage/script/core/ExpressionScript.java | 3 --- .../amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java | 2 +- 5 files changed, 1 insertion(+), 8 deletions(-) diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java index 485048746d..50e71eca42 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/ast/tree/Sort.java @@ -21,7 +21,6 @@ import static com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.SortOrder.DESC; import com.amazon.opendistroforelasticsearch.sql.ast.AbstractNodeVisitor; -import com.amazon.opendistroforelasticsearch.sql.ast.expression.Argument; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Field; import com.google.common.collect.ImmutableList; import java.util.List; diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java index 2354eb128c..5f2855ec66 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/logical/LogicalSort.java @@ -17,12 +17,10 @@ import com.amazon.opendistroforelasticsearch.sql.ast.tree.Sort.SortOption; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; -import java.util.Arrays; import java.util.Collections; import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.RequiredArgsConstructor; import lombok.ToString; import org.apache.commons.lang3.tuple.Pair; diff --git a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java index a1b639bd75..8216c7f45c 100644 --- a/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java +++ b/core/src/main/java/com/amazon/opendistroforelasticsearch/sql/planner/physical/SortOperator.java @@ -23,7 +23,6 @@ import com.amazon.opendistroforelasticsearch.sql.data.utils.ExprValueOrdering; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; import com.amazon.opendistroforelasticsearch.sql.planner.physical.SortOperator.Sorter.SorterBuilder; -import com.google.common.collect.Iterators; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java index 3a8a0356e9..9087ef3d5f 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java @@ -19,11 +19,8 @@ import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.FLOAT; import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; -import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD; -import static java.util.stream.Collectors.reducing; import static java.util.stream.Collectors.toMap; -import com.amazon.opendistroforelasticsearch.sql.data.model.ExprMissingValue; import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue; import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value.ElasticsearchExprValueFactory; diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java index 79b95f9f05..a0847773df 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/ExplainIT.java @@ -78,7 +78,7 @@ public void testFilterAndAggPushDownExplain() throws Exception { @Test public void testSortPushDownExplain() throws Exception { - String expected = loadFromFile("expectedOutput/ppl/explain_filter_agg_push.json"); + String expected = loadFromFile("expectedOutput/ppl/explain_sort_push.json"); String actual = explainQueryToString( "source=elasticsearch-sql_test_index_account"