Skip to content

Commit f5e4de7

Browse files
committed
SQL: Fix issue with IN not resolving to underlying keyword field (#38440)
- Add resolution to the exact keyword field (if exists) for text fields. - Add proper verification and error message if underlying keyword doesn'texist. - Move check for field attribute in the comparison list to the `resolveType()` method of `IN`. Fixes: #38424
1 parent 60a7c7d commit f5e4de7

File tree

4 files changed

+56
-30
lines changed

4 files changed

+56
-30
lines changed

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java

+23
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@
55
*/
66
package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;
77

8+
import org.elasticsearch.xpack.sql.analysis.index.MappingException;
89
import org.elasticsearch.xpack.sql.expression.Expression;
910
import org.elasticsearch.xpack.sql.expression.Expressions;
11+
import org.elasticsearch.xpack.sql.expression.FieldAttribute;
1012
import org.elasticsearch.xpack.sql.expression.Foldables;
1113
import org.elasticsearch.xpack.sql.expression.Nullability;
1214
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
@@ -105,6 +107,27 @@ protected Pipe makePipe() {
105107
return new InPipe(source(), this, children().stream().map(Expressions::pipe).collect(Collectors.toList()));
106108
}
107109

110+
@Override
111+
protected TypeResolution resolveType() {
112+
if (value instanceof FieldAttribute) {
113+
try {
114+
((FieldAttribute) value).exactAttribute();
115+
} catch (MappingException e) {
116+
return new TypeResolution(format(null, "[{}] cannot operate on field of data type [{}]: {}",
117+
functionName(), value().dataType().esType, e.getMessage()));
118+
}
119+
}
120+
121+
for (Expression ex : list) {
122+
if (ex.foldable() == false) {
123+
return new TypeResolution(format(null, "Comparisons against variables are not (currently) supported; offender [{}] in [{}]",
124+
Expressions.name(ex),
125+
name()));
126+
}
127+
}
128+
return super.resolveType();
129+
}
130+
108131
@Override
109132
public int hashCode() {
110133
return Objects.hash(value, list);

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+3-12
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,6 @@
105105
import java.util.List;
106106
import java.util.Map;
107107
import java.util.Map.Entry;
108-
import java.util.Optional;
109108
import java.util.function.Supplier;
110109

111110
import static java.util.Collections.singletonList;
@@ -708,16 +707,6 @@ static class InComparisons extends ExpressionTranslator<In> {
708707

709708
@Override
710709
protected QueryTranslation asQuery(In in, boolean onAggs) {
711-
Optional<Expression> firstNotFoldable = in.list().stream().filter(expression -> !expression.foldable()).findFirst();
712-
713-
if (firstNotFoldable.isPresent()) {
714-
throw new SqlIllegalArgumentException(
715-
"Line {}:{}: Comparisons against variables are not (currently) supported; offender [{}] in [{}]",
716-
firstNotFoldable.get().sourceLocation().getLineNumber(),
717-
firstNotFoldable.get().sourceLocation().getColumnNumber(),
718-
Expressions.name(firstNotFoldable.get()),
719-
in.name());
720-
}
721710

722711
if (in.value() instanceof NamedExpression) {
723712
NamedExpression ne = (NamedExpression) in.value();
@@ -735,7 +724,9 @@ protected QueryTranslation asQuery(In in, boolean onAggs) {
735724
else {
736725
Query q = null;
737726
if (in.value() instanceof FieldAttribute) {
738-
q = new TermsQuery(in.source(), ne.name(), in.list());
727+
FieldAttribute fa = (FieldAttribute) in.value();
728+
// equality should always be against an exact match (which is important for strings)
729+
q = new TermsQuery(in.source(), fa.isInexact() ? fa.exactAttribute().name() : fa.name(), in.list());
739730
} else {
740731
q = new ScriptQuery(in.source(), in.asScript());
741732
}

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

+19-8
Original file line numberDiff line numberDiff line change
@@ -385,23 +385,34 @@ public void testInNestedWithDifferentDataTypesFromLeftValue_SelectClause() {
385385
}
386386

387387
public void testInWithDifferentDataTypes_WhereClause() {
388-
assertEquals("1:49: expected data type [text], value provided is of type [integer]",
389-
error("SELECT * FROM test WHERE text IN ('foo', 'bar', 4)"));
388+
assertEquals("1:52: expected data type [keyword], value provided is of type [integer]",
389+
error("SELECT * FROM test WHERE keyword IN ('foo', 'bar', 4)"));
390390
}
391391

392392
public void testInNestedWithDifferentDataTypes_WhereClause() {
393-
assertEquals("1:60: expected data type [text], value provided is of type [integer]",
394-
error("SELECT * FROM test WHERE int = 1 OR text IN ('foo', 'bar', 2)"));
393+
assertEquals("1:63: expected data type [keyword], value provided is of type [integer]",
394+
error("SELECT * FROM test WHERE int = 1 OR keyword IN ('foo', 'bar', 2)"));
395395
}
396396

397397
public void testInWithDifferentDataTypesFromLeftValue_WhereClause() {
398-
assertEquals("1:35: expected data type [text], value provided is of type [integer]",
399-
error("SELECT * FROM test WHERE text IN (1, 2)"));
398+
assertEquals("1:38: expected data type [keyword], value provided is of type [integer]",
399+
error("SELECT * FROM test WHERE keyword IN (1, 2)"));
400400
}
401401

402402
public void testInNestedWithDifferentDataTypesFromLeftValue_WhereClause() {
403-
assertEquals("1:46: expected data type [text], value provided is of type [integer]",
404-
error("SELECT * FROM test WHERE int = 1 OR text IN (1, 2)"));
403+
assertEquals("1:49: expected data type [keyword], value provided is of type [integer]",
404+
error("SELECT * FROM test WHERE int = 1 OR keyword IN (1, 2)"));
405+
}
406+
407+
public void testInWithFieldInListOfValues() {
408+
assertEquals("1:26: Comparisons against variables are not (currently) supported; offender [int] in [int IN (1, int)]",
409+
error("SELECT * FROM test WHERE int IN (1, int)"));
410+
}
411+
412+
public void testInOnFieldTextWithNoKeyword() {
413+
assertEquals("1:26: [IN] cannot operate on field of data type [text]: " +
414+
"No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead",
415+
error("SELECT * FROM test WHERE text IN ('foo', 'bar')"));
405416
}
406417

407418
public void testNotSupportedAggregateOnDate() {

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

+11-10
Original file line numberDiff line numberDiff line change
@@ -309,8 +309,8 @@ public void testTranslateInExpression_WhereClause() {
309309
tq.asBuilder().toString().replaceAll("\\s", ""));
310310
}
311311

312-
public void testTranslateInExpression_WhereClauseAndNullHandling() {
313-
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', null, 'lala', null, 'foo', concat('la', 'la'))");
312+
public void testTranslateInExpression_WhereClause_TextFieldWithKeyword() {
313+
LogicalPlan p = plan("SELECT * FROM test WHERE some.string IN ('foo', 'bar', 'lala', 'foo', concat('la', 'la'))");
314314
assertTrue(p instanceof Project);
315315
assertTrue(p.children().get(0) instanceof Filter);
316316
Expression condition = ((Filter) p.children().get(0)).condition();
@@ -319,21 +319,22 @@ public void testTranslateInExpression_WhereClauseAndNullHandling() {
319319
Query query = translation.query;
320320
assertTrue(query instanceof TermsQuery);
321321
TermsQuery tq = (TermsQuery) query;
322-
assertEquals("{\"terms\":{\"keyword\":[\"foo\",\"lala\"],\"boost\":1.0}}",
322+
assertEquals("{\"terms\":{\"some.string.typical\":[\"foo\",\"bar\",\"lala\"],\"boost\":1.0}}",
323323
tq.asBuilder().toString().replaceAll("\\s", ""));
324324
}
325325

326-
public void testTranslateInExpressionInvalidValues_WhereClause() {
327-
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', 'bar', keyword)");
326+
public void testTranslateInExpression_WhereClauseAndNullHandling() {
327+
LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', null, 'lala', null, 'foo', concat('la', 'la'))");
328328
assertTrue(p instanceof Project);
329329
assertTrue(p.children().get(0) instanceof Filter);
330330
Expression condition = ((Filter) p.children().get(0)).condition();
331331
assertFalse(condition.foldable());
332-
SqlIllegalArgumentException ex = expectThrows(SqlIllegalArgumentException.class, () -> QueryTranslator.toQuery(condition, false));
333-
assertEquals(
334-
"Line 1:52: Comparisons against variables are not (currently) supported; "
335-
+ "offender [keyword] in [keyword IN ('foo', 'bar', keyword)]",
336-
ex.getMessage());
332+
QueryTranslation translation = QueryTranslator.toQuery(condition, false);
333+
Query query = translation.query;
334+
assertTrue(query instanceof TermsQuery);
335+
TermsQuery tq = (TermsQuery) query;
336+
assertEquals("{\"terms\":{\"keyword\":[\"foo\",\"lala\"],\"boost\":1.0}}",
337+
tq.asBuilder().toString().replaceAll("\\s", ""));
337338
}
338339

339340
public void testTranslateInExpression_WhereClause_Painless() {

0 commit comments

Comments
 (0)