Skip to content

Commit b10c360

Browse files
Address last code review comments
1 parent d8076d2 commit b10c360

File tree

14 files changed

+173
-59
lines changed

14 files changed

+173
-59
lines changed

test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -776,7 +776,6 @@ public void testLookupExplosionManyMatches() throws IOException {
776776
}
777777

778778
public void testLookupExplosionManyMatchesExpression() throws IOException {
779-
// 1500, 10000 is enough locally, but some CI machines need more.
780779
int lookupEntries = 10000;
781780
assertCircuitBreaks(attempt -> lookupExplosion(attempt * 1500, lookupEntries, 1, lookupEntries, true));
782781
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.esql.analysis;
99

10+
import org.elasticsearch.xpack.esql.VerificationException;
1011
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1112
import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute;
1213
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
@@ -116,7 +117,6 @@ public static List<Attribute> maybeResolveAgainstList(
116117
int colDiff = a.sourceLocation().getColumnNumber() - b.sourceLocation().getColumnNumber();
117118
return lineDiff != 0 ? lineDiff : (colDiff != 0 ? colDiff : a.name().compareTo(b.name()));
118119
}).map(a -> "line " + a.sourceLocation().toString().substring(1) + " [" + a.name() + "]").toList();
119-
120-
throw new IllegalStateException("Reference [" + ua.name() + "] is ambiguous; " + "matches any of " + refs);
120+
throw new VerificationException("Found ambiguous reference to [" + ua.name() + "]; " + "matches any of " + refs);
121121
}
122122
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@
3333
* This class is used in the context of an expression based lookup join,
3434
* where we need to generate a query for each row of the left dataset.
3535
* The query is then used to fetch the matching rows from the right dataset.
36-
* The query is a binary comparison between a field from the right dataset and a value from the left dataset.
37-
* The field is on the left side of the comparison and the value is on the right side.
36+
* The query is a binary comparison between a field from the right dataset
37+
* and a single value from the left dataset. e.g right_id > 5
3838
* The value is extracted from a block at a given position.
3939
* The comparison is then translated to a Lucene query.
4040
* If the comparison cannot be translated, an exception is thrown.

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

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
* The query is then used to fetch the matching rows from the right dataset.
5050
* The class supports two types of joins:
5151
* 1. Field-based join: The join conditions are based on the equality of fields from the left and right datasets.
52+
* It is used for field-based join when the join is on more than one field or there is a preJoinFilter
5253
* 2. Expression-based join: The join conditions are based on a complex expression that can involve multiple fields and operators.
5354
*/
5455
public class ExpressionQueryList implements LookupEnrichQueryGenerator {
@@ -76,13 +77,6 @@ private ExpressionQueryList(
7677
* For example | LOOKUP JOIN on field1, field2, field3
7778
* The query lists are generated from the join conditions.
7879
* The pre-join filter is an optional filter that is applied to the right dataset before the join.
79-
* @param queryLists The list of query lists that will be combined.
80-
* @param context The search execution context.
81-
* @param rightPreJoinPlan The physical plan for the right side of the join.
82-
* @param clusterService The cluster service.
83-
* @param aliasFilter The alias filter.
84-
* @return A new {@link ExpressionQueryList} for a field-based join.
85-
* @throws IllegalArgumentException if the number of query lists is less than 2 and there is no pre-join filter.
8680
*/
8781
public static ExpressionQueryList fieldBasedJoin(
8882
List<QueryList> queryLists,
@@ -104,14 +98,6 @@ public static ExpressionQueryList fieldBasedJoin(
10498
* Example | LOOKUP JOIN on left_field > right_field AND left_field2 == right_field2
10599
* The query lists are generated from the join conditions.
106100
* The pre-join filter is an optional filter that is applied to the right dataset before the join.
107-
* @param context The search execution context.
108-
* @param rightPreJoinPlan The physical plan for the right side of the join.
109-
* @param clusterService The cluster service.
110-
* @param request The transport request.
111-
* @param aliasFilter The alias filter.
112-
* @param warnings The warnings.
113-
* @return A new {@link ExpressionQueryList} for an expression-based join.
114-
* @throws IllegalStateException if the join conditions are null.
115101
*/
116102
public static ExpressionQueryList expressionBasedJoin(
117103
SearchExecutionContext context,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ public void writeTo(StreamOutput out) throws IOException {
337337
planOut.writeOptionalNamedWriteable(joinOnConditions);
338338
} else {
339339
if (joinOnConditions != null) {
340-
throw new EsqlIllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
340+
throw new IllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
341341
}
342342
}
343343
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ private JoinInfo processFieldBasedJoin(List<Expression> expressions) {
733733
} else {
734734
throw new ParsingException(
735735
f.source(),
736-
"JOIN ON clause must be a comma separated list of fields or a single expression found [{}]",
736+
"JOIN ON clause must be a comma separated list of fields or a single expression, found [{}]",
737737
f.sourceText()
738738
);
739739
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.elasticsearch.common.io.stream.StreamOutput;
1313
import org.elasticsearch.common.io.stream.Writeable;
1414
import org.elasticsearch.core.Nullable;
15-
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
1615
import org.elasticsearch.xpack.esql.core.capabilities.Resolvables;
1716
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1817
import org.elasticsearch.xpack.esql.core.expression.Expression;
@@ -61,7 +60,7 @@ public void writeTo(StreamOutput out) throws IOException {
6160
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_LOOKUP_JOIN_ON_EXPRESSION)) {
6261
out.writeOptionalNamedWriteable(joinOnConditions);
6362
} else if (joinOnConditions != null) {
64-
throw new EsqlIllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
63+
throw new IllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
6564
}
6665
}
6766

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1212
import org.elasticsearch.common.io.stream.StreamInput;
1313
import org.elasticsearch.common.io.stream.StreamOutput;
14-
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
1514
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1615
import org.elasticsearch.xpack.esql.core.expression.AttributeSet;
1716
import org.elasticsearch.xpack.esql.core.expression.Expression;
@@ -80,7 +79,7 @@ public void writeTo(StreamOutput out) throws IOException {
8079
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_LOOKUP_JOIN_ON_EXPRESSION)) {
8180
out.writeOptionalNamedWriteable(joinOnConditions);
8281
} else if (joinOnConditions != null) {
83-
throw new EsqlIllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
82+
throw new IllegalArgumentException("LOOKUP JOIN with ON conditions is not supported on remote node");
8483
}
8584
}
8685

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2218,6 +2218,48 @@ public void testLookupJoinDataTypeMismatch() {
22182218
);
22192219
}
22202220

2221+
public void testLookupJoinExpressionAmbiguousRight() {
2222+
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V12.isEnabled());
2223+
String queryString = """
2224+
from test
2225+
| rename languages as language_code
2226+
| lookup join languages_lookup ON salary == language_code
2227+
""";
2228+
2229+
assertEquals(
2230+
" ambiguous reference to [language_code]; matches any of [line 2:10 [language_code], line 3:15 [language_code]]",
2231+
error(queryString)
2232+
);
2233+
}
2234+
2235+
public void testLookupJoinExpressionAmbiguousLeft() {
2236+
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V12.isEnabled());
2237+
String queryString = """
2238+
from test
2239+
| rename languages as language_name
2240+
| lookup join languages_lookup ON language_name == language_code
2241+
""";
2242+
2243+
assertEquals(
2244+
" ambiguous reference to [language_name]; matches any of [line 2:10 [language_name], line 3:15 [language_name]]",
2245+
error(queryString)
2246+
);
2247+
}
2248+
2249+
public void testLookupJoinExpressionAmbiguousBoth() {
2250+
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V12.isEnabled());
2251+
String queryString = """
2252+
from test
2253+
| rename languages as language_code
2254+
| lookup join languages_lookup ON language_code != language_code
2255+
""";
2256+
2257+
assertEquals(
2258+
" ambiguous reference to [language_code]; matches any of [line 2:10 [language_code], line 3:15 [language_code]]",
2259+
error(queryString)
2260+
);
2261+
}
2262+
22212263
public void testFullTextFunctionOptions() {
22222264
checkOptionDataTypes(Match.ALLOWED_OPTIONS, "FROM test | WHERE match(title, \"Jean\", {\"%s\": %s})");
22232265
checkOptionDataTypes(QueryString.ALLOWED_OPTIONS, "FROM test | WHERE QSTR(\"title: Jean\", {\"%s\": %s})");

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperatorTests.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,7 @@ protected MapMatcher extendStatusMatcher(MapMatcher mapMatcher, List<Page> input
455455

456456
@Override
457457
public void testSimpleCircuitBreaking() {
458+
// only test field based join and EQ to prevents timeouts in Ci
458459
if (operation == null || operation.equals(EsqlBinaryComparison.BinaryComparisonOperation.EQ)) {
459460
super.testSimpleCircuitBreaking();
460461
}

0 commit comments

Comments
 (0)