-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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.
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
ESQL: Fix bug when combining projections #107131
Conversation
Recursive aliases (eval x = 1, x1 = x) were not taken into account when combining projections causing the target field to be lost (and only the immediate intermediate named expression to be used instead which became invalid). Fix elastic#107083
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @costin, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good, only minor remarks - except one potentially non optimal pruning in an added test, but that shouldn't block this
@@ -1550,3 +1550,37 @@ s2point1:d | s_mv:i | languages:i | |||
2.1 | 3 | 5 | |||
2.1 | 3 | null | |||
; | |||
|
|||
evalOverridingKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this also need a skip for pre 8.14?
assertThat(Expressions.names(keep.projections()), contains("last_name", "f2", "f4")); | ||
var eval = as(keep.child(), Eval.class); | ||
assertThat(Expressions.names(eval.fields()), contains("f4")); | ||
var add = as(Alias.unwrap(eval.fields().get(0)), Add.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should assert that the summands are language and 3.
* \_Aggregate[[last_name{f}#23, first_name{f}#20, k{r}#4],[SUM(salary{f}#24) AS s, last_name{f}#23, first_name{f}#20, first_n | ||
* ame{f}#20 AS k]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially out of scope, but: couldn't/shouldn't we prune k
from the groupings, since it's an alias for first_name
? k
doesn't seem to be used in the aggregates, either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #107166
// collect named expressions declaration in the lower list | ||
AttributeMap<NamedExpression> namedExpressions = new AttributeMap<>(); | ||
// while also collecting the alias map for resolving the source (f1 = 1, f2 = f1, etc..) | ||
AttributeMap<Expression> aliases = new AttributeMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comments/var names here are a bit hard to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💔 Backport failed
You can use sqren/backport to manually backport by running |
Recursive aliases (eval x = 1, x1 = x) were not taken into account when combining projections causing the target field to be lost (and only the immediate intermediate named expression to be used instead which became invalid). Fix elastic#107083 (cherry picked from commit a9388e1)
Raised #107167 for backport |
Recursive aliases (eval x = 1, x1 = x) were not taken into account when
combining projections causing the target field to be lost (and only the
immediate intermediate named expression to be used instead which became
invalid).
Fix #107083