Skip to content

Commit ef03007

Browse files
authored
Fix PushDownAndCombineLimits local limit handling when the limits are equal (elastic#139399) (elastic#139477)
* Fix PushDownAndCombineLimits local limit handling when the limits are equal (cherry picked from commit c80fa9a)
1 parent d27dd84 commit ef03007

File tree

2 files changed

+16
-4
lines changed

2 files changed

+16
-4
lines changed

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,20 @@ private static Limit combineLimits(Limit upper, Limit lower, FoldContext ctx) {
8383
// Keep the smallest limit
8484
var upperLimitValue = (int) upper.limit().fold(ctx);
8585
var lowerLimitValue = (int) lower.limit().fold(ctx);
86-
// We want to preserve the duplicated() value of the smaller limit.
87-
if (lowerLimitValue <= upperLimitValue) {
88-
return lower.withLocal(lower.local());
86+
/*
87+
* We always want to select the smaller of the limits, but with the local flag it gets a bit tricky.
88+
* If one of the limits is smaller, that's what we will choose. But if the limits are exactly equal,
89+
* then we can choose the local limit because this may enable some queries that would otherwise be prohibited
90+
* due to pipeline-breaking nature of non-local limits in combination with remote Enrich.
91+
* However this may not be true if we have more situations where local limits are generated which do not have
92+
* guarantees that local limit produced by remote enrich pushing provides.
93+
* See also: https://github.com/elastic/elasticsearch/pull/139399#pullrequestreview-3573026118
94+
*/
95+
if (lowerLimitValue < upperLimitValue) {
96+
return lower;
97+
} else if (lowerLimitValue == upperLimitValue) {
98+
// If any of them is local, we want the local limit
99+
return lower.local() ? lower : lower.withLocal(upper.local());
89100
} else {
90101
return new Limit(upper.source(), upper.limit(), lower.child(), upper.duplicated(), upper.local());
91102
}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimitsTests.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,8 @@ public void testPushableLimitDuplicate() {
242242
int precedingLimitValue = randomIntBetween(1, 10_000);
243243
Limit precedingLimit = new Limit(EMPTY, new Literal(EMPTY, precedingLimitValue, INTEGER), relation);
244244
LogicalPlan duplicatingLimitTestPlan = duplicatingTestCase.buildPlan(precedingLimit, a);
245-
int upperLimitValue = randomIntBetween(1, precedingLimitValue);
245+
// Explicitly raise the probability of equal limits, to test for https://github.com/elastic/elasticsearch/issues/139250
246+
int upperLimitValue = randomBoolean() ? precedingLimitValue : randomIntBetween(1, precedingLimitValue);
246247
Limit upperLimit = new Limit(EMPTY, new Literal(EMPTY, upperLimitValue, INTEGER), duplicatingLimitTestPlan);
247248
Limit optimizedPlan = as(optimizePlan(upperLimit), Limit.class);
248249
duplicatingTestCase.checkOptimizedPlan(duplicatingLimitTestPlan, optimizedPlan.child());

0 commit comments

Comments
 (0)