ESQL: Align `RENAME` behavior with `EVAL` for sequential processing #122250

kanoshiou · 2025-02-11T13:22:23Z

The RENAME command should function as similarly to other commands like EVAL as possible, executing operations sequentially from left to right.

For example, the following query:

row  foo = 10, bar = null | rename bar AS foo, foo AS bar

is equivalent to:

row foo = 10, bar = null 
| rename bar AS foo  // foo = null (`bar` is renamed to `foo`, making the original `foo` inaccessible)
| rename foo AS bar  // bar = null (the new `foo` is then renamed to `bar`)

and should produce the result:

      bar      
---------------
null

Closes #121739

kanoshiou · 2025-02-11T13:23:58Z

During code validation, a question arose:

row foo = 10, bar = null | rename bar2 as bar2

When the pre-modification name and the post-modification name are the same, should it throw a VerificationException of unknown column ? Currently, it simply ignores this rename operation.

I think #122165 is a related issue to this problem.

elasticsearchmachine · 2025-02-14T17:38:32Z

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

astefan · 2025-02-26T09:01:25Z

@kanoshiou thank you for this contribution. At this point, though, this subject is in internal discussions on how ES|QL should actually behave in this case. There are other similar scenarios in other queries involving overrides and all of these should be considered when making a decision.
This is just an fyi and for setting expectations on our responsiveness for this PR review. Thank you for your understanding.

ivancea

Hi @kanoshiou, sorry for the delay!
After some discussions, we want to keep RENAME as similar as other commands like EVAL as possible, where operations are "executed" from left to right.

For RENAME, it means that:

| RENAME a AS b, b AS c

Should be identical to:

| RENAME a AS b
| RENAME b AS c

And that:

ROW a=1, b=2
| RENAME
    a AS b, // b=1 ("a" got renamed to "b", "b" is no longer accessible)
    b AS a  // a=1 (the new "b" got renamed to "a" again)

Should first shadow the original b, and finally leave only a.

I've added some comments with what would be the expected result of the CSV tests, and some proposed new tests to cover some potential edge cases.

Also, about the case you comment: row foo = 10, bar = null | rename bar2 as bar2
I would leave it out of this PR. I see you commented on the other issue already; that should be enough for when we want to open a discussion over that topic.

Thank you again! I'll be watching this PR for when a review is needed; tell me if there's something you need

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

kanoshiou · 2025-03-17T12:00:33Z

Thank you @ivancea for your review and for assisting with the test cases. I've updated the branch, and it would be great if you could review it at your convenience.

ivancea · 2025-03-17T16:41:23Z

buildkite test this

ivancea · 2025-03-18T10:59:42Z

buildkite test this

ivancea · 2025-03-20T13:07:12Z

buildkite test this

ivancea · 2025-03-20T13:08:04Z

(Sorry for the repeated commands and merges; there's a failing check and I'm trying to ensure that it's unrelated before merging)

kanoshiou · 2025-03-20T15:56:18Z

@ivancea I believe this issue is unrelated, as it has already been muted by #125305.

ivancea

Thank you!

elasticsearchmachine · 2025-03-20T16:05:55Z

💚 Backport successful

Status	Branch	Result
✅	8.x
✅	9.0

…lastic#122250)

…122250) (#125325) Co-authored-by: kanoshiou <[email protected]>

…sing (#122250) (#125324) * ESQL: Align `RENAME` behavior with `EVAL` for sequential processing (#122250) * Fixed line length on comment for 8.x --------- Co-authored-by: kanoshiou <[email protected]>

…lastic#122250)

We shouldn't mutate LogicalPlan.child().output() (or just LogicalPlan.output()); we did so in RENAME, but only in EsqlNodeSubclassTest, making them flaky. >The PR at #122250 seems to have created a flaky test failure in `EsqlNodeSubclassTests`. Local runs with `-Dtests.iters=100` lead to about two dozen failures in over 70k tests run. This is not a high failure rate, but still requires addressing. > >The single line added to the Analyzer by that PR causes an `UnsupportedOperationException` on attempting to mutate an immutable collection when running `EsqlNodeSubclassTests`. It turns out that this code path comes from `Rename.output()` which is only called in test scenarios. So a quick fix is to copy the child output into a mutable collection.

We shouldn't mutate LogicalPlan.child().output() (or just LogicalPlan.output()); we did so in RENAME, but only in EsqlNodeSubclassTest, making them flaky. >The PR at elastic#122250 seems to have created a flaky test failure in `EsqlNodeSubclassTests`. Local runs with `-Dtests.iters=100` lead to about two dozen failures in over 70k tests run. This is not a high failure rate, but still requires addressing. > >The single line added to the Analyzer by that PR causes an `UnsupportedOperationException` on attempting to mutate an immutable collection when running `EsqlNodeSubclassTests`. It turns out that this code path comes from `Rename.output()` which is only called in test scenarios. So a quick fix is to copy the child output into a mutable collection.

We shouldn't mutate LogicalPlan.child().output() (or just LogicalPlan.output()); we did so in RENAME, but only in EsqlNodeSubclassTest, making them flaky. >The PR at #122250 seems to have created a flaky test failure in `EsqlNodeSubclassTests`. Local runs with `-Dtests.iters=100` lead to about two dozen failures in over 70k tests run. This is not a high failure rate, but still requires addressing. > >The single line added to the Analyzer by that PR causes an `UnsupportedOperationException` on attempting to mutate an immutable collection when running `EsqlNodeSubclassTests`. It turns out that this code path comes from `Rename.output()` which is only called in test scenarios. So a quick fix is to copy the child output into a mutable collection.

…lastic#122250)

We shouldn't mutate LogicalPlan.child().output() (or just LogicalPlan.output()); we did so in RENAME, but only in EsqlNodeSubclassTest, making them flaky. >The PR at elastic#122250 seems to have created a flaky test failure in `EsqlNodeSubclassTests`. Local runs with `-Dtests.iters=100` lead to about two dozen failures in over 70k tests run. This is not a high failure rate, but still requires addressing. > >The single line added to the Analyzer by that PR causes an `UnsupportedOperationException` on attempting to mutate an immutable collection when running `EsqlNodeSubclassTests`. It turns out that this code path comes from `Rename.output()` which is only called in test scenarios. So a quick fix is to copy the child output into a mutable collection.

Swapping names

5b9070c

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Feb 11, 2025

kanoshiou and others added 5 commits February 11, 2025 21:30

Add tests

413c885

Add changelog

ec26025

Update testcase

ce12e7b

Add more tests

619373a

Merge branch 'main' into rename-swap-values

6d2cdc7

limotova added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Feb 14, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 14, 2025

ivancea self-requested a review February 19, 2025 18:01

kanoshiou changed the title ~~ES|QL: Improve RENAME logic to swap names~~ ESQL: Improve RENAME logic to swap names Feb 21, 2025

astefan requested a review from bpintea March 7, 2025 15:34

ivancea reviewed Mar 14, 2025

View reviewed changes

kanoshiou and others added 3 commits March 17, 2025 15:45

Merge branch 'main' into rename-swap-values

fe55544

Update

353fdd7

Merge branch 'main' into rename-swap-values

f776e09

kanoshiou changed the title ~~ESQL: Improve RENAME logic to swap names~~ ESQL: Align RENAME behavior with EVAL for sequential processing Mar 17, 2025

Update changelog

445c4fb

ivancea self-assigned this Mar 17, 2025

ivancea added the >bug label Mar 17, 2025

Merge branch 'main' into rename-swap-values

6aee63c

Merge branch 'refs/heads/main' into rename-swap-values

54f6360

Merge branch 'main' into rename-swap-values

24fa367

ivancea approved these changes Mar 20, 2025

View reviewed changes

ivancea added v8.19.0 v9.0.1 auto-backport Automatically create backport pull requests when merged labels Mar 20, 2025

ivancea merged commit 873827d into elastic:main Mar 20, 2025
17 of 19 checks passed

This was referenced Mar 20, 2025

[8.x] ESQL: Align RENAME behavior with EVAL for sequential processing (#122250) #125324

Merged

[9.0] ESQL: Align RENAME behavior with EVAL for sequential processing (#122250) #125325

Merged

ivancea pushed a commit to ivancea/elasticsearch that referenced this pull request Mar 20, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (e…

1728097

…lastic#122250)

ivancea pushed a commit to ivancea/elasticsearch that referenced this pull request Mar 20, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (e…

d28a656

…lastic#122250)

elasticsearchmachine pushed a commit that referenced this pull request Mar 20, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (#…

574de38

…122250) (#125325) Co-authored-by: kanoshiou <[email protected]>

afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Mar 21, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (e…

439f463

…lastic#122250)

smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (e…

328fb22

…lastic#122250)

craigtaverner added a commit to craigtaverner/elasticsearch that referenced this pull request Mar 24, 2025

Un-flake some flaky tests recently introduced by elastic#122250

9992e4a

This was referenced Mar 24, 2025

Un-flake some flaky tests recently introduced by #122250 #125504

Closed

ESQL: Fix EsqlNodeSubclassTest failure from mutating #125503

Merged

omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025

ESQL: Align RENAME behavior with EVAL for sequential processing (e…

5879f90

…lastic#122250)

ivancea mentioned this pull request Apr 8, 2025

ESQL: Updated RENAME docs with the behaviour of multiple column renames #126462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ESQL: Align `RENAME` behavior with `EVAL` for sequential processing #122250

ESQL: Align `RENAME` behavior with `EVAL` for sequential processing #122250

Uh oh!

kanoshiou commented Feb 11, 2025 •

edited

Loading

Uh oh!

kanoshiou commented Feb 11, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented Feb 14, 2025

Uh oh!

astefan commented Feb 26, 2025

Uh oh!

ivancea left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kanoshiou commented Mar 17, 2025

Uh oh!

ivancea commented Mar 17, 2025

Uh oh!

ivancea commented Mar 18, 2025

Uh oh!

ivancea commented Mar 20, 2025

Uh oh!

ivancea commented Mar 20, 2025

Uh oh!

kanoshiou commented Mar 20, 2025

Uh oh!

ivancea left a comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 20, 2025

Uh oh!

Uh oh!

ESQL: Align RENAME behavior with EVAL for sequential processing #122250

ESQL: Align RENAME behavior with EVAL for sequential processing #122250

Uh oh!

Conversation

kanoshiou commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kanoshiou commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 14, 2025

Uh oh!

astefan commented Feb 26, 2025

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kanoshiou commented Mar 17, 2025

Uh oh!

ivancea commented Mar 17, 2025

Uh oh!

ivancea commented Mar 18, 2025

Uh oh!

ivancea commented Mar 20, 2025

Uh oh!

ivancea commented Mar 20, 2025

Uh oh!

kanoshiou commented Mar 20, 2025

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 20, 2025

💚 Backport successful

Uh oh!

Uh oh!

ESQL: Align `RENAME` behavior with `EVAL` for sequential processing #122250

ESQL: Align `RENAME` behavior with `EVAL` for sequential processing #122250

kanoshiou commented Feb 11, 2025 •

edited

Loading

kanoshiou commented Feb 11, 2025 •

edited

Loading