ESQL: allow sorting by expressions and not only regular fields by astefan · Pull Request #107158 · elastic/elasticsearch

astefan · 2024-04-05T14:18:54Z

This allows ESQL to support sort commands over complex expressions like sort salary + languages or sort concat(left(last_name, 1), left(first_name, 1)), salary desc.

…sort_expressions

elasticsearchmachine · 2024-04-05T14:19:18Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-04-05T14:19:18Z

Hi @astefan, I've created a changelog YAML for you.

astefan · 2024-04-05T14:22:19Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+     *     \_Eval[[emp_no{f}#36 + salary{f}#41 * 13[INTEGER] AS $$order_by$temp_name$0, NEG(salary{f}#41) AS $$order_by$temp_name$1]]
     *       \_EsRelation[test][_meta_field{f}#42, emp_no{f}#36, first_name{f}#37, ..]
     */
    public void testPushdownWithOverwrittenName() {


Interestingly, we already supported expressions in sort but not in all cases, thanks to the work @alex-spies has done in #105650. I have kept his original test, but changed it so that the sort is on individual fields (notice below the sort command that I changed to SORT emp_no ASC nulls first, salary DESC nulls last, emp_no).

astefan · 2024-04-05T14:23:39Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+     *     \_Eval[[emp_no{f}#36 + salary{f}#41 * 13[INTEGER] AS $$order_by$temp_name$0, NEG(salary{f}#41) AS $$order_by$temp_name$1]]
+     *       \_EsRelation[test][_meta_field{f}#42, emp_no{f}#36, first_name{f}#37, ..]
+     */
+    public void testReplaceSortByExpressions() {


The original queries from testPushdownWithOverwrittenName in #105650 are now used to prove the functionality added by this PR is sound.

…search into sort_expressions

costin

LGTM

costin · 2024-04-05T18:17:58Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+
+            for (Order order : orderBy.order()) {
+                if (order.child() instanceof Attribute == false) {
+                    var name = rawTemporaryName("order_by", "temp_name", String.valueOf(counter++));


Maybe find a better name than "temp_name" - such as the ordinal of the order it tries to replace?

alex-spies

LGTM; only left minor comments, but this would be good-to-go as-is IMHO.

alex-spies · 2024-04-08T08:03:01Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

            new SkipQueryOnEmptyMappings(),
-            new SubstituteSpatialSurrogates()
+            new SubstituteSpatialSurrogates(),
+            new ReplaceOrderByExpressionWithEval()


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.

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".

alex-spies · 2024-04-08T08:07:40Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+                        new org.elasticsearch.xpack.esql.expression.Order(
+                            order.source(),
+                            eval.toAttribute(),
+                            order.direction(),
+                            order.nullsPosition()


Couldn't this just be order.replaceChildren(List.of(eval.toAttribute()))?

alex-spies · 2024-04-08T08:13:19Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec

+from employees
+| sort concat(left(last_name, 1), left(first_name, 1)), salary desc
+| keep first_name, last_name, salary
+| eval c = concat(left(last_name, 1), left(first_name, 1))


That's pretty paranoid, but maybe it's better if we just have

Suggested change

| eval c = concat(left(last_name, 1), left(first_name, 1))

| eval l = left(last_name, 1, r = left(first_name, 1)

Reason: in the future, c might be eliminated by common subexpression elimination and we end up testing a different opt. rule.

Yeah, it's pretty far fetched, but I don't mind changing this. The spirit of the test remains the same with this suggestion.

alex-spies · 2024-04-08T08:22:39Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+
+        @Override
+        protected LogicalPlan rule(OrderBy orderBy) {
+            int counter = 0;


I wonder if this counter maybe should be a (static?) class member instead of a local var?

I could see multiple subsequent sorts accidentally shadowing each other's temporary evals in some edge cases/weird interactions with other opt. rules - by making the name+counter essentially unique, we'd be avoiding this.

alex-spies · 2024-04-08T08:47:14Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+            assertThat(firstOrderExpr.direction(), equalTo(org.elasticsearch.xpack.ql.expression.Order.OrderDirection.ASC));
+            assertThat(firstOrderExpr.nullsPosition(), equalTo(org.elasticsearch.xpack.ql.expression.Order.NullsPosition.LAST));
+            var renamedEmpNoSalaryExpression = as(firstOrderExpr.child(), ReferenceAttribute.class);
+            assertThat(renamedEmpNoSalaryExpression.toString(), startsWith("$$order_by$temp_name$0"));


This doesn't assert the expression that defines $$order_by$temp_name$0 anymore; I think we should add these asserts, otherwise we only test the correctness of the sort but not of the eval after the opt. rule has run. Same in testReplaceSortByExpressions

alex-spies · 2024-04-08T08:50:46Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec

+Mokhtar             |Bernatsky          |38992          |BM             
+Parto               |Bamford            |61805          |BP             
+Premal              |Baek               |52833          |BP
+;


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.

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

…sort_expressions

drewdaemon · 2024-04-26T20:18:24Z

Great improvement! However, I don't see this in the docs. Is an update there planned?

## Summary `SORT` accepts expressions as of elastic/elasticsearch#107158. This PR updates the validator and autocompletion engine to account for this improvement. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added — looks like docs haven't been added for this feature yet. I opened the discussion with ES team ([ref](elastic/elasticsearch#107158 (comment))) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary `SORT` accepts expressions as of elastic/elasticsearch#107158. This PR updates the validator and autocompletion engine to account for this improvement. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added — looks like docs haven't been added for this feature yet. I opened the discussion with ES team ([ref](elastic/elasticsearch#107158 (comment))) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 5ba6a39)

# Backport This will backport the following commits from `main` to `8.14`: - [[ES|QL] Sorting accepts expressions (#181916)](#181916)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Drew Tate <drew.tate@elastic.co>

astefan added 2 commits April 5, 2024 16:27

Support expressions in sort command

6ec60c9

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

0beb8ff

…sort_expressions

astefan added >feature :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI v8.14.0 labels Apr 5, 2024

astefan requested review from alex-spies and costin April 5, 2024 14:18

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 5, 2024

Update docs/changelog/107158.yaml

491e7c6

astefan commented Apr 5, 2024

View reviewed changes

astefan added 3 commits April 5, 2024 18:14

add skips for bwc tests

09b2f0a

Merge branch 'sort_expressions' of https://github.com/astefan/elastic…

bf7d297

…search into sort_expressions

Another skip

6306311

costin approved these changes Apr 5, 2024

View reviewed changes

alex-spies approved these changes Apr 8, 2024

View reviewed changes

astefan added 3 commits April 9, 2024 15:41

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

2b8affb

…sort_expressions

Address reviews

825b5fc

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

ad6a635

…sort_expressions

astefan merged commit 31c05e9 into elastic:main Apr 9, 2024

astefan deleted the sort_expressions branch April 9, 2024 15:57

drewdaemon mentioned this pull request Apr 26, 2024

[ES|QL] Sorting accepts expressions elastic/kibana#181916

Merged

2 tasks

alex-spies mentioned this pull request Oct 7, 2024

ESQL: Tests for arbitrary expressions in SORT #113998

Open

craigtaverner mentioned this pull request Oct 17, 2024

Missing docs for ES|QL sorting by expressions #115011

Closed

idegtiarenko mentioned this pull request Jun 20, 2025

Fix toIpInAgg & toIpInSort #129743

Merged

wchaparro mentioned this pull request Mar 5, 2026

Document ES|QL SORT by expression #143705

Merged

	\| eval c = concat(left(last_name, 1), left(first_name, 1))
	\| eval l = left(last_name, 1, r = left(first_name, 1)

Conversation

astefan commented Apr 5, 2024

Uh oh!

elasticsearchmachine commented Apr 5, 2024

Uh oh!

elasticsearchmachine commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewdaemon commented Apr 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants