Skip to content

ESQL: allow sorting by expressions and not only regular fields #107158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
astefan merged 9 commits into from
Apr 9, 2024
Merged

ESQL: allow sorting by expressions and not only regular fields #107158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6ec60c9
Support expressions in sort command
astefan Apr 4, 2024
0beb8ff
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Apr 5, 2024
491e7c6
Update docs/changelog/107158.yaml
astefan Apr 5, 2024
09b2f0a
add skips for bwc tests
astefan Apr 5, 2024
bf7d297
Merge branch 'sort_expressions' of https://github.com/astefan/elastic…
astefan Apr 5, 2024
6306311
Another skip
astefan Apr 5, 2024
2b8affb
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Apr 9, 2024
825b5fc
Address reviews
astefan Apr 9, 2024
ad6a635
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/107158.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 107158
summary: "ESQL: allow sorting by expressions and not only regular fields"
area: ES|QL
type: feature
issues: []
66 changes: 66 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,3 +424,69 @@ emp_no:i
-10002
-10003
;

sortExpression1#[skip:-8.13.99,reason:supported in 8.14]
FROM employees
| SORT emp_no + salary ASC
| EVAL emp_no = -emp_no
| LIMIT 10
| EVAL sum = -emp_no + salary
| KEEP emp_no, salary, sum
;

emp_no:i | salary:i | sum:i
-10015 |25324 |35339
-10035 |25945 |35980
-10092 |25976 |36068
-10048 |26436 |36484
-10057 |27215 |37272
-10084 |28035 |38119
-10026 |28336 |38362
-10068 |28941 |39009
-10060 |29175 |39235
-10042 |30404 |40446
;

sortConcat1#[skip:-8.13.99,reason:supported in 8.14]
from employees
| sort concat(left(last_name, 1), left(first_name, 1)), salary desc
| keep first_name, last_name, salary
| eval ll = left(last_name, 1), lf = left(first_name, 1)
| limit 10
;

first_name:keyword | last_name:keyword | salary:integer|ll:keyword|lf:keyword
Mona |Azuma |46595 |A |M
Satosi |Awdeh |50249 |A |S
Brendon |Bernini |33370 |B |B
Breannda |Billingsley |29175 |B |B
Cristinel |Bouloucos |58715 |B |C
Charlene |Brattka |28941 |B |C
Margareta |Bierman |41933 |B |M
Mokhtar |Bernatsky |38992 |B |M
Parto |Bamford |61805 |B |P
Premal |Baek |52833 |B |P
;

sortConcat2#[skip:-8.13.99,reason:supported in 8.14]
from employees
| eval ln = last_name, fn = first_name, concat = concat(left(last_name, 1), left(first_name, 1))
| sort concat(left(ln, 1), left(fn, 1)), salary desc
| keep f*, l*, salary
| eval c = concat(left(last_name, 1), left(first_name, 1))
| drop *name, lan*
| limit 10
;

fn:keyword | ln:keyword | salary:integer| c:keyword
Mona |Azuma |46595 |AM
Satosi |Awdeh |50249 |AS
Brendon |Bernini |33370 |BB
Breannda |Billingsley |29175 |BB
Cristinel |Bouloucos |58715 |BC
Charlene |Brattka |28941 |BC
Margareta |Bierman |41933 |BM
Mokhtar |Bernatsky |38992 |BM
Parto |Bamford |61805 |BP
Premal |Baek |52833 |BP
;
Copy link
Copy Markdown
Contributor

@alex-spies alex-spies Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a test with two subesequent sorts with expressions for their groupbys can't hurt, esp. if they use the same expressions:

...
| sort a + b, c
| eval d = c
| sort a + b, d

This could have interesting interactions with var. shadowing and some pruning opt. rules.

Copy link
Copy Markdown
Contributor Author

@astefan astefan Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing spectacular since both sorts are merged and they are identified as the same, but I've added a test to LogicalPlanOptimizerTests.

24 changes: 24 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1585,3 +1585,27 @@ c:l | k1:i | languages:i
21 | 5 | 5
10 | null | null
;

minWithSortExpression1#[skip:-8.13.99,reason:supported in 8.14]
FROM employees | STATS min = min(salary) by languages | SORT min + languages;

min:i | languages:i
25324 |5
25976 |1
26436 |3
27215 |4
29175 |2
28336 |null
;

minWithSortExpression2#[skip:-8.13.99,reason:supported in 8.14]
FROM employees | STATS min = min(salary) by languages | SORT min + CASE(languages == 5, 655, languages);

min:i | languages:i
25976 |1
25324 |5
26436 |3
27215 |4
29175 |2
28336 |null
;
33 changes: 32 additions & 1 deletion ...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import static java.util.Arrays.asList;
import static java.util.Collections.singleton;
import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputExpressions;
import static org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizer.SubstituteSurrogates.rawTemporaryName;
import static org.elasticsearch.xpack.ql.expression.Expressions.asAttributes;
import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.TransformDirection;
import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.TransformDirection.DOWN;
Expand Down Expand Up @@ -125,7 +126,8 @@ protected static Batch<LogicalPlan> substitutions() {
new ReplaceRegexMatch(),
new ReplaceAliasingEvalWithProject(),
new SkipQueryOnEmptyMappings(),
new SubstituteSpatialSurrogates()
new SubstituteSpatialSurrogates(),
new ReplaceOrderByExpressionWithEval()
Copy link
Copy Markdown
Contributor

@alex-spies alex-spies Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: There's 4 places now that take expressions out of a plan node and turn them into evals:

  • ReplaceStatsNestedExpressionWithEval and this create an Eval before the node.
  • ReplaceStatsAggExpressionWithEval and SubstituteSurrogates create an Eval after the node.

I wonder if this should be consolidated into 2 opt. rules, or at least should use common helper methods.

It's probably fine for now, though.

Copy link
Copy Markdown
Contributor Author

@astefan astefan Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you in that we should look into the possibility of refactoring this. But this should probably be done separately under the umbrella of "refactoring" or maybe "tech debt".

// new NormalizeAggregate(), - waits on https://github.com/elastic/elasticsearch/issues/100634
);
}
Expand Down Expand Up @@ -321,6 +323,35 @@ protected SpatialRelatesFunction rule(SpatialRelatesFunction function) {
}
}

static class ReplaceOrderByExpressionWithEval extends OptimizerRules.OptimizerRule<OrderBy> {
private static int counter = 0;

@Override
protected LogicalPlan rule(OrderBy orderBy) {
int size = orderBy.order().size();
List<Alias> evals = new ArrayList<>(size);
List<Order> newOrders = new ArrayList<>(size);

for (int i = 0; i < size; i++) {
var order = orderBy.order().get(i);
if (order.child() instanceof Attribute == false) {
var name = rawTemporaryName("order_by", String.valueOf(i), String.valueOf(counter++));
var eval = new Alias(order.child().source(), name, order.child());
newOrders.add(order.replaceChildren(List.of(eval.toAttribute())));
evals.add(eval);
} else {
newOrders.add(order);
}
}
if (evals.isEmpty()) {
return orderBy;
} else {
var newOrderBy = new OrderBy(orderBy.source(), new Eval(orderBy.source(), orderBy.child(), evals), newOrders);
return new Project(orderBy.source(), newOrderBy, orderBy.output());
}
}
}

static class ConvertStringToByteRef extends OptimizerRules.OptimizerExpressionRule<Literal> {

ConvertStringToByteRef() {
Expand Down
Loading