Skip to content

Commit 00fc5a4

Browse files
julian-elasticjulian-elastic
committed
Add more UTs and bugfixes
1 parent d253a55 commit 00fc5a4

File tree

5 files changed

+211
-21
lines changed

5 files changed

+211
-21
lines changed

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

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,45 @@ warning:Line 3:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON lef
370370
warning:Line 3:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
371371

372372

373+
left_id:integer | left_name:keyword | id_int:integer | name_str:keyword | is_active_left:boolean | is_active_bool:boolean
374+
1 | Alice | null | null | true | null
375+
[1, 19, 21] | Sophia | null | null | true | null
376+
2 | Bob | null | null | false | null
377+
3 | Charlie | 1 | Alice | true | true
378+
3 | Charlie | 1 | Alice | true | true
379+
4 | David | 2 | Bob | false | false
380+
4 | David | 3 | Charlie | false | false
381+
5 | Eve | 1 | Alice | true | true
382+
5 | Eve | 1 | Alice | true | true
383+
5 | Eve | 3 | Charlie | true | true
384+
6 | null | 1 | Alice | true | true
385+
6 | null | 1 | Alice | true | true
386+
6 | null | 3 | Charlie | true | true
387+
6 | null | 5 | Eve | true | true
388+
6 | null | 5 | Eve | true | true
389+
7 | Grace | 2 | Bob | false | false
390+
7 | Grace | 3 | Charlie | false | false
391+
7 | Grace | 4 | David | false | false
392+
8 | Hank | 1 | Alice | true | true
393+
8 | Hank | 1 | Alice | true | true
394+
;
395+
396+
lookupMultiColMixedGtEqSwapLeftRight
397+
required_capability: join_lookup_v12
398+
required_capability: lookup_join_on_boolean_expression
399+
400+
FROM multi_column_joinable
401+
| RENAME id_int AS left_id, name_str AS left_name, is_active_bool AS is_active_left
402+
| LOOKUP JOIN multi_column_joinable_lookup ON id_int < left_id AND is_active_left == is_active_bool
403+
| KEEP left_id, left_name, id_int, name_str, is_active_left, is_active_bool
404+
| SORT left_id, left_name, id_int, name_str, is_active_left, is_active_left
405+
| LIMIT 20
406+
;
407+
408+
warning:Line 3:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_int < left_id AND is_active_left == is_active_bool] failed, treating result as null. Only first 20 failures recorded.
409+
warning:Line 3:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
410+
411+
373412
left_id:integer | left_name:keyword | id_int:integer | name_str:keyword | is_active_left:boolean | is_active_bool:boolean
374413
1 | Alice | null | null | true | null
375414
[1, 19, 21] | Sophia | null | null | true | null
@@ -512,3 +551,117 @@ left_id:integer | left_name:keyword | left_is_active:boolean | left_ip:ip | id_
512551
2 | Bob | false | 192.168.1.3 | 14 | Nina | true | 192.168.1.14
513552
2 | Bob | false | 192.168.1.3 | 16 | Paul | true | 192.168.1.16
514553
;
554+
555+
556+
lookupJoinOnSameLeftFieldTwice
557+
required_capability: join_lookup_v12
558+
required_capability: lookup_join_on_boolean_expression
559+
560+
FROM multi_column_joinable
561+
| RENAME id_int AS id_left, name_str AS name_left, is_active_bool AS is_active_left, ip_addr AS ip_addr_left
562+
| LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND id_left < other2
563+
| KEEP id_left, name_left, extra1, other1, other2
564+
| SORT id_left, name_left, extra1, other1, other2
565+
;
566+
567+
warning:Line 3:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND id_left < other2] failed, treating result as null. Only first 20 failures recorded.
568+
warning:Line 3:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
569+
570+
id_left:integer | name_left:keyword | extra1:keyword | other1:keyword | other2:integer
571+
1 | Alice | foo | alpha | 1000
572+
1 | Alice | foo | beta | 2000
573+
[1, 19, 21] | Sophia | zyx | null | null
574+
2 | Bob | bar | gamma | 3000
575+
3 | Charlie | baz | delta | 4000
576+
3 | Charlie | baz | epsilon | 5000
577+
4 | David | qux | zeta | 6000
578+
5 | Eve | quux | eta | 7000
579+
5 | Eve | quux | theta | 8000
580+
6 | null | corge | null | null
581+
7 | Grace | grault | kappa | 10000
582+
8 | Hank | garply | lambda | 11000
583+
9 | Ivy | waldo | null | null
584+
10 | John | fred | null | null
585+
12 | Liam | xyzzy | nu | 13000
586+
13 | Mia | thud | xi | 14000
587+
14 | Nina | foo2 | omicron | 15000
588+
15 | Oscar | bar2 | null | null
589+
[17, 18] | Olivia | xyz | null | null
590+
null | Kate | plugh | null | null
591+
;
592+
593+
lookupJoinOnSameRightFieldTwice
594+
required_capability: join_lookup_v12
595+
required_capability: lookup_join_on_boolean_expression
596+
597+
FROM multi_column_joinable
598+
| RENAME id_int AS id_left, name_str AS name_left, is_active_bool AS is_active_left, ip_addr AS ip_addr_left
599+
| LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND extra2 > id_int
600+
| KEEP id_left, name_left, extra1, other1, other2
601+
| SORT id_left, name_left, extra1, other1, other2
602+
;
603+
604+
warning:Line 3:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND extra2 > id_int] failed, treating result as null. Only first 20 failures recorded.
605+
warning:Line 3:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
606+
607+
608+
id_left:integer | name_left:keyword | extra1:keyword | other1:keyword | other2:integer
609+
1 | Alice | foo | alpha | 1000
610+
1 | Alice | foo | beta | 2000
611+
[1, 19, 21] | Sophia | zyx | null | null
612+
2 | Bob | bar | gamma | 3000
613+
3 | Charlie | baz | delta | 4000
614+
3 | Charlie | baz | epsilon | 5000
615+
4 | David | qux | zeta | 6000
616+
5 | Eve | quux | eta | 7000
617+
5 | Eve | quux | theta | 8000
618+
6 | null | corge | null | null
619+
7 | Grace | grault | kappa | 10000
620+
8 | Hank | garply | lambda | 11000
621+
9 | Ivy | waldo | null | null
622+
10 | John | fred | null | null
623+
12 | Liam | xyzzy | nu | 13000
624+
13 | Mia | thud | xi | 14000
625+
14 | Nina | foo2 | omicron | 15000
626+
15 | Oscar | bar2 | null | null
627+
[17, 18] | Olivia | xyz | null | null
628+
null | Kate | plugh | null | null
629+
;
630+
631+
lookupJoinOnSameRightFieldTwiceEval
632+
required_capability: join_lookup_v12
633+
required_capability: lookup_join_on_boolean_expression
634+
635+
FROM multi_column_joinable
636+
| RENAME id_int AS id_left, name_str AS name_left, is_active_bool AS is_active_left, ip_addr AS ip_addr_left
637+
| EVAL left_id2 = ((extra2 / 200)::integer)*2
638+
| LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND left_id2 != id_int
639+
| KEEP id_left, name_left, extra1, other1, other2, left_id2
640+
| SORT id_left, name_left, extra1, other1, other2, left_id2
641+
;
642+
643+
warning:Line 4:3: evaluation of [LOOKUP JOIN multi_column_joinable_lookup ON id_left == id_int AND name_left == name_str AND left_id2 != id_int] failed, treating result as null. Only first 20 failures recorded.
644+
warning:Line 4:3: java.lang.IllegalArgumentException: LOOKUP JOIN encountered multi-value
645+
646+
id_left:integer | name_left:keyword | extra1:keyword | other1:keyword | other2:integer | left_id2:integer
647+
1 | Alice | foo | alpha | 1000 | 0
648+
1 | Alice | foo | beta | 2000 | 0
649+
[1, 19, 21] | Sophia | zyx | null | null | 210
650+
2 | Bob | bar | null | null | 2
651+
3 | Charlie | baz | delta | 4000 | 2
652+
3 | Charlie | baz | epsilon | 5000 | 2
653+
4 | David | qux | null | null | 4
654+
5 | Eve | quux | eta | 7000 | 4
655+
5 | Eve | quux | theta | 8000 | 4
656+
6 | null | corge | null | null | 6
657+
7 | Grace | grault | kappa | 10000 | 6
658+
8 | Hank | garply | null | null | 8
659+
9 | Ivy | waldo | null | null | 8
660+
10 | John | fred | null | null | 10
661+
12 | Liam | xyzzy | null | null | 12
662+
13 | Mia | thud | xi | 14000 | 12
663+
14 | Nina | foo2 | null | null | 14
664+
15 | Oscar | bar2 | null | null | 14
665+
[17, 18] | Olivia | xyz | null | null | 170
666+
null | Kate | plugh | null | null | 10
667+
;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,11 @@ private LogicalPlan resolveLookup(Lookup l, List<Attribute> childrenOutput) {
720720
return l;
721721
}
722722

723-
private List<Expression> resolveJoinFilters(List<Expression> filters, List<Attribute> leftOutput, List<Attribute> rightOutput) {
723+
private List<Expression> resolveJoinFiltersAndSwapIfNeeded(
724+
List<Expression> filters,
725+
List<Attribute> leftOutput,
726+
List<Attribute> rightOutput
727+
) {
724728
if (filters.isEmpty()) {
725729
return emptyList();
726730
}
@@ -729,7 +733,24 @@ private List<Expression> resolveJoinFilters(List<Expression> filters, List<Attri
729733

730734
List<Expression> resolvedFilters = new ArrayList<>(filters.size());
731735
for (Expression filter : filters) {
732-
resolvedFilters.add(filter.transformUp(UnresolvedAttribute.class, ua -> maybeResolveAttribute(ua, childrenOutput)));
736+
Expression filterResolved = filter.transformUp(UnresolvedAttribute.class, ua -> maybeResolveAttribute(ua, childrenOutput));
737+
if (filterResolved instanceof EsqlBinaryComparison binaryComparison) {
738+
if (binaryComparison.left() instanceof Attribute attributeLeft
739+
&& binaryComparison.right() instanceof Attribute attributeRight) {
740+
if (leftOutput.contains(attributeLeft) && rightOutput.contains(attributeRight)) {
741+
// left is from left side and right is from right side, all good
742+
} else if (leftOutput.contains(attributeRight) && rightOutput.contains(attributeLeft)) {
743+
// left is from right side and right is from left side, need to swap
744+
filterResolved = binaryComparison.swapLeftAndRight();
745+
} else {
746+
// both sides are from the same side, this is not supported in join condition
747+
throw new EsqlIllegalArgumentException(
748+
"Join condition must be between attribute on the left and right side, found: " + filter
749+
);
750+
}
751+
}
752+
}
753+
resolvedFilters.add(filterResolved);
733754
}
734755
return resolvedFilters;
735756
}
@@ -763,7 +784,7 @@ private Join resolveLookupJoin(LookupJoin join) {
763784
List<Expression> resolvedFilters = new ArrayList<>();
764785
List<Attribute> matchKeys;
765786
if (join.config().joinOnConditions() != null) {
766-
resolvedFilters = resolveJoinFilters(
787+
resolvedFilters = resolveJoinFiltersAndSwapIfNeeded(
767788
Predicates.splitAnd(join.config().joinOnConditions()),
768789
join.left().output(),
769790
join.right().output()

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -715,7 +715,6 @@ public JoinInfo visitExpressionBasedLookupJoin(EsqlBaseParser.ExpressionBasedLoo
715715
);
716716
}
717717
}
718-
validateJoinFields(joinFields);
719718
return new JoinInfo(joinFields, joinExpressions);
720719
}
721720

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -232,23 +232,9 @@ public static List<Attribute> computeOutput(List<Attribute> leftOutput, List<Att
232232
.toList();
233233
output = mergeOutputAttributes(rightOutputWithoutMatchFields, leftOutput);
234234
} else {
235-
// suppose we have
236-
// index1 a, b, c, d
237-
// index2 c, d, e, f
238-
// we have expression join on (index1.b > index2.c AND index1.d > index2.d)
239-
// We keep a,b,d from the left side (a, b only on left, d is join key, so we keep it from the left)
240-
// we keep c,e,f from the right side(c is not part of the join key on the left so gets shadowed, e,f only on right)
241-
Set<String> leftJoinKeyNames = config.leftFields()
242-
.stream()
243-
.map(Attribute::name)
244-
.collect(java.util.stream.Collectors.toSet());
245-
// as we can do (left_key > right_key) now as join condition, we want to preserve the right_key
246-
// unless it is also a key on the left side,
247-
// but we need to do name equality only
248-
List<Attribute> rightOutputWithoutMatchFields = rightOutput.stream()
249-
.filter(attr -> leftJoinKeyNames.contains(attr.name()) == false)
250-
.toList();
251-
output = mergeOutputAttributes(rightOutputWithoutMatchFields, leftOutput);
235+
// We don't allow any attributes in the joinOnConditions that don't have unique names
236+
// so right always overwrites left in case of name clashes
237+
output = mergeOutputAttributes(rightOutput, leftOutput);
252238
}
253239

254240
} else {

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8994,4 +8994,35 @@ public void testTranslateMetricsGroupedByTBucketInTSMode() {
89948994
Bucket bucket = as(Alias.unwrap(eval.fields().get(0)), Bucket.class);
89958995
assertThat(Expressions.attribute(bucket.field()).name(), equalTo("@timestamp"));
89968996
}
8997+
8998+
/**
8999+
* Limit[1000[INTEGER],true]
9000+
* \_Join[LEFT,[languages{f}#8, language_code{f}#16],[languages{f}#8],[language_code{f}#16],languages{f}#8 == language_code{
9001+
* f}#16]
9002+
* |_EsqlProject[[languages{f}#8]]
9003+
* | \_Limit[1000[INTEGER],false]
9004+
* | \_EsRelation[test][_meta_field{f}#11, emp_no{f}#5, first_name{f}#6, ge..]
9005+
* \_EsRelation[languages_lookup][LOOKUP][language_code{f}#16, language_name{f}#17]
9006+
*/
9007+
public void testLookupJoinExpressionSwapped() {
9008+
LogicalPlan plan = optimizedPlan("""
9009+
from test
9010+
| keep languages
9011+
| lookup join languages_lookup ON language_code == languages
9012+
""");
9013+
9014+
var limit = asLimit(plan, 1000, true);
9015+
var join = as(limit.child(), Join.class);
9016+
assertEquals("language_code == languages", join.config().joinOnConditions().toString());
9017+
var equals = as(join.config().joinOnConditions(), Equals.class);
9018+
// we expect left and right to be swapped
9019+
var left = as(equals.left(), Attribute.class);
9020+
var right = as(equals.right(), Attribute.class);
9021+
assertEquals("language_code", right.name());
9022+
assertEquals("languages", left.name());
9023+
var project = as(join.left(), EsqlProject.class);
9024+
var limitPastJoin = asLimit(project.child(), 1000, false);
9025+
as(limitPastJoin.child(), EsRelation.class);
9026+
as(join.right(), EsRelation.class);
9027+
}
89979028
}

0 commit comments

Comments
 (0)