-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[ESQL] Adds parsing and planner wiring for LIMIT BY #143762
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
Changes from all commits
9c3ef06
869254f
7be9e5d
08c9d83
e8db931
6665e6f
975cb89
3bbe10d
dc80e02
650d699
6cee286
b96fda3
376046f
8de248c
3a3663e
9de8437
118fce4
31821f3
a07bed3
7955598
2252f9d
614cf8e
066ebb7
e47b07d
a89c8fd
26a0c76
6851b6b
3375850
3bf8e8a
609c51f
28989be
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 @@ | ||
| 9314000 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| sql_optional_allow_partial_search_results,9313000 | ||
| esql_limit_by,9314000 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2190,6 +2190,12 @@ public enum Cap { | |
| */ | ||
| EXTERNAL_COMMAND(Build.current().isSnapshot()), | ||
|
|
||
| /** | ||
|
|
||
| * Enables LIMIT N BY without a preceding SORT. | ||
| */ | ||
| LIMIT_BY, | ||
|
Comment on lines
+2193
to
+2197
|
||
|
|
||
| /** | ||
| * https://github.com/elastic/elasticsearch/issues/142219 | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,9 @@ | |
| import org.elasticsearch.xpack.esql.plan.logical.Limit; | ||
| import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Lookup; | ||
| import org.elasticsearch.xpack.esql.plan.logical.OrderBy; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Project; | ||
| import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; | ||
| import org.elasticsearch.xpack.esql.plan.logical.promql.PromqlCommand; | ||
| import org.elasticsearch.xpack.esql.telemetry.FeatureMetric; | ||
| import org.elasticsearch.xpack.esql.telemetry.Metrics; | ||
|
|
@@ -119,6 +121,7 @@ Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics) { | |
| checkUnsupportedAttributeRenaming(p, failures); | ||
| checkInsist(p, failures); | ||
| checkLimitBeforeInlineStats(p, failures); | ||
| checkLimitBy(p, failures); | ||
| }); | ||
|
|
||
| if (failures.hasFailures() == false) { | ||
|
|
@@ -332,6 +335,23 @@ private static void checkLimitBeforeInlineStats(LogicalPlan plan, Failures failu | |
| } | ||
| } | ||
|
|
||
| // TODO: remove this check when SORT + LIMIT BY (TopN) support is added | ||
| private static void checkLimitBy(LogicalPlan plan, Failures failures) { | ||
|
Contributor
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. Note for reviewers: This is temporary until the TopN part is merged |
||
| if (plan instanceof Limit limit && limit.groupings().isEmpty() == false) { | ||
| LogicalPlan child = limit.child(); | ||
| while (child instanceof UnaryPlan unary) { | ||
| if (child instanceof OrderBy) { | ||
| failures.add(fail(limit, "SORT cannot be used before LIMIT BY")); | ||
| break; | ||
| } | ||
| if (child instanceof Limit l && l.groupings().isEmpty()) { | ||
| break; | ||
| } | ||
| child = unary.child(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void licenseCheck(LogicalPlan plan, Failures failures) { | ||
| Consumer<Node<?>> licenseCheck = n -> { | ||
| if (n instanceof LicenseAware la && la.licenseCheck(licenseState) == false) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyAggregates; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyForkBranches; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInLimitBy; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInOrderBy; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantSortClauses; | ||
|
|
@@ -60,6 +61,7 @@ | |
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateNestedExpressionWithEval; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAliasingEvalWithProject; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceLimitAndSortAsTopN; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceLimitByExpressionWithEval; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceOrderByExpressionWithEval; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceRegexMatch; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceRowAsLocalRelation; | ||
|
|
@@ -181,6 +183,7 @@ protected static Batch<LogicalPlan> substitutions() { | |
| // check for a trivial conversion introduced by a surrogate | ||
| new ReplaceTrivialTypeConversions(), | ||
| new ReplaceOrderByExpressionWithEval(), | ||
| new ReplaceLimitByExpressionWithEval(), | ||
|
Comment on lines
185
to
+186
Contributor
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. I see
Contributor
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. Adding to this, I think a test like this would fail now (as in. limit won't be pruned), but work if it's separated:
Member
Author
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. I've taken the following steps:
|
||
| // new NormalizeAggregate(), - waits on https://github.com/elastic/elasticsearch/issues/100634 | ||
| new SubstituteApproximationPlan() | ||
| ); | ||
|
|
@@ -224,6 +227,7 @@ protected static Batch<LogicalPlan> operators() { | |
| new PruneFilters(), | ||
| new PruneColumns(), | ||
| new PruneLiteralsInOrderBy(), | ||
| new PruneLiteralsInLimitBy(), | ||
| new PushDownAndCombineLimits(), | ||
| new PushLimitToKnn(), | ||
| new PushDownAndCombineFilters(), | ||
|
|
||
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.
What's this change?
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry I forgot to explain it.
antlr4 only accepts a lib argument, so here passing the second one overrides the first:
https://github.com/antlr/antlr4/blob/faa457ae5b9c39b572334814f0b36c855bc23010/tool/src/org/antlr/v4/Tool.java#L99
We should not have this in the build if it's doing nothing