ESQL: Remove redundant TopN
#131833
ESQL: Remove redundant TopN
#131833
Conversation
elasticsearchmachine
added
v9.2.0
needs:triage
gmarouli
added
:Analytics/ES|QL
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
|
@elasticsearchmachine test this please |
|
Hi, @smalyshev. It looks like some CI tests are failing, possibly due to the branch being out-of-date with main. Could you try updating the branch before re-running the tests? |
There was a problem hiding this comment.
Thanks @kanoshiou and apologies that it took us so long to get back to you! Your contributions are very much appreciated!
Unfortunately, I think the new version of PruneRedundantOrderBy is affecting too many queries, including ones where removing an upstream TopN is incorrect. I give examples below.
I think we should limit the scope by adding a new rule that only looks for subsequent TopNs, specifically, and place that rule into the cleanup batch of the LogicalPlanOptimizer. That's more obviously correct. Cases when there are query plan nodes between two TopNs and the upstream one should be pruned are trickier to handle and likely require to mark query plans in some way when they are compatible with this optimization.
| // A child TopN is redundant if it has the same sort order as the parent. | ||
| // We do not need to compare their values because `PushDownAndCombineLimits` | ||
| // has already pushed down the lower limit value | ||
| if (childTopN.order().equals(parentTopN.order())) { |
There was a problem hiding this comment.
Caution: equals will currently still return true even if we order by 2 attributes that have the same name+type, but not the same NameId - i.e. attributes that mean something else.
This is a different bug that needs fixing separately, but I wanted to point out that currently it will prevent this from rightfully triggering in a couple situations.
There was a problem hiding this comment.
Oh, I’m currently encountering this bug while resolving another issue. This is sad 😭
Is this bug touching too much code, so it’s hard to fix?
...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java
Outdated
Show resolved
Hide resolved
| result.add(childTopN); | ||
| toCheck.push(childTopN.child()); | ||
| } | ||
| } else if (p instanceof SortAgnostic) { |
There was a problem hiding this comment.
The new version of the rule is incorrect, I'm afraid.
We continue going down the plan as long as the children are SortAgnostic. However, being SortAgnostic doesn't automatically imply that the plans behave the same when we feed them an unlimited number of rows.
For instance, INLINE STATS is SortAgnostic (resp. InlineJoin is). But removing a TopN from before an INLINE STATS changes the meaning of the query.
Also, queries can become much more expensive: For instance, MvExpand is SortAgnostic, but SORT a | LIMIT 10 | MV_EXPAND b | SORT a | LIMIT 10 is generally cheaper to run than just MV_EXPAND b | SORT a | LIMIT 3.
|
Thank you for your review @alex-spies! I've made several changes based on your feedback. However, I'm still finding it challenging to handle cases where there are query plan nodes between two From the examples you provided in your comment, removing the second |
|
Hi @alex-spies, I’ve updated the branch. The optimization now removes redundant child |
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Thanks a lot for your effort @kanoshiou ! This one's not easy!
I still think we should limit the scope to ... | TOP N | TOP M | ... . It's very difficult to get the rule right if there are other nodes between the TOP N and TOP M. We have something similar in HoistRemoteEnrichTopN, but I don't think this can be applied here 1:1. In fact, the complexity increased sine HoistRemoteEnrichTopN was added because now TOP Ns can also be local, meaning, they don't necessitate breaking the pipeline. (And getting HoistRemoteEnrichTopN was highly complex, too.)
| || p instanceof Filter | ||
| || p instanceof Insist | ||
| || p instanceof Limit |
There was a problem hiding this comment.
Removing past limits and filters is incorrect. There is probably a class of nodes where TOP N | COMMAND | TOP M can be simplified to COMMAND | TOP min(N,M), but these cannot be included:
SORT a | LIMIT 10 | WHERE b > 10 | SORT c | LIMIT 1
vs.
LIMIT 10 | WHERE b > 10 | SORT c | LIMIT 1
The first takes the top 10 as, removes those with b <= 10 and then gives the top c from this subset.
The second takes random 10 rows, then takes those with b > 10, and finally gives the top c from this subset. But the subsets are different.
- Refactor PruneRedundantTopN to use a more straightforward approach for removing redundant TopN operations - Replace complex breadth-first recursive search with a simpler while loop traversal - Remove unnecessary method for checking removable child nodes - Improve performance and readability of TopN pruning logic - Update test case to reflect simplified optimization approach
|
Thanks for the review and the detailed explanation, @alex-spies! You're absolutely right about the complexity of handling non-consecutive |
# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/topN.csv-spec
5 participants
Redundant
TopNoperations were not being removed from the execution plan, leading to unnecessary performance overhead.We can safely consider a child
TopNplan redundant if it shares the same sort order as its parent. There's no need to compare their limit values, sincePushDownAndCombineLimitsalready pushes down the lower limit value, making it safe to remove the childTopN.Also, the
PruneRedundantOrderByrule was only executed before theReplaceLimitAndSortAsTopN, so it could only detect redundantOrderByoperations but not redundantTopNoperations that were created later in the optimization pipeline.Closes #131233