-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Handle right hand side of Inline Stats coming optimized with LocalRelation shortcut #135011
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: Handle right hand side of Inline Stats coming optimized with LocalRelation shortcut #135011
Conversation
|
Hi @astefan, I've created a changelog YAML for you. |
…handle_right_LocalRelation
…handle_right_LocalRelation
…handle_right_LocalRelation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| private static LogicalPlan emptyLocalRelation(UnaryPlan plan) { | ||
| // create an empty local relation with no attributes | ||
| return new LocalRelation(plan.source(), plan.output(), EmptyLocalSupplier.EMPTY); | ||
| return skipPlan(plan); |
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.
Unrelated to the meat of the PR
…handle_right_LocalRelation
bpintea
left a comment
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.
LG, not sure about the source of the failure, but it seems it might have a different origin? Happy to re-review otherwise.
| } else if (ij.right() instanceof LocalRelation && skipped == null | ||
| || ij.right() instanceof LocalRelation == false && ij.right().anyMatch(p -> p instanceof LocalRelation)) { |
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.
Could this be:
| } else if (ij.right() instanceof LocalRelation && skipped == null | |
| || ij.right() instanceof LocalRelation == false && ij.right().anyMatch(p -> p instanceof LocalRelation)) { | |
| } else if (ij.right().children().stream().anyMatch(c -> c.anyMatch(LocalRelation.class::isInstance))) { |
Then I think you wouldn't need to pass the extra skipped param.
Which, otherwise, could be a bool, maybe, since it's only used in a test against null.
| var p = ij.right(); | ||
| p.setOptimized(); | ||
| subPlan.set(new LogicalPlanTuple(p, ij.right())); |
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.
Don't know if it'd be worth creating a light static method to do this here and the similar thing above.
|
@bpintea thank you for having a look. I have added a more detailed description to the PR, trying to clarify the situations (conceptually) this PR is trying to address and why this particular fix has been chosen. Hope this helps. |
…handle_right_LocalRelation
…handle_right_LocalRelation
| import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin; | ||
| import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; | ||
|
|
||
| public final class PruneEmptyInlineStatsRightSide extends OptimizerRules.OptimizerRule<InlineJoin> { |
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.
We have a PruneLeftJoinOnNullMatchingField. Should we call this some similar way? (maybe PruneInlineJoinOnEmptyRightSide -- I guess we could use InlineJoin instead of InlineStats, since the rule should apply correctly even if we found other uses of InlineJoin).
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.
Yep, makes sense. I've renamed it.
| } else if (ij.right() instanceof LocalRelation relation | ||
| && (subPlansResults.isEmpty() || subPlansResults.contains(relation) == false) | ||
| || ij.right() instanceof LocalRelation == false && ij.right().anyMatch(p -> p instanceof LocalRelation)) { |
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.
Might be worth extracting this check into a method, it's a bit hard to read.
I tried to simplify it a bit, but we run into block tracking issues, so I guess this could probably be simplified, with some extra accounting.
But that can be a followup.
…handle_right_LocalRelation
|
Late to the party, but just wanted to say this looks right to me. Thanks for keeping me in the loop, the inline stats machinery and how it evolves is super interesting! |
…-dls
* upstream/main: (55 commits)
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135525
Address es819 tsdb doc values format performance bug (elastic#135505)
Remove obsolete --add-opens from JDK API extractor tool (elastic#135445)
[CI] Fix MergeWithFailureIT (elastic#135447)
Increase wait time in AdaptiveAllocationsScalerServiceTests (elastic#135510)
ES|QL: Handle multi values in FUSE (elastic#135448)
Mute org.elasticsearch.upgrades.SyntheticSourceRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135512
Add trust configuration for cross cluster api keys (elastic#134893)
ESQL: Fix flakiness in SessionUtilsTests (elastic#135375)
Mute org.elasticsearch.upgrades.LogsdbIndexingRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135511
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135325
Require all functions to provide examples (elastic#135094)
Mute org.elasticsearch.upgrades.SyntheticSourceRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135344
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135236
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135238
Mute org.elasticsearch.upgrades.LogsdbIndexingRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135327
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=3} elastic#135324
Mute org.elasticsearch.upgrades.StandardToLogsDbIndexModeRollingUpgradeIT testLogsIndexing {upgradedNodes=3} elastic#135315
ESQL: Handle right hand side of Inline Stats coming optimized with LocalRelation shortcut (elastic#135011)
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=3} elastic#135237
...
This PR covers a small range of edge cases where the logical optimization rules "compete" in optimizing aggregations to a simplified tree that is either a
LocalRelationby itself or that have the source leaf node asLocalRelation. When the right-hand side becomes a non-concrete source, it is one of two situations now:EsqlSessionran the left-hand side query and the results are modeled asLocalRelationwhich replace theStubRelationof the right-hand sideThe code in the PR covers the second scenario above. There are few rules which touch the idea of
inline statsand which optimize away the right-hand side, but there are also others which are generic and have no different behavior forinline stats.PruneEmptyPlansis one rule where I debated a lot if it should also deal withStubRelationas an "empty" plan or remain as is. I decided that the particularities ofinlinejoinare not the concern of this rule, but those of the EsqlSession where the "coordination" of theinlinejoinis mostly happening.PruneColumnsis another rule where the pruning of the right-hand side is handledReplaceStatsFilteredAggWithEvaldeals with pruninginline statsandstatsthat have filters that can also be pruned