Skip to content

Commit

Permalink
Fix combination of SORT commands (elastic#102307) (elastic#102309)
Browse files Browse the repository at this point in the history 
last one just overrides the others
  • Loading branch information
@luigidellaquila
luigidellaquila authored Nov 16, 2023
1 parent eacb881 commit d5c20b4
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 31 deletions.
2 changes: 1 addition & 1 deletion ...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@ protected LogicalPlan rule(OrderBy orderBy) {

if (child instanceof OrderBy childOrder) {
// combine orders
return new OrderBy(orderBy.source(), childOrder.child(), CollectionUtils.combine(orderBy.order(), childOrder.order()));
return new OrderBy(orderBy.source(), childOrder.child(), orderBy.order());
} else if (child instanceof Project) {
return pushDownPastProject(orderBy);
}
Expand Down
42 changes: 12 additions & 30 deletions .../esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ public void testCombineOrderBy() {
| sort salary""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("salary", "emp_no"));
assertThat(orderNames(topN), contains("salary"));
as(topN.child(), EsRelation.class);
}

Expand All @@ -791,7 +791,7 @@ public void testCombineOrderByThroughEval() {
| sort x""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("x", "emp_no"));
assertThat(orderNames(topN), contains("x"));
var eval = as(topN.child(), Eval.class);
as(eval.child(), EsRelation.class);
}
Expand All @@ -805,7 +805,7 @@ public void testCombineOrderByThroughEvalWithTwoDefs() {
| sort z""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("z", "emp_no"));
assertThat(orderNames(topN), contains("z"));
var eval = as(topN.child(), Eval.class);
assertThat(Expressions.names(eval.fields()), contains("x", "y", "z"));
as(eval.child(), EsRelation.class);
Expand All @@ -819,7 +819,7 @@ public void testCombineOrderByThroughDissect() {
| sort x""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("x", "emp_no"));
assertThat(orderNames(topN), contains("x"));
var dissect = as(topN.child(), Dissect.class);
as(dissect.child(), EsRelation.class);
}
Expand All @@ -832,7 +832,7 @@ public void testCombineOrderByThroughGrok() {
| sort x""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("x", "emp_no"));
assertThat(orderNames(topN), contains("x"));
var grok = as(topN.child(), Grok.class);
as(grok.child(), EsRelation.class);
}
Expand All @@ -846,7 +846,7 @@ public void testCombineOrderByThroughProject() {

var keep = as(plan, Project.class);
var topN = as(keep.child(), TopN.class);
assertThat(orderNames(topN), contains("salary", "emp_no"));
assertThat(orderNames(topN), contains("salary"));
as(topN.child(), EsRelation.class);
}

Expand All @@ -861,7 +861,7 @@ public void testCombineOrderByThroughProjectAndEval() {

var keep = as(plan, Project.class);
var topN = as(keep.child(), TopN.class);
assertThat(orderNames(topN), contains("salary", "emp_no"));
assertThat(orderNames(topN), contains("salary"));
var eval = as(topN.child(), Eval.class);
assertThat(Expressions.names(eval.fields()), contains("e"));
as(eval.child(), EsRelation.class);
Expand All @@ -877,7 +877,7 @@ public void testCombineOrderByThroughProjectWithAlias() {

var keep = as(plan, Project.class);
var topN = as(keep.child(), TopN.class);
assertThat(orderNames(topN), contains("salary", "emp_no"));
assertThat(orderNames(topN), contains("salary"));
as(topN.child(), EsRelation.class);
}

Expand All @@ -889,7 +889,7 @@ public void testCombineOrderByThroughFilter() {
| sort salary""");

var topN = as(plan, TopN.class);
assertThat(orderNames(topN), contains("salary", "emp_no"));
assertThat(orderNames(topN), contains("salary"));
var filter = as(topN.child(), Filter.class);
as(filter.child(), EsRelation.class);
}
Expand Down Expand Up @@ -997,7 +997,7 @@ public void testMultipleMvExpandWithSortAndLimit() {
var keep = as(plan, EsqlProject.class);
var topN = as(keep.child(), TopN.class);
assertThat(topN.limit().fold(), equalTo(5));
assertThat(orderNames(topN), contains("salary", "first_name"));
assertThat(orderNames(topN), contains("salary"));
var limit = as(topN.child(), Limit.class);
assertThat(limit.limit().fold(), equalTo(5));
var mvExp = as(limit.child(), MvExpand.class);
Expand Down Expand Up @@ -1312,10 +1312,10 @@ public void testCombineMultipleOrderByAndLimits() {

var keep = as(plan, Project.class);
var topN = as(keep.child(), TopN.class);
assertThat(orderNames(topN), contains("emp_no", "first_name"));
assertThat(orderNames(topN), contains("emp_no"));
var filter = as(topN.child(), Filter.class);
var topN2 = as(filter.child(), TopN.class);
assertThat(orderNames(topN2), contains("salary", "emp_no"));
assertThat(orderNames(topN2), contains("salary"));
as(topN2.child(), EsRelation.class);
}

Expand Down Expand Up @@ -1356,12 +1356,6 @@ public void testDontPruneSameFieldDifferentDirectionSortClauses() {
new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")),
Order.OrderDirection.DESC,
Order.NullsPosition.FIRST
),
new Order(
EMPTY,
new FieldAttribute(EMPTY, "salary", mapping.get("salary")),
Order.OrderDirection.ASC,
Order.NullsPosition.LAST
)
)
);
Expand Down Expand Up @@ -1405,12 +1399,6 @@ public void testPruneRedundantSortClauses() {
new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")),
Order.OrderDirection.DESC,
Order.NullsPosition.LAST
),
new Order(
EMPTY,
new FieldAttribute(EMPTY, "salary", mapping.get("salary")),
Order.OrderDirection.DESC,
Order.NullsPosition.LAST
)
)
);
Expand All @@ -1435,12 +1423,6 @@ public void testDontPruneSameFieldDifferentDirectionSortClauses_UsingAlias() {
new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")),
Order.OrderDirection.ASC,
Order.NullsPosition.LAST
),
new Order(
EMPTY,
new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")),
Order.OrderDirection.DESC,
Order.NullsPosition.FIRST
)
)
);
Expand Down

0 comments on commit d5c20b4

Please sign in to comment.