ESQL: Reuse child outputSet inside the plan where possible #124611
Conversation
Avoid creating outputSet between nodes that passthrough their input Relates elastic#124395
costin
added
>enhancement
:Analytics/ES|QL
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
Hi @costin, I've created a changelog YAML for you. |
|
The initial PR (making AttributeMap/Set immutable) become larger and ended up in chasing subtle bugs with planning - I've extracted this small change as it has a direct impact on queries with many fields. |
There was a problem hiding this comment.
Thanks @costin, I like the idea and I think it makes a lot of sense.
It only needs a little change to UnaryPlan, see comment below.
| @Override | ||
| public AttributeSet outputSet() { | ||
| return child().outputSet(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm afraid this is incorrect: now all the subplans that override output() but not outputSet() (eg. Eval) return a wrong output set, based on the child rather than on the actual output
There was a problem hiding this comment.
++, this is dangerous, we normally don't have to override outputSet but rely it to be constructed from output().
A IMHO better way that remains safe:
public AttributeSet outputSet() {
if (lazyOutputSet == null) {
List<Attribute> output = output();
lazyOutputSet = (output == child.output?) child.outputSet() : new AttributeSet(output);
}
return lazyOutputSet;
There was a problem hiding this comment.
I think this change itself makes sense. I'd like it a little better if the improvements were tracked by benches - I think this would strongly benefit from a micro benchmark (or a nightly!) that demonstrates the effect of this improvement on execution times of our optimizer(s).
| @Override | ||
| public AttributeSet outputSet() { | ||
| return child().outputSet(); | ||
| } | ||
|
|
There was a problem hiding this comment.
++, this is dangerous, we normally don't have to override outputSet but rely it to be constructed from output().
A IMHO better way that remains safe:
public AttributeSet outputSet() {
if (lazyOutputSet == null) {
List<Attribute> output = output();
lazyOutputSet = (output == child.output?) child.outputSet() : new AttributeSet(output);
}
return lazyOutputSet;
|
|
||
| @Override | ||
| public AttributeSet outputSet() { | ||
| return child().outputSet(); |
There was a problem hiding this comment.
With a better default implementation of this in UnaryPlan, this might be obsolete. Same goes for the corresponding overrides in the other classes.
| return child().output(); | ||
| } |
There was a problem hiding this comment.
Out of scope, but I noticed this is redundant.
costin
added
the
auto-backport
…24611) Avoid creating outputSet between nodes that passthrough their input Relates elastic#124395
…24611) Avoid creating outputSet between nodes that passthrough their input Relates elastic#124395
…24611) Avoid creating outputSet between nodes that passthrough their input Relates elastic#124395
Avoid creating outputSet between nodes that passthrough their input
Relates #124395