From 9671e943ec5106088cb92de164d6c069cd974d1d Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 5 Feb 2019 18:00:50 +0200 Subject: [PATCH 1/2] SQL: Fix issue with IN not resolving to underlying keyword field Add resolution to the exact keyword field (if exists) for text fields. Add proper verification and error message if underlying keyword doesn't exist. Move check for field attribute in the comparison list to the `resolveType()` method of `IN`. Fixes: #38424 --- .../predicate/operator/comparison/In.java | 23 ++++++++++++++++ .../xpack/sql/planner/QueryTranslator.java | 20 ++++++-------- .../analyzer/VerifierErrorMessagesTests.java | 26 +++++++++++++------ .../sql/planner/QueryTranslatorTests.java | 21 ++++++++------- 4 files changed, 60 insertions(+), 30 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java index f8f0bb35b504e..d0164c09f9fb9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java @@ -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; @@ -21,6 +23,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.stream.Collectors; import static org.elasticsearch.common.logging.LoggerMessageFormat.format; @@ -105,6 +108,26 @@ 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 ex) { + return new TypeResolution(format(null, "[{}] cannot operate on first argument field of data type [{}]", + functionName(), value().dataType().esType)); + } + } + + Optional firstNotFoldable = list.stream().filter(expression -> !expression.foldable()).findFirst(); + if (firstNotFoldable.isPresent()) { + return new TypeResolution(format(null, "Comparisons against variables are not (currently) supported; offender [{}] in [{}]", + Expressions.name(firstNotFoldable.get()), + name())); + } + return super.resolveType(); + } + @Override public int hashCode() { return Objects.hash(value, list); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java index de529b2e4ca61..ed8838043d5ed 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java @@ -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; @@ -708,16 +707,6 @@ static class InComparisons extends ExpressionTranslator { @Override protected QueryTranslation asQuery(In in, boolean onAggs) { - Optional 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(); @@ -735,7 +724,14 @@ 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(); + String name = fa.name(); + // equality should always be against an exact match + // (which is important for strings) + if (fa.isInexact()) { + name = fa.exactAttribute().name(); + } + q = new TermsQuery(in.source(), name, in.list()); } else { q = new ScriptQuery(in.source(), in.asScript()); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 992ab6e1905fe..083dcb9c3b4a5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -385,23 +385,33 @@ 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 first argument field of data type [text]", + error("SELECT * FROM test WHERE text IN ('foo', 'bar')")); } public void testNotSupportedAggregateOnDate() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 2cad625889474..34af1d2455981 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -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(); @@ -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() { From b0729dda6a18bd91d3d042ef5d680136d9905474 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 6 Feb 2019 12:24:04 +0200 Subject: [PATCH 2/2] Address comments --- .../predicate/operator/comparison/In.java | 18 +++++++++--------- .../xpack/sql/planner/QueryTranslator.java | 9 ++------- .../analyzer/VerifierErrorMessagesTests.java | 3 ++- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java index d0164c09f9fb9..f76523eaf0cd9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java @@ -23,7 +23,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; -import java.util.Optional; import java.util.stream.Collectors; import static org.elasticsearch.common.logging.LoggerMessageFormat.format; @@ -113,17 +112,18 @@ protected TypeResolution resolveType() { if (value instanceof FieldAttribute) { try { ((FieldAttribute) value).exactAttribute(); - } catch (MappingException ex) { - return new TypeResolution(format(null, "[{}] cannot operate on first argument field of data type [{}]", - functionName(), value().dataType().esType)); + } catch (MappingException e) { + return new TypeResolution(format(null, "[{}] cannot operate on field of data type [{}]: {}", + functionName(), value().dataType().esType, e.getMessage())); } } - Optional firstNotFoldable = list.stream().filter(expression -> !expression.foldable()).findFirst(); - if (firstNotFoldable.isPresent()) { - return new TypeResolution(format(null, "Comparisons against variables are not (currently) supported; offender [{}] in [{}]", - Expressions.name(firstNotFoldable.get()), - name())); + 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(); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java index ed8838043d5ed..73e9ff57f379e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java @@ -725,13 +725,8 @@ protected QueryTranslation asQuery(In in, boolean onAggs) { Query q = null; if (in.value() instanceof FieldAttribute) { FieldAttribute fa = (FieldAttribute) in.value(); - String name = fa.name(); - // equality should always be against an exact match - // (which is important for strings) - if (fa.isInexact()) { - name = fa.exactAttribute().name(); - } - q = new TermsQuery(in.source(), name, in.list()); + // 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()); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 083dcb9c3b4a5..78a0484ffe490 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -410,7 +410,8 @@ public void testInWithFieldInListOfValues() { } public void testInOnFieldTextWithNoKeyword() { - assertEquals("1:26: [IN] cannot operate on first argument field of data type [text]", + 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')")); }