ESQL: Fix planning of dangling Project with InlineJoin
#132992
Conversation
This fixes a planning error leaving a Project on top of an empty LocalRelation marker unpruned. The empty EmptyRelation is added in the plan as a marker, signaling that the right hand-side of the join can be pruned entirely (and thus the entire InlineJoin).
|
Fixes #132417 (comment) |
interestingly, it fails in CsvTests, but not in EsqlSpecIT, which accepts MIN(MV_COUNT(integer)) as both long and integer type.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| private static LogicalPlan pruneColumnsInProject(Project project, AttributeSet.Builder used) { | ||
| LogicalPlan p = project; | ||
|
|
||
| // A project atop a stub relation will only output attributes which are already part of the INLINEJOIN left-hand side output. |
There was a problem hiding this comment.
This feels brittle in the sense that there may be other rules that might have a different behavior and mess up with the expected pair of steps/actions (1. transform the aggregate in a Project then 2. look for the exact combination Project -> StubRelation) this pruning expects here.
I've had such a solution in my draft PR here but eventually I changed it after reconsidering what is happening in the life of an InlineJoin (basically I considered the flow of the EsqlSession as well, because there are things that are InlineJoin specific and it expects a StubRelation to exist OR the InlineJoin to be pruned completely) and I ended up with this solution instead: https://github.com/elastic/elasticsearch/pull/132934/files#diff-4c0de47236c79e13849f81a2b9765240c822a6af6e90902ba4912f1548c65082R90-R107
Maybe explore a bit more the pruning idea based on what I mentioned above on the idea that the Project + StubRelation you are expecting there might be coming from other rules and mess up the pruning.
There was a problem hiding this comment.
Thanks for the review. Generally:
the Project + StubRelation you are expecting there might be coming from other rules
this simplification only happens when the plan has the pattern: InlineJoin - Project - StubRelation. It'll only be problematic if we'll ever have StubRelations on the left hand-side of the InlineJoin. But I guess the verification can be done earlier in the flow, on Aggregate/Project substitution and make the substitution less pattern-dependent.
I'll give it a try.
There was a problem hiding this comment.
@astefan, I've taken another approach, simplifying the analysis of the Aggregate iif this is part of an InlineJoin, which allows simplifying the Project analysis downstream too. I think the logic should hold irrespective of InlineJoin being currently only used for INLINESTATS or later modeling other commands too.
The case where an agg on the right hand-side of an InlineJoin was pruned emitted unnecessary nodes in the case when only groups remained available.
|
Thanks, Andrei! |
3 participants
This fixes a planning error leaving a
Projecton top of an emptyLocalRelationmarker unpruned. The emptyEmptyRelationis added in the plan as a marker, signalling that the right hand-side of the join can be pruned entirely (and thus the entireInlineJoin).