Skip to content

Commit 5cdb9c1

Browse files
julian-elasticjulian-elastic
committed
Address code review comments and bugfix
1 parent b8f9ba6 commit 5cdb9c1

File tree

8 files changed

+425
-290
lines changed

8 files changed

+425
-290
lines changed

‎docs/reference/query-languages/esql/_snippets/commands/types/lookup-join.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,17 @@
55
| field from the left index | field from the lookup index |
66
| --- | --- |
77
| boolean | boolean |
8-
| byte | half_float, float, double, scaled_float, byte, short, integer, long |
8+
| byte | byte, short, integer, long, half_float, float, double, scaled_float |
99
| date | date |
1010
| date_nanos | date_nanos |
1111
| double | half_float, float, double, scaled_float, byte, short, integer, long |
1212
| float | half_float, float, double, scaled_float, byte, short, integer, long |
1313
| half_float | half_float, float, double, scaled_float, byte, short, integer, long |
14-
| integer | half_float, float, double, scaled_float, byte, short, integer, long |
14+
| integer | byte, short, integer, long, half_float, float, double, scaled_float |
1515
| ip | ip |
1616
| keyword | keyword |
17-
| long | half_float, float, double, scaled_float, byte, short, integer, long |
17+
| long | byte, short, integer, long, half_float, float, double, scaled_float |
1818
| scaled_float | half_float, float, double, scaled_float, byte, short, integer, long |
19-
| short | half_float, float, double, scaled_float, byte, short, integer, long |
19+
| short | byte, short, integer, long, half_float, float, double, scaled_float |
2020
| text | keyword |
2121

‎x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java‎

Lines changed: 389 additions & 194 deletions
Large diffs are not rendered by default.

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ static TransportRequest readFrom(StreamInput in, BlockFactory blockFactory) thro
265265
}
266266
Expression joinOnConditions = null;
267267
if (in.getTransportVersion().onOrAfter(TransportVersions.ESQL_LOOKUP_JOIN_ON_EXPRESSION)) {
268-
joinOnConditions = in.readOptionalNamedWriteable(Expression.class);
268+
joinOnConditions = planIn.readOptionalNamedWriteable(Expression.class);
269269
}
270270
TransportRequest result = new TransportRequest(
271271
sessionId,
@@ -332,7 +332,7 @@ public void writeTo(StreamOutput out) throws IOException {
332332
planOut.writeOptionalNamedWriteable(rightPreJoinPlan);
333333
}
334334
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_LOOKUP_JOIN_ON_EXPRESSION)) {
335-
out.writeOptionalNamedWriteable(joinOnConditions);
335+
planOut.writeOptionalNamedWriteable(joinOnConditions);
336336
} else {
337337
if (joinOnConditions != null) {
338338
throw new EsqlIllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/MatchConfig.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ public MatchConfig(FieldAttribute.FieldName fieldName, int channel, DataType typ
2929
}
3030

3131
public MatchConfig(FieldAttribute.FieldName fieldName, Layout.ChannelAndType input) {
32-
// TODO: Using exactAttribute was supposed to handle TEXT fields with KEYWORD subfields - but we don't allow these in lookup
33-
// indices, so the call to exactAttribute looks redundant now.
3432
this(fieldName, input.channel(), input.type());
3533
}
3634

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ protected LogicalPlan rule(Join join) {
4949
return join;
5050
}
5151

52-
if (join.left() instanceof Project project && join.config().type() == JoinTypes.LEFT && join.config().joinOnConditions() == null) {
52+
if (join.left() instanceof Project project && join.config().type() == JoinTypes.LEFT) {
5353
AttributeMap.Builder<Expression> aliasBuilder = AttributeMap.builder();
5454
project.forEachExpression(Alias.class, a -> aliasBuilder.put(a.toAttribute(), a.child()));
5555
var aliasesFromProject = aliasBuilder.build();

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -704,8 +704,9 @@ public JoinInfo visitExpressionBasedLookupJoin(EsqlBaseParser.ExpressionBasedLoo
704704
f = handleNegationOfEquals(f);
705705
if (f instanceof EsqlBinaryComparison comparison
706706
&& comparison.left() instanceof UnresolvedAttribute left
707-
&& comparison.right() instanceof UnresolvedAttribute) {
707+
&& comparison.right() instanceof UnresolvedAttribute right) {
708708
joinFields.add(left);
709+
joinFields.add(right);
709710
joinExpressions.add(f);
710711
} else {
711712
throw new ParsingException(

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

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8996,22 +8996,24 @@ public void testTranslateMetricsGroupedByTBucketInTSMode() {
89968996
}
89978997

89988998
/**
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{
8999+
*
9000+
* Project[[languages{f}#8, language_code{f}#16, language_name{f}#17]]
9001+
* \_Limit[1000[INTEGER],true]
9002+
* \_Join[LEFT,[languages{f}#8, language_code{f}#16],[languages{f}#8],[language_code{f}#16],languages{f}#8 == language_code{
90019003
* 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]
9004+
* |_Limit[1000[INTEGER],false]
9005+
* | \_EsRelation[test][_meta_field{f}#11, emp_no{f}#5, first_name{f}#6, ge..]
9006+
* \_EsRelation[languages_lookup][LOOKUP][language_code{f}#16, language_name{f}#17]
9007+
*
90069008
*/
90079009
public void testLookupJoinExpressionSwapped() {
90089010
LogicalPlan plan = optimizedPlan("""
90099011
from test
90109012
| keep languages
90119013
| lookup join languages_lookup ON language_code == languages
90129014
""");
9013-
9014-
var limit = asLimit(plan, 1000, true);
9015+
var project = as(plan, Project.class);
9016+
var limit = asLimit(project.child(), 1000, true);
90159017
var join = as(limit.child(), Join.class);
90169018
assertEquals("language_code == languages", join.config().joinOnConditions().toString());
90179019
var equals = as(join.config().joinOnConditions(), Equals.class);
@@ -9020,8 +9022,7 @@ public void testLookupJoinExpressionSwapped() {
90209022
var right = as(equals.right(), Attribute.class);
90219023
assertEquals("language_code", right.name());
90229024
assertEquals("languages", left.name());
9023-
var project = as(join.left(), EsqlProject.class);
9024-
var limitPastJoin = asLimit(project.child(), 1000, false);
9025+
var limitPastJoin = asLimit(join.left(), 1000, false);
90259026
as(limitPastJoin.child(), EsRelation.class);
90269027
as(join.right(), EsRelation.class);
90279028
}

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

Lines changed: 16 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -241,13 +241,11 @@ public void testShadowingAfterPushdown2() {
241241
}
242242

243243
/**
244-
* EsqlProject[[languages{f}#30, emp_no{f}#27, salary{f}#32]]
244+
* Project[[languages{f}#30, emp_no{f}#27, salary{f}#32]]
245245
* \_Limit[1000[INTEGER],true]
246-
* \_Join[LEFT,[lang{r}#10, languages{f}#30],[lang{r}#10],[languages{f}#30],lang{r}#10 == languages{f}#30]
247-
* |_EsqlProject[[_meta_field{f}#22, emp_no{f}#16, first_name{f}#17, gender{f}#18, hire_date{f}#23, job{f}#24, job.raw{f}#25,
248-
* languages{f}#19 AS lang2#4, last_name{f}#20, long_noidx{f}#26, salary{f}#21, emp_no{f}#16 AS lang#10]]
249-
* | \_Limit[1000[INTEGER],false]
250-
* | \_EsRelation[test][_meta_field{f}#22, emp_no{f}#16, first_name{f}#17, ..]
246+
* \_Join[LEFT,[emp_no{f}#16, languages{f}#30],[emp_no{f}#16],[languages{f}#30],emp_no{f}#16 == languages{f}#30]
247+
* |_Limit[1000[INTEGER],false]
248+
* | \_EsRelation[test][_meta_field{f}#22, emp_no{f}#16, first_name{f}#17, ..]
251249
* \_EsRelation[test_lookup][LOOKUP][emp_no{f}#27, languages{f}#30, salary{f}#32]
252250
*/
253251
public void testShadowingAfterPushdownExpressionJoin() {
@@ -267,8 +265,7 @@ public void testShadowingAfterPushdownExpressionJoin() {
267265
var limit1 = asLimit(project.child(), 1000, true);
268266
var join = as(limit1.child(), Join.class);
269267
var lookupRel = as(join.right(), EsRelation.class);
270-
var project2 = as(join.left(), Project.class);
271-
var limit2 = asLimit(project2.child(), 1000, false);
268+
var limit2 = asLimit(join.left(), 1000, false);
272269
var mainRel = as(limit2.child(), EsRelation.class);
273270

274271
var projections = project.projections();
@@ -285,33 +282,14 @@ public void testShadowingAfterPushdownExpressionJoin() {
285282
var salary = as(projections.get(2), FieldAttribute.class);
286283
assertEquals("salary", salary.fieldName().string());
287284
assertTrue(lookupRel.outputSet().contains(salary));
288-
289-
var project2Projections = project2.projections();
290-
var lang = as(project2Projections.stream().filter(p -> "lang".equals(p.name())).findFirst().get(), Alias.class);
291-
var langAttr = lang.toAttribute();
292-
var originalEmpNo = as(lang.child(), FieldAttribute.class);
293-
assertEquals("emp_no", originalEmpNo.fieldName().string());
294-
assertTrue(mainRel.outputSet().contains(originalEmpNo));
295-
296-
var joinConfig = join.config();
297-
assertSame(JoinTypes.LEFT, joinConfig.type());
298-
var leftKeys = joinConfig.leftFields();
299-
assertEquals(1, leftKeys.size());
300-
assertTrue(leftKeys.get(0).semanticEquals(langAttr));
301-
var rightKeys = joinConfig.rightFields();
302-
assertEquals(1, rightKeys.size());
303-
assertEquals("languages", rightKeys.get(0).name());
304-
assertTrue(lookupRel.outputSet().contains(rightKeys.get(0)));
305285
}
306286

307287
/**
308-
* EsqlProject[[languages{f}#24, emp_no{f}#21, salary{f}#26]]
288+
* Project[[languages{f}#24, emp_no{f}#21, salary{f}#26]]
309289
* \_Limit[1000[INTEGER],true]
310-
* \_Join[LEFT,[lang{r}#4, languages{f}#24],[lang{r}#4],[languages{f}#24],lang{r}#4 == languages{f}#24]
311-
* |_EsqlProject[[_meta_field{f}#16, emp_no{f}#10, first_name{f}#11, gender{f}#12, hire_date{f}#17, job{f}#18, job.raw{f}#19,
312-
* languages{f}#13 AS lang#4, last_name{f}#14, long_noidx{f}#20, salary{f}#15]]
313-
* | \_Limit[1000[INTEGER],false]
314-
* | \_EsRelation[test][_meta_field{f}#16, emp_no{f}#10, first_name{f}#11, ..]
290+
* \_Join[LEFT,[languages{f}#24, languages{f}#13],[languages{f}#13],[languages{f}#24],languages{f}#13 == languages{f}#24]
291+
* |_Limit[1000[INTEGER],false]
292+
* | \_EsRelation[test][_meta_field{f}#16, emp_no{f}#10, first_name{f}#11, ..]
315293
* \_EsRelation[test_lookup][LOOKUP][emp_no{f}#21, languages{f}#24, salary{f}#26]
316294
*/
317295
public void testShadowingAfterPushdownRenameExpressionJoin() {
@@ -329,8 +307,7 @@ public void testShadowingAfterPushdownRenameExpressionJoin() {
329307
var limit1 = asLimit(project.child(), 1000, true);
330308
var join = as(limit1.child(), Join.class);
331309
var lookupRel = as(join.right(), EsRelation.class);
332-
var project2 = as(join.left(), Project.class);
333-
var limit2 = asLimit(project2.child(), 1000, false);
310+
var limit2 = asLimit(join.left(), 1000, false);
334311
var mainRel = as(limit2.child(), EsRelation.class);
335312

336313
var projections = project.projections();
@@ -347,34 +324,15 @@ public void testShadowingAfterPushdownRenameExpressionJoin() {
347324
var salary = as(projections.get(2), FieldAttribute.class);
348325
assertEquals("salary", salary.fieldName().string());
349326
assertTrue(lookupRel.outputSet().contains(salary));
350-
351-
var project2Projections = project2.projections();
352-
var lang = as(project2Projections.stream().filter(p -> "lang".equals(p.name())).findFirst().get(), Alias.class);
353-
var langAttr = lang.toAttribute();
354-
var originalLanguages = as(lang.child(), FieldAttribute.class);
355-
assertEquals("languages", originalLanguages.fieldName().string());
356-
assertTrue(mainRel.outputSet().contains(originalLanguages));
357-
358-
var joinConfig = join.config();
359-
assertSame(JoinTypes.LEFT, joinConfig.type());
360-
var leftKeys = joinConfig.leftFields();
361-
assertEquals(1, leftKeys.size());
362-
assertTrue(leftKeys.get(0).semanticEquals(langAttr));
363-
var rightKeys = joinConfig.rightFields();
364-
assertEquals(1, rightKeys.size());
365-
assertEquals("languages", rightKeys.get(0).name());
366-
assertTrue(lookupRel.outputSet().contains(rightKeys.get(0)));
367327
}
368328

369329
/**
370-
* EsqlProject[[languages{f}#25, emp_no{f}#22, salary{f}#27]]
330+
* Project[[languages{f}#25, emp_no{f}#22, salary{f}#27]]
371331
* \_Limit[1000[INTEGER],true]
372-
* \_Join[LEFT,[languages{f}#25, lang{r}#4],[lang{r}#4],[languages{f}#25],lang{r}#4 == languages{f}#25]
373-
* |_EsqlProject[[_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, gender{f}#13, hire_date{f}#18, job{f}#19, job.raw{f}#20, l
374-
* ast_name{f}#15, long_noidx{f}#21, salary{f}#16, lang{r}#4]]
375-
* | \_Eval[[languages{f}#14 + 0[INTEGER] AS lang#4]]
376-
* | \_Limit[1000[INTEGER],false]
377-
* | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..]
332+
* \_Join[LEFT,[lang{r}#4, languages{f}#25],[lang{r}#4],[languages{f}#25],lang{r}#4 == languages{f}#25]
333+
* |_Eval[[languages{f}#14 + 0[INTEGER] AS lang#4]]
334+
* | \_Limit[1000[INTEGER],false]
335+
* | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..]
378336
* \_EsRelation[test_lookup][LOOKUP][emp_no{f}#22, languages{f}#25, salary{f}#27]
379337
*/
380338
public void testShadowingAfterPushdownEvalExpressionJoin() {
@@ -393,8 +351,7 @@ public void testShadowingAfterPushdownEvalExpressionJoin() {
393351
var limit1 = asLimit(project.child(), 1000, true);
394352
var join = as(limit1.child(), Join.class);
395353
var lookupRel = as(join.right(), EsRelation.class);
396-
var project2 = as(join.left(), Project.class);
397-
var eval = as(project2.child(), Eval.class);
354+
var eval = as(join.left(), Eval.class);
398355
var limit2 = asLimit(eval.child(), 1000, false);
399356
var mainRel = as(limit2.child(), EsRelation.class);
400357

@@ -412,23 +369,6 @@ public void testShadowingAfterPushdownEvalExpressionJoin() {
412369
var salary = as(projections.get(2), FieldAttribute.class);
413370
assertEquals("salary", salary.fieldName().string());
414371
assertTrue(lookupRel.outputSet().contains(salary));
415-
416-
var lang = as(eval.fields().get(0), Alias.class);
417-
var langAttr = lang.toAttribute();
418-
var add = as(lang.child(), org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Add.class);
419-
var originalLanguages = as(add.left(), FieldAttribute.class);
420-
assertEquals("languages", originalLanguages.fieldName().string());
421-
assertTrue(mainRel.outputSet().contains(originalLanguages));
422-
423-
var joinConfig = join.config();
424-
assertSame(JoinTypes.LEFT, joinConfig.type());
425-
var leftKeys = joinConfig.leftFields();
426-
assertEquals(1, leftKeys.size());
427-
assertTrue(leftKeys.get(0).semanticEquals(langAttr));
428-
var rightKeys = joinConfig.rightFields();
429-
assertEquals(1, rightKeys.size());
430-
assertEquals("languages", rightKeys.get(0).name());
431-
assertTrue(lookupRel.outputSet().contains(rightKeys.get(0)));
432372
}
433373

434374
private static void assertLeftJoinConfig(

0 commit comments

Comments
 (0)