Repository navigation
ESQL: Prune InlineJoin right aggregations by delegating to the child plan - #139357
Conversation
| if (aggregate.groupings().containsAll(remaining)) { | ||
| p = aggregate.child(); | ||
| } | ||
| // TODO: deal with prunning partial groupings in InlineJoin right side |
There was a problem hiding this comment.
I am aware of another type of pruning bug regarding groupings pruning, but I chose to deal with that one in a separate PR.
| private static LogicalPlan pruneColumnsInInlineJoinRight(InlineJoin ij, AttributeSet.Builder used, Holder<Boolean> recheck) { | ||
| LogicalPlan p = ij; | ||
|
|
||
| used.addAll(ij.references()); |
There was a problem hiding this comment.
This was essential. Now the references are added to used after pruning happens, which makes much more sense imo.
bpintea
left a comment
There was a problem hiding this comment.
Thanks Andrei. This turned out a bit tricky.
LGTM, left some questions only.
| // if the right has no aggregation anymore, but it still has some other plans (evals, projects), | ||
| // we keep those and integrate them into the main plan. The InlineJoin is also replaced entirely. | ||
| p = InlineJoin.replaceStub(ij.left(), right); | ||
| p = new Project(ij.source(), p, mergeOutputExpressions(p.output(), ij.left().output())); |
There was a problem hiding this comment.
Could this have been:
| p = new Project(ij.source(), p, mergeOutputExpressions(p.output(), ij.left().output())); | |
| p = new Project(ij.source(), p, ij.output()); |
?
Maybe safer this computed way, though, only curious if it'd trigger some issues, as the output shouldn't change, I think.
There was a problem hiding this comment.
The output of the InlineJoin has been (and my gut feeling is that still is with the GH issue I referenced above as the next bug to tackle) a tricky aspect. Unless the InlineJoin doesn't change/transform and a new Object is created, the output will stay the same once initialized. In the case of disappearing attributes (like some of the groupings, some of the aggregates), the output of the InlineJoin should reflect them. In this specific code, the left side becomes the Stub relation replacement for the right side and the resulting plan chunk is integrated in the main plan.
And with prunning the Aggregate completely, some stuff are not there in the output anymore and in one of the unit tests this was visible. And again, thank you for adding those unit tests, they were both a pain to deal with, but also a lifesaver to help me navigate the tricky aspects of InlineJoin pruning.
| if (inlineJoin) { | ||
| p = emptyLocalRelation(aggregate); | ||
| // all aggregates are pruned, delegate to child plan | ||
| p = aggregate.child(); |
There was a problem hiding this comment.
Wouldn't we need a Projection here?
I suppose this is a stub, but is it always a stub?
There was a problem hiding this comment.
It should be a Stub indeed, but we shouldn't assume that. New commands, new rules can add stuff in there.
Regarding the Projection, our tests are so good in this regard, btw, and if something doesn't break, it should be ok. Conceptually speaking, though, if whatever the output of the Agg is/was, at this point its output doesn't matter anymore and I don't think we need an explicit projection.
| // already part of the IJ output (from the left-hand side): the agg can just be dropped entirely. | ||
| p = emptyLocalRelation(aggregate); | ||
| if (aggregate.groupings().containsAll(remaining)) { | ||
| p = aggregate.child(); |
There was a problem hiding this comment.
Same here, is it always safe?
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @astefan, I've created a changelog YAML for you. |
Fixes #138283