Skip to content

Commit

Permalink
SQL: Fix issue with IN not resolving to underlying keyword field (#38440
Browse files Browse the repository at this point in the history 
)

- 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
  • Loading branch information
@matriv
matriv committed Feb 6, 2019
1 parent 4773720 commit 5e798c1
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 30 deletions.
23 changes: 23 additions & 0 deletions ...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
*/
package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison;

import org.elasticsearch.xpack.sql.analysis.index.MappingException;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.Expressions;
import org.elasticsearch.xpack.sql.expression.FieldAttribute;
import org.elasticsearch.xpack.sql.expression.Foldables;
import org.elasticsearch.xpack.sql.expression.Nullability;
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
Expand Down Expand Up @@ -105,6 +107,27 @@ protected Pipe makePipe() {
return new InPipe(source(), this, children().stream().map(Expressions::pipe).collect(Collectors.toList()));
}

@Override
protected TypeResolution resolveType() {
if (value instanceof FieldAttribute) {
try {
((FieldAttribute) value).exactAttribute();
} catch (MappingException e) {
return new TypeResolution(format(null, "[{}] cannot operate on field of data type [{}]: {}",
functionName(), value().dataType().esType, e.getMessage()));
}
}

for (Expression ex : list) {
if (ex.foldable() == false) {
return new TypeResolution(format(null, "Comparisons against variables are not (currently) supported; offender [{}] in [{}]",
Expressions.name(ex),
name()));
}
}
return super.resolveType();
}

@Override
public int hashCode() {
return Objects.hash(value, list);
Expand Down
15 changes: 3 additions & 12 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.function.Supplier;

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

@Override
protected QueryTranslation asQuery(In in, boolean onAggs) {
Optional<Expression> firstNotFoldable = in.list().stream().filter(expression -> !expression.foldable()).findFirst();

if (firstNotFoldable.isPresent()) {
throw new SqlIllegalArgumentException(
"Line {}:{}: Comparisons against variables are not (currently) supported; offender [{}] in [{}]",
firstNotFoldable.get().sourceLocation().getLineNumber(),
firstNotFoldable.get().sourceLocation().getColumnNumber(),
Expressions.name(firstNotFoldable.get()),
in.name());
}

if (in.value() instanceof NamedExpression) {
NamedExpression ne = (NamedExpression) in.value();
Expand All @@ -735,7 +724,9 @@ protected QueryTranslation asQuery(In in, boolean onAggs) {
else {
Query q = null;
if (in.value() instanceof FieldAttribute) {
q = new TermsQuery(in.source(), ne.name(), in.list());
FieldAttribute fa = (FieldAttribute) in.value();
// equality should always be against an exact match (which is important for strings)
q = new TermsQuery(in.source(), fa.isInexact() ? fa.exactAttribute().name() : fa.name(), in.list());
} else {
q = new ScriptQuery(in.source(), in.asScript());
}
Expand Down
27 changes: 19 additions & 8 deletions ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,23 +385,34 @@ public void testInNestedWithDifferentDataTypesFromLeftValue_SelectClause() {
}

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

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

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

public void testInNestedWithDifferentDataTypesFromLeftValue_WhereClause() {
assertEquals("1:46: expected data type [text], value provided is of type [integer]",
error("SELECT * FROM test WHERE int = 1 OR text IN (1, 2)"));
assertEquals("1:49: expected data type [keyword], value provided is of type [integer]",
error("SELECT * FROM test WHERE int = 1 OR keyword IN (1, 2)"));
}

public void testInWithFieldInListOfValues() {
assertEquals("1:26: Comparisons against variables are not (currently) supported; offender [int] in [int IN (1, int)]",
error("SELECT * FROM test WHERE int IN (1, int)"));
}

public void testInOnFieldTextWithNoKeyword() {
assertEquals("1:26: [IN] cannot operate on field of data type [text]: " +
"No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead",
error("SELECT * FROM test WHERE text IN ('foo', 'bar')"));
}

public void testNotSupportedAggregateOnDate() {
Expand Down
21 changes: 11 additions & 10 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,8 @@ public void testTranslateInExpression_WhereClause() {
tq.asBuilder().toString().replaceAll("\\s", ""));
}

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

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

public void testTranslateInExpression_WhereClause_Painless() {
Expand Down

0 comments on commit 5e798c1

Please sign in to comment.