Skip to content

ES|QL: fix LIMIT pushdown past MV_EXPAND #115624

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.

Sign up for GitHub

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

Merged
luigidellaquila merged 14 commits into from
Oct 30, 2024
Merged

ES|QL: fix LIMIT pushdown past MV_EXPAND #115624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
65096cb
ES|QL: fix LIMIT pushdown past MV_EXPAND
luigidellaquila Oct 25, 2024
280a4b5
Update docs/changelog/115624.yaml
luigidellaquila Oct 25, 2024
f471d54
Merge branch 'main' into esql/fix_sort_mv_expand_limit_2
elasticmachine Oct 25, 2024
b03ef72
Remove DuplicateLimitAfterMvExpand and add comments
luigidellaquila Oct 25, 2024
5b3fd2e
Merge remote-tracking branch 'luigidellaquila/esql/fix_sort_mv_expand…
luigidellaquila Oct 25, 2024
db86a6f
More tests
luigidellaquila Oct 28, 2024
e171cdf
Spotless
luigidellaquila Oct 28, 2024
5755968
Merge branch 'main' into esql/fix_sort_mv_expand_limit_2
elasticmachine Oct 28, 2024
3b407e7
Remove unnecessary checks
luigidellaquila Oct 28, 2024
2322b04
Merge remote-tracking branch 'luigidellaquila/esql/fix_sort_mv_expand…
luigidellaquila Oct 28, 2024
7ee5811
Merge branch 'main' into esql/fix_sort_mv_expand_limit_2
luigidellaquila Oct 30, 2024
8f1c2d7
More tests
luigidellaquila Oct 30, 2024
adc4d81
Merge branch 'main' into esql/fix_sort_mv_expand_limit_2
elasticmachine Oct 30, 2024
af70d28
Merge branch 'main' into esql/fix_sort_mv_expand_limit_2
elasticmachine Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.elasticsearch.xpack.esql.optimizer.rules.logical.CombineProjections;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.ConstantFolding;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.ConvertStringToByteRef;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.DuplicateLimitAfterMvExpand;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.FoldNull;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.LiteralsOnTheRight;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PartiallyFoldCase;
Expand Down Expand Up @@ -174,7 +173,6 @@ protected static Batch<LogicalPlan> operators() {
new PruneColumns(),
new PruneLiteralsInOrderBy(),
new PushDownAndCombineLimits(),
new DuplicateLimitAfterMvExpand(),
new PushDownAndCombineFilters(),
new PushDownEval(),
new PushDownRegexExtract(),
Expand Down
108 changes: 0 additions & 108 deletions ...ava/org/elasticsearch/xpack/esql/optimizer/rules/logical/DuplicateLimitAfterMvExpand.java
View file
Open in desktop

This file was deleted.

7 changes: 7 additions & 0 deletions ...n/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ public LogicalPlan rule(Limit limit) {
if (unary instanceof Eval || unary instanceof Project || unary instanceof RegexExtract || unary instanceof Enrich) {
return unary.replaceChild(limit.replaceChild(unary.child()));
} else if (unary instanceof MvExpand mvx) {
Copy link
Contributor

@craigtaverner craigtaverner Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this code previously did not push the limit down past the MvExpand at all, but the bug reports indicate that that was happening. Was it happening somewhere else? If so, should that location be modified?

Copy link
Contributor Author

@luigidellaquila luigidellaquila Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, good point, I forgot about it...
We have another rule, called DuplicateLimitAfterMvExpand, that does exactly what I describe above:
MV_EXPAND | LIMIT -> LIMIT | MV_EXPAND | LIMIT

I think we can remove it

Copy link
Contributor Author

@luigidellaquila luigidellaquila Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another rule, AddDefaultTopN, that is intrinsically wrong (it adds an arbitrary limit at the beginning of the query) and that IMHO should be removed as well, but I'm not sure this fix is enough to do it, maybe the problems it tries to solve are not only related to MV_EXPAND.

// MV_EXPAND can increase the number of rows, so we cannot just push the limit down
// (we also have to preserve the LIMIT afterwards)
//
// To avoid infinite loops, ie.
// | MV_EXPAND | LIMIT -> | LIMIT | MV_EXPAND | LIMIT -> ... | MV_EXPAND | LIMIT
// we add an inner limit to MvExpand and just push down the existing limit, ie.
// | MV_EXPAND | LIMIT N -> | LIMIT N | MV_EXPAND (with limit N)
var limitSource = limit.limit();
var limitVal = (int) limitSource.fold();
Integer mvxLimit = mvx.limit();
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ private PhysicalPlan map(UnaryPlan p, PhysicalPlan child) {
if (p instanceof MvExpand mvExpand) {
MvExpandExec result = new MvExpandExec(mvExpand.source(), map(mvExpand.child()), mvExpand.target(), mvExpand.expanded());
if (mvExpand.limit() != null && mvExpand.limit() >= 0) {
Copy link
Contributor

@bpintea bpintea Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about the >= 0 bit. Is it needed, do we do it anywhere else?

Copy link
Contributor Author

@luigidellaquila luigidellaquila Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we don't support a LIMIT -1... probably I should remove that check.

// MvExpand could have an inner limit
// see PushDownAndCombineLimits rule
return new LimitExec(result.source(), result, new Literal(Source.EMPTY, mvExpand.limit(), DataType.INTEGER));
}
return result;
Expand Down