Skip to content

Commit 702a322

Browse files
ESQL: Add Warning for Sort Under Lookup Join (elastic#141482)
Adds a warning when a SORT (or TopN) is followed by a LOOKUP JOIN without a subsequent SORT to restore order. Since LOOKUP JOIN does not preserve the order of its input, users may get unexpected results if they expect sorted output. The warning is generated by a new optimizer rule WarnLostSortOrder that runs once after optimization, traversing the plan tree to detect this pattern. This PR has been developed partially with the help of GenAI tools (Cursor).
1 parent 333357a commit 702a322

File tree

14 files changed

+338
-1
lines changed

14 files changed

+338
-1
lines changed

docs/changelog/141482.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
area: ES|QL
2+
issues:
3+
- 141483
4+
pr: 141482
5+
summary: Add Warning for Sort Under Lookup Join
6+
type: enhancement

x-pack/plugin/build.gradle

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,13 @@ tasks.named("yamlRestCompatTestTransform").configure(
164164
"Vectors are no longer returned by default"
165165
)
166166
task.skipTest("esql/190_lookup_join/lookup-no-key-only-key", "Requires the fix")
167+
task.skipTest("esql/190_lookup_join/basic", "New SORT warning not in original test")
168+
task.skipTest("esql/191_lookup_join_text/keyword-keyword", "New SORT warning not in original test")
169+
task.skipTest("esql/191_lookup_join_text/text-keyword", "New SORT warning not in original test")
170+
task.skipTest("esql/192_lookup_join_on_aliases/alias-as-lookup-index", "New SORT warning not in original test")
171+
task.skipTest("esql/192_lookup_join_on_aliases/alias-repeated-alias", "New SORT warning not in original test")
172+
task.skipTest("esql/192_lookup_join_on_aliases/alias-repeated-index", "New SORT warning not in original test")
173+
task.skipTest("esql/192_lookup_join_on_aliases/alias-pattern-single", "New SORT warning not in original test")
167174
task.skipTest("ml/data_frame_analytics_crud/Test put config with remote source index", "Error message changed")
168175
task.skipTest("esql/40_tsdb/PromQL irate on counter field", "We filter out null values now")
169176
}

x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join-expression.csv-spec

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,7 @@ id_left:integer | name_left:keyword | other1_from_first_join:keyword | other1:ke
682682
lookupOnExpressionOnTheCoordinator
683683
required_capability: join_lookup_v12
684684
required_capability: lookup_join_on_boolean_expression
685+
required_capability: lookup_join_sort_warning
685686

686687
FROM employees
687688
| SORT emp_no
@@ -691,6 +692,8 @@ FROM employees
691692
| KEEP emp_no, language_code_left, language_name
692693
;
693694

695+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
696+
694697
emp_no:integer | language_code_left:integer | language_name:keyword
695698
10001 | 2 | French
696699
10002 | 5 | null

x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ language_code:integer | language_name:keyword
3737

3838
basicOnTheCoordinator
3939
required_capability: join_lookup_v12
40+
required_capability: lookup_join_sort_warning
4041

4142
FROM employees
4243
| SORT emp_no
@@ -46,6 +47,8 @@ FROM employees
4647
| KEEP emp_no, language_code, language_name
4748
;
4849

50+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
51+
4952
emp_no:integer | language_code:integer | language_name:keyword
5053
10001 | 2 | French
5154
10002 | 5 | null
@@ -72,6 +75,7 @@ emp_no:integer | language_code:integer | language_name:keyword | language_code_x
7275

7376
subsequentEvalOnTheCoordinator
7477
required_capability: join_lookup_v12
78+
required_capability: lookup_join_sort_warning
7579

7680
FROM employees
7781
| SORT emp_no
@@ -82,6 +86,8 @@ FROM employees
8286
| EVAL language_name = TO_LOWER(language_name), language_code_x2 = 2*language_code
8387
;
8488

89+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
90+
8591
emp_no:integer | language_code:integer | language_name:keyword | language_code_x2:integer
8692
10001 | 2 | french | 4
8793
10002 | 5 | null | 10
@@ -90,6 +96,7 @@ emp_no:integer | language_code:integer | language_name:keyword | language_code_x
9096

9197
sortEvalBeforeLookup
9298
required_capability: join_lookup_v12
99+
required_capability: lookup_join_sort_warning
93100

94101
FROM employees
95102
| SORT emp_no
@@ -99,6 +106,8 @@ FROM employees
99106
| LIMIT 3
100107
;
101108

109+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
110+
102111
emp_no:integer | language_code:integer | language_name:keyword
103112
10001 | 2 | French
104113
10002 | 3 | Spanish
@@ -267,6 +276,7 @@ emp_no:integer | language_code:integer | language_name:keyword | country:keyword
267276

268277
nonUniqueRightKeyOnTheCoordinator
269278
required_capability: join_lookup_v12
279+
required_capability: lookup_join_sort_warning
270280

271281
FROM employees
272282
| SORT emp_no
@@ -277,6 +287,8 @@ FROM employees
277287
| KEEP emp_no, language_code, language_name, country
278288
;
279289

290+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
291+
280292
ignoreOrder:true
281293
emp_no:integer | language_code:integer | language_name:keyword | country:keyword
282294
10001 | 1 | English | Canada
@@ -294,6 +306,7 @@ emp_no:integer | language_code:integer | language_name:keyword | country:keyword
294306
nonUniqueRightKeyOnTheCoordinatorCorrectOrdering
295307
// Same as above, but don't ignore the order completely. At least the emp_no col must remain correctly ordered.
296308
required_capability: join_lookup_v12
309+
required_capability: lookup_join_sort_warning
297310

298311
FROM employees
299312
| SORT emp_no
@@ -303,6 +316,8 @@ FROM employees
303316
| KEEP emp_no, language_code
304317
;
305318

319+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
320+
306321
emp_no:integer | language_code:integer
307322
10001 | 1
308323
10001 | 1
@@ -686,6 +701,7 @@ emp_no:integer | language_code:integer | language_name:keyword
686701

687702
filterOnRightSideOnTheCoordinator
688703
required_capability: join_lookup_v12
704+
required_capability: lookup_join_sort_warning
689705

690706
FROM employees
691707
| SORT emp_no
@@ -696,12 +712,15 @@ FROM employees
696712
| KEEP emp_no, language_code, language_name
697713
;
698714

715+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
716+
699717
emp_no:integer | language_code:integer | language_name:keyword
700718
10005 | 1 | English
701719
;
702720

703721
filterOnJoinKeyOnTheCoordinator
704722
required_capability: join_lookup_v12
723+
required_capability: lookup_join_sort_warning
705724

706725
FROM employees
707726
| SORT emp_no
@@ -712,12 +731,15 @@ FROM employees
712731
| KEEP emp_no, language_code, language_name
713732
;
714733

734+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
735+
715736
emp_no:integer | language_code:integer | language_name:keyword
716737
10005 | 1 | English
717738
;
718739

719740
filterOnJoinKeyAndRightSideOnTheCoordinator
720741
required_capability: join_lookup_v12
742+
required_capability: lookup_join_sort_warning
721743

722744
FROM employees
723745
| SORT emp_no
@@ -728,6 +750,8 @@ FROM employees
728750
| KEEP emp_no, language_code, language_name
729751
;
730752

753+
warning:Line 2:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
754+
731755
emp_no:integer | language_code:integer | language_name:keyword
732756
10001 | 2 | French
733757
10003 | 4 | German
@@ -1798,6 +1822,7 @@ joinMaskingRegex
17981822
required_capability: union_types
17991823
required_capability: join_lookup_v12
18001824
required_capability: fix_join_masking_regex_extract
1825+
required_capability: lookup_join_sort_warning
18011826
from books,message_*,ul*
18021827
| enrich languages_policy on status
18031828
| drop `language_name`, `bytes_out`, `id`, id
@@ -1819,6 +1844,8 @@ from books,message_*,ul*
18191844
| keep `ratings`, ratings
18201845
;
18211846

1847+
warning:Line 7:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
1848+
18221849
ratings:long
18231850
1
18241851
;
@@ -5686,6 +5713,7 @@ language_name:keyword | language_code:integer
56865713
lookupJoinWithSemanticFilterDeduplicationComplex
56875714
required_capability: join_lookup_v12
56885715
required_capability: lookup_join_semantic_filter_dedup
5716+
required_capability: lookup_join_sort_warning
56895717

56905718
from languages_lookup_non_unique_ke*
56915719
| enrich languages_policy on language_name
@@ -5702,6 +5730,7 @@ from languages_lookup_non_unique_ke*
57025730
| where NOT false AND other2 == 50
57035731
;
57045732

5733+
warning:Line 3:3: SORT is followed by a LOOKUP JOIN which does not preserve order; add another SORT after the LOOKUP JOIN if order is required
57055734
warning:Line 13:24: evaluation of [other2 == 50] failed, treating result as null. Only first 20 failures recorded.
57065735
warning:Line 13:24: java.lang.IllegalArgumentException: single-value function encountered multi-value
57075736

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1786,6 +1786,11 @@ public enum Cap {
17861786
*/
17871787
LOOKUP_JOIN_SEMANTIC_FILTER_DEDUP,
17881788

1789+
/**
1790+
* Warning when SORT is followed by LOOKUP JOIN which does not preserve order.
1791+
*/
1792+
LOOKUP_JOIN_SORT_WARNING,
1793+
17891794
/**
17901795
* Temporarily forbid the use of an explicit or implicit LIMIT before INLINE STATS.
17911796
*/

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@
7575
import org.elasticsearch.xpack.esql.optimizer.rules.logical.SubstituteSurrogateExpressions;
7676
import org.elasticsearch.xpack.esql.optimizer.rules.logical.SubstituteSurrogatePlans;
7777
import org.elasticsearch.xpack.esql.optimizer.rules.logical.TranslateTimeSeriesAggregate;
78+
import org.elasticsearch.xpack.esql.optimizer.rules.logical.WarnLostSortOrder;
7879
import org.elasticsearch.xpack.esql.optimizer.rules.logical.local.PruneLeftJoinOnNullMatchingField;
7980
import org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.TranslatePromqlToTimeSeriesAggregate;
8081
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
@@ -112,6 +113,7 @@ public class LogicalPlanOptimizer extends ParameterizedRuleExecutor<LogicalPlan,
112113
operators(),
113114
new Batch<>("Skip Compute", new SkipQueryOnLimitZero()),
114115
cleanup(),
116+
warnings(),
115117
new Batch<>("Set as Optimized", Limiter.ONCE, new SetAsOptimized())
116118
);
117119

@@ -248,4 +250,8 @@ protected static Batch<LogicalPlan> cleanup() {
248250
new CombineLimitTopN()
249251
);
250252
}
253+
254+
protected static Batch<LogicalPlan> warnings() {
255+
return new Batch<>("Warn On Plan Structure", Limiter.ONCE, new WarnLostSortOrder());
256+
}
251257
}
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.optimizer.rules.logical;
9+
10+
import org.elasticsearch.xpack.esql.core.tree.Source;
11+
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
12+
import org.elasticsearch.xpack.esql.plan.logical.OrderBy;
13+
import org.elasticsearch.xpack.esql.plan.logical.SortPreserving;
14+
import org.elasticsearch.xpack.esql.plan.logical.TopN;
15+
import org.elasticsearch.xpack.esql.plan.logical.join.Join;
16+
import org.elasticsearch.xpack.esql.rule.Rule;
17+
18+
import static org.elasticsearch.common.logging.HeaderWarning.addWarning;
19+
20+
/**
21+
* Warns when a SORT or TopN is followed by a LOOKUP JOIN which does not preserve order,
22+
* and there is no subsequent SORT to restore the order.
23+
* <p>
24+
* For example:
25+
* <pre>
26+
* FROM test | SORT x | LOOKUP JOIN lookup ON key | LIMIT 10
27+
* </pre>
28+
* will produce a warning because the SORT order is lost by the LOOKUP JOIN.
29+
* <p>
30+
* But:
31+
* <pre>
32+
* FROM test | SORT x | LOOKUP JOIN lookup ON key | SORT y | LIMIT 10
33+
* </pre>
34+
* will NOT produce a warning because the final SORT restores a defined order.
35+
*/
36+
public final class WarnLostSortOrder extends Rule<LogicalPlan, LogicalPlan> {
37+
38+
@Override
39+
public LogicalPlan apply(LogicalPlan plan) {
40+
// runs only once on the top node
41+
// does not do anything per node so no need to implement rule method
42+
warnLostSorts(plan, false);
43+
return plan;
44+
}
45+
46+
private void warnLostSorts(LogicalPlan plan, boolean seenJoin) {
47+
if (plan instanceof OrderBy || plan instanceof TopN) {
48+
if (seenJoin) {
49+
Source source = plan.source();
50+
addWarning(
51+
"Line {}:{}: SORT is followed by a LOOKUP JOIN which does not preserve order; "
52+
+ "add another SORT after the LOOKUP JOIN if order is required",
53+
source.source().getLineNumber(),
54+
source.source().getColumnNumber()
55+
);
56+
}
57+
// Stop traversing this branch - we found a sort.
58+
// If we there is a lookup join under we are fine because this sort is above it
59+
// and we already printed a warning if there was lookup join above this node
60+
// so we can stop traversing the tree here
61+
return;
62+
}
63+
64+
// Only warn for Join that doesn't preserve sort order
65+
// We do not warn for SortPreserving joins such as InlineStats
66+
boolean newSeenJoin = seenJoin || (plan instanceof Join && plan instanceof SortPreserving == false);
67+
68+
for (LogicalPlan child : plan.children()) {
69+
warnLostSorts(child, newSeenJoin);
70+
}
71+
}
72+
}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/HoistOrderByBeforeInlineJoinOptimizerTests.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,6 +1070,11 @@ public void testInlineJoinPrunedAfterSortAndLookupJoin() {
10701070

10711071
var rightRelation = as(join.right(), EsRelation.class);
10721072
assertThat(rightRelation.concreteQualifiedIndices(), is(Set.of("languages_lookup")));
1073+
assertWarnings(
1074+
"No limit defined, adding default limit of [1000]",
1075+
"Line 4:3: SORT is followed by a LOOKUP JOIN which does not preserve order; "
1076+
+ "add another SORT after the LOOKUP JOIN if order is required"
1077+
);
10731078
}
10741079

10751080
public void testFailureWhenSortAndSortBreakerBeforeInlineStats() {
@@ -1092,6 +1097,13 @@ public void testFailureWhenSortAndSortBreakerBeforeInlineStats() {
10921097
"line 5:3: INLINE STATS [INLINE STATS max_lang = MAX(languages) BY gender] cannot yet have an unbounded SORT"
10931098
+ " [SORT emp_no] before it"
10941099
);
1100+
if (cmd.startsWith("LOOKUP JOIN")) {
1101+
assertWarnings(
1102+
"No limit defined, adding default limit of [1000]",
1103+
"Line 3:3: SORT is followed by a LOOKUP JOIN which does not preserve order; "
1104+
+ "add another SORT after the LOOKUP JOIN if order is required"
1105+
);
1106+
}
10951107
}
10961108
}
10971109
}

0 commit comments

Comments
 (0)