-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Remove redundant TopN
#131833
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
base: main
Are you sure you want to change the base?
ESQL: Remove redundant TopN
#131833
Changes from 4 commits
f95816f
870ca7a
6caf40a
744217e
544acbe
fc919f7
8eedd7b
6b470d4
df5862f
62a43b7
f06f02e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 131833 | ||
summary: "ESQL: Remove redundant `TopN`" | ||
area: ES|QL | ||
type: enhancement | ||
issues: | ||
- 131233 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ public class PruneRedundantOrderBy extends OptimizerRules.OptimizerRule<LogicalP | |
@Override | ||
protected LogicalPlan rule(LogicalPlan plan) { | ||
if (plan instanceof OrderBy || plan instanceof TopN || plan instanceof Aggregate) { | ||
Set<OrderBy> redundant = findRedundantSort(((UnaryPlan) plan).child()); | ||
Set<LogicalPlan> redundant = findRedundantSort(plan); | ||
if (redundant.isEmpty()) { | ||
return plan; | ||
} | ||
|
@@ -58,25 +58,32 @@ protected LogicalPlan rule(LogicalPlan plan) { | |
* breadth-first recursion to find redundant SORTs in the children tree. | ||
* Returns an identity set (we need to compare and prune the exact instances) | ||
*/ | ||
private Set<OrderBy> findRedundantSort(LogicalPlan plan) { | ||
Set<OrderBy> result = Collections.newSetFromMap(new IdentityHashMap<>()); | ||
private Set<LogicalPlan> findRedundantSort(LogicalPlan plan) { | ||
Set<LogicalPlan> result = Collections.newSetFromMap(new IdentityHashMap<>()); | ||
|
||
Deque<LogicalPlan> toCheck = new ArrayDeque<>(); | ||
toCheck.push(plan); | ||
toCheck.push(((UnaryPlan) plan).child()); | ||
|
||
while (true) { | ||
if (toCheck.isEmpty()) { | ||
return result; | ||
} | ||
while (toCheck.isEmpty() == false) { | ||
LogicalPlan p = toCheck.pop(); | ||
if (p instanceof OrderBy ob) { | ||
result.add(ob); | ||
toCheck.push(ob.child()); | ||
} else if (p instanceof TopN childTopN && plan instanceof TopN parentTopN) { | ||
// Check if a child TopN is redundant compared to a parent TopN. | ||
// 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())) { | ||
kanoshiou marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
result.add(childTopN); | ||
toCheck.push(childTopN.child()); | ||
} | ||
} else if (p instanceof SortAgnostic) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new version of the rule is incorrect, I'm afraid. We continue going down the plan as long as the children are For instance, Also, queries can become much more expensive: For instance, |
||
for (LogicalPlan child : p.children()) { | ||
toCheck.push(child); | ||
} | ||
} | ||
} | ||
return result; | ||
} | ||
} |
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.
Caution:
equals
will currently still returntrue
even if we order by 2 attributes that have the same name+type, but not the sameNameId
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?