From 782b6fec5d0ffae5acc566209606db4b977f44fe Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 29 Oct 2018 17:09:15 +0100 Subject: [PATCH 1/7] SQL: "Polish" painless script generated from `IN` Replace standard `||` and `==` painless operators with `or` and `eq` null safe alternatives from `InternalSqlScriptUtils`. Follow up to #34750 --- .../whitelist/InternalSqlScriptUtils.java | 8 ++- .../sql/expression/gen/script/Scripts.java | 2 +- .../xpack/sql/expression/predicate/In.java | 54 +++++++------------ .../xpack/sql/optimizer/Optimizer.java | 3 +- .../xpack/sql/querydsl/query/TermsQuery.java | 4 +- .../xpack/sql/type/DataTypeConversion.java | 3 ++ .../xpack/sql/plugin/sql_whitelist.txt | 3 +- .../xpack/sql/optimizer/OptimizerTests.java | 2 +- .../sql/planner/QueryTranslatorTests.java | 32 ++++++++--- .../sql/type/DataTypeConversionTests.java | 6 +++ 10 files changed, 69 insertions(+), 48 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java index 9aabb3f10ecdc..a4ddd5f74ae8b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java @@ -21,6 +21,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.string.ReplaceFunctionProcessor; import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation; import org.elasticsearch.xpack.sql.expression.function.scalar.string.SubstringFunctionProcessor; +import org.elasticsearch.xpack.sql.expression.predicate.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNullProcessor; import org.elasticsearch.xpack.sql.expression.predicate.logical.BinaryLogicProcessor.BinaryLogicOperation; import org.elasticsearch.xpack.sql.expression.predicate.logical.NotProcessor; @@ -31,6 +32,7 @@ import org.elasticsearch.xpack.sql.util.StringUtils; import java.time.ZonedDateTime; +import java.util.List; import java.util.Map; /** @@ -113,6 +115,10 @@ public static Boolean notNull(Object expression) { return IsNotNullProcessor.apply(expression); } + public static Boolean in(Object value, List values) { + return In.doFold(value, values); + } + // // Regex // @@ -375,4 +381,4 @@ public static String substring(String s, Number start, Number length) { public static String ucase(String s) { return (String) StringOperation.UCASE.apply(s); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Scripts.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Scripts.java index f9e2588a9c035..21ac12e51da89 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Scripts.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/script/Scripts.java @@ -87,4 +87,4 @@ public static ScriptTemplate binaryMethod(String methodName, ScriptTemplate left .build(), dataType); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java index 9b16b77511ca7..1f3d666ee6157 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java @@ -8,11 +8,10 @@ import org.elasticsearch.xpack.sql.expression.Attribute; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.expression.NamedExpression; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunctionAttribute; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; -import org.elasticsearch.xpack.sql.expression.gen.script.Params; -import org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver; import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Comparisons; @@ -30,7 +29,6 @@ import java.util.StringJoiner; import java.util.stream.Collectors; -import static java.lang.String.format; import static org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder.paramsBuilder; public class In extends NamedExpression implements ScriptWeaver { @@ -84,17 +82,21 @@ public boolean foldable() { @Override public Boolean fold() { + // Optimization for early return and Query folding to LocalExec if (value.dataType() == DataType.NULL) { return null; } if (list.size() == 1 && list.get(0).dataType() == DataType.NULL) { - return false; + return null; } - Object foldedLeftValue = value.fold(); + return doFold(value.fold(), Foldables.valuesOf(list, value.dataType())); + } + + public static Boolean doFold(Object value, List values) { Boolean result = false; - for (Expression rightValue : list) { - Boolean compResult = Comparisons.eq(foldedLeftValue, rightValue.fold()); + for (Object v : values) { + Boolean compResult = Comparisons.eq(value, v); if (compResult == null) { result = null; } else if (compResult) { @@ -122,34 +124,18 @@ public Attribute toAttribute() { @Override public ScriptTemplate asScript() { - StringJoiner sj = new StringJoiner(" || "); ScriptTemplate leftScript = asScript(value); - List rightParams = new ArrayList<>(); - String scriptPrefix = leftScript + "=="; - LinkedHashSet values = list.stream().map(Expression::fold).collect(Collectors.toCollection(LinkedHashSet::new)); - for (Object valueFromList : values) { - // if checked against null => false - if (valueFromList != null) { - if (valueFromList instanceof Expression) { - ScriptTemplate rightScript = asScript((Expression) valueFromList); - sj.add(scriptPrefix + rightScript.template()); - rightParams.add(rightScript.params()); - } else { - if (valueFromList instanceof String) { - sj.add(scriptPrefix + '"' + valueFromList + '"'); - } else { - sj.add(scriptPrefix + valueFromList.toString()); - } - } - } - } - - ParamsBuilder paramsBuilder = paramsBuilder().script(leftScript.params()); - for (Params p : rightParams) { - paramsBuilder = paramsBuilder.script(p); - } - - return new ScriptTemplate(format(Locale.ROOT, "%s", sj.toString()), paramsBuilder.build(), dataType()); + // remove duplicates + // TODO: Don't exclude nulls, painless script should handle them + List values = new ArrayList<>( + list.stream().map(Expression::fold).filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new))); + + return new ScriptTemplate(String.format(Locale.ROOT, formatTemplate("{sql}.in(%s, {})"), leftScript.template(), "%s"), + paramsBuilder() + .script(leftScript.params()) + .variable(values) + .build(), + dataType()); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index 8443358a12cb2..2d18a687a301e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -682,8 +682,7 @@ protected LogicalPlan rule(Filter filter) { if (TRUE.equals(filter.condition())) { return filter.child(); } - // TODO: add comparison with null as well - if (FALSE.equals(filter.condition())) { + if (FALSE.equals(filter.condition()) || ((Literal) filter.condition()).value() == null) { return new LocalRelation(filter.location(), new EmptyExecutable(filter.output())); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java index 91ea49a8a3ce3..66d206f829a32 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java @@ -9,7 +9,7 @@ import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.tree.Location; -import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.DataTypes; import java.util.Collections; import java.util.LinkedHashSet; @@ -27,7 +27,7 @@ public class TermsQuery extends LeafQuery { public TermsQuery(Location location, String term, List values) { super(location); this.term = term; - values.removeIf(e -> e.dataType() == DataType.NULL); + values.removeIf(e -> DataTypes.isNull(e.dataType())); if (values.isEmpty()) { this.values = Collections.emptySet(); } else { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java index 3312f449ec622..53f7e6b1ab16d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypeConversion.java @@ -105,6 +105,9 @@ public static Conversion conversionFor(DataType from, DataType to) { if (to == DataType.NULL) { return Conversion.NULL; } + if (from == DataType.NULL) { + return Conversion.NULL; + } Conversion conversion = conversion(from, to); if (conversion == null) { diff --git a/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt b/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt index 998dab84783f0..827947424b08e 100644 --- a/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt +++ b/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt @@ -24,6 +24,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS Boolean lte(Object, Object) Boolean gt(Object, Object) Boolean gte(Object, Object) + Boolean in(Object, java.util.List) # # Logical @@ -107,4 +108,4 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS String space(Number) String substring(String, Number, Number) String ucase(String) -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 0246499f7f9c0..62cb60ac98fa6 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -340,7 +340,7 @@ public void testConstantFoldingIn_LeftValueNotFoldable() { public void testConstantFoldingIn_RightValueIsNull() { In in = new In(EMPTY, getFieldAttribute(), Arrays.asList(NULL, NULL)); Literal result= (Literal) new ConstantFolding().rule(in); - assertEquals(false, result.value()); + assertNull(result.value()); } public void testConstantFoldingIn_LeftValueIsNull() { 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 c1e5a0d2dafad..58d16af81fbeb 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 @@ -33,6 +33,7 @@ import java.util.Map; import java.util.TimeZone; +import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.core.StringStartsWith.startsWith; public class QueryTranslatorTests extends AbstractBuilderTestCase { @@ -161,7 +162,7 @@ public void testLikeConstructsNotSupported() { } public void testTranslateInExpression_WhereClause() throws IOException { - LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', 'bar', 'lala', 'foo', concat('la', 'la'))"); + LogicalPlan p = plan("SELECT * FROM test WHERE keyword IN ('foo', 'bar', 'l''ala', 'foo', concat('l''a', 'la'))"); assertTrue(p instanceof Project); assertTrue(p.children().get(0) instanceof Filter); Expression condition = ((Filter) p.children().get(0)).condition(); @@ -170,7 +171,7 @@ public void testTranslateInExpression_WhereClause() throws IOException { Query query = translation.query; assertTrue(query instanceof TermsQuery); TermsQuery tq = (TermsQuery) query; - assertEquals("keyword:(bar foo lala)", tq.asBuilder().toQuery(createShardContext()).toString()); + assertEquals("keyword:(bar foo l'ala)", tq.asBuilder().toQuery(createShardContext()).toString()); } public void testTranslateInExpression_WhereClauseAndNullHandling() throws IOException { @@ -206,12 +207,14 @@ public void testTranslateInExpression_HavingClause_Painless() { QueryTranslation translation = QueryTranslator.toQuery(condition, false); assertTrue(translation.query instanceof ScriptQuery); ScriptQuery sq = (ScriptQuery) translation.query; - assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20)", sq.script().toString()); + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", + sq.script().toString()); assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(sq.script().params().toString(), endsWith(", {v=[10, 20]}]")); } - public void testTranslateInExpression_HavingClauseAndNullHandling_Painless() { - LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, null, 20, null, 30 - 10)"); + public void testTranslateInExpression_HavingClause_PainlessOneArg() { + LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, 30 - 20)"); assertTrue(p instanceof Project); assertTrue(p.children().get(0) instanceof Filter); Expression condition = ((Filter) p.children().get(0)).condition(); @@ -219,7 +222,24 @@ public void testTranslateInExpression_HavingClauseAndNullHandling_Painless() { QueryTranslation translation = QueryTranslator.toQuery(condition, false); assertTrue(translation.query instanceof ScriptQuery); ScriptQuery sq = (ScriptQuery) translation.query; - assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20)", sq.script().toString()); + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", sq.script().toString()); assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(sq.script().params().toString(), endsWith(", {v=[10]}]")); + + } + + public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() { + LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, null, 20, 30, null, 30 - 10)"); + assertTrue(p instanceof Project); + assertTrue(p.children().get(0) instanceof Filter); + Expression condition = ((Filter) p.children().get(0)).condition(); + assertFalse(condition.foldable()); + QueryTranslation translation = QueryTranslator.toQuery(condition, false); + assertTrue(translation.query instanceof ScriptQuery); + ScriptQuery sq = (ScriptQuery) translation.query; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", + sq.script().toString()); + assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(sq.script().params().toString(), endsWith(", {v=[10, 20, 30]}]")); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java index b191646a9cdee..7a04139430e33 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypeConversionTests.java @@ -224,6 +224,12 @@ public void testConversionToNull() { assertNull(conversion.convert(10.0)); } + public void testConversionFromNull() { + Conversion conversion = DataTypeConversion.conversionFor(DataType.NULL, DataType.INTEGER); + assertNull(conversion.convert(null)); + assertNull(conversion.convert(10)); + } + public void testConversionToIdentity() { Conversion conversion = DataTypeConversion.conversionFor(DataType.INTEGER, DataType.INTEGER); assertNull(conversion.convert(null)); From a0cd7237da546f607f0d6764276555fc72af4ff6 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 30 Oct 2018 16:08:52 +0100 Subject: [PATCH 2/7] move In to the same package as InPipe & InProcessor --- .../elasticsearch/xpack/sql/analysis/analyzer/Verifier.java | 2 +- .../function/scalar/whitelist/InternalSqlScriptUtils.java | 2 +- .../expression/predicate/{ => operator/comparison}/In.java | 4 +--- .../org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java | 2 +- .../java/org/elasticsearch/xpack/sql/planner/QueryFolder.java | 2 +- .../org/elasticsearch/xpack/sql/planner/QueryTranslator.java | 2 +- .../predicate/{ => operator/comparison}/InProcessorTests.java | 3 +-- .../predicate/{ => operator/comparison}/InTests.java | 2 +- .../org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java | 2 +- .../org/elasticsearch/xpack/sql/tree/NodeSubclassTests.java | 2 +- 10 files changed, 10 insertions(+), 13 deletions(-) rename x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/{ => operator/comparison}/In.java (95%) rename x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/{ => operator/comparison}/InProcessorTests.java (94%) rename x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/{ => operator/comparison}/InTests.java (95%) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index c8834240c6ceb..5394625d88272 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -19,7 +19,7 @@ import org.elasticsearch.xpack.sql.expression.function.Functions; import org.elasticsearch.xpack.sql.expression.function.Score; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.plan.logical.Aggregate; import org.elasticsearch.xpack.sql.plan.logical.Distinct; import org.elasticsearch.xpack.sql.plan.logical.Filter; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java index a4ddd5f74ae8b..152df2399a56b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java @@ -21,7 +21,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.string.ReplaceFunctionProcessor; import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation; import org.elasticsearch.xpack.sql.expression.function.scalar.string.SubstringFunctionProcessor; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNullProcessor; import org.elasticsearch.xpack.sql.expression.predicate.logical.BinaryLogicProcessor.BinaryLogicOperation; import org.elasticsearch.xpack.sql.expression.predicate.logical.NotProcessor; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java similarity index 95% rename from x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java rename to x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java index 1f3d666ee6157..3af5d1bd81c93 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -package org.elasticsearch.xpack.sql.expression.predicate; +package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison; import org.elasticsearch.xpack.sql.expression.Attribute; import org.elasticsearch.xpack.sql.expression.Expression; @@ -14,8 +14,6 @@ import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptWeaver; -import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.Comparisons; -import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.InPipe; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java index 84bb213fb6805..893f71c8bcb1b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java @@ -24,7 +24,7 @@ import org.elasticsearch.xpack.sql.expression.function.Function; import org.elasticsearch.xpack.sql.expression.function.UnresolvedFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.Cast; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNull; import org.elasticsearch.xpack.sql.expression.predicate.Range; import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 3605898210fc5..c17c1311cccc6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -30,7 +30,7 @@ import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.expression.gen.pipeline.UnaryPipe; import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.plan.physical.AggregateExec; import org.elasticsearch.xpack.sql.plan.physical.EsQueryExec; import org.elasticsearch.xpack.sql.plan.physical.FilterExec; 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 9fcd542ef631d..4e0bdea88b80b 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 @@ -31,7 +31,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeHistogramFunction; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNull; import org.elasticsearch.xpack.sql.expression.predicate.Range; import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InProcessorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessorTests.java similarity index 94% rename from x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InProcessorTests.java rename to x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessorTests.java index 59c9df1f89996..3303072e50078 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InProcessorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessorTests.java @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -package org.elasticsearch.xpack.sql.expression.predicate; +package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable.Reader; @@ -11,7 +11,6 @@ import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.function.scalar.Processors; import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor; -import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.InProcessor; import java.util.Arrays; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InTests.java similarity index 95% rename from x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InTests.java rename to x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InTests.java index 984fb833feb08..c78014afbc471 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/InTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InTests.java @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -package org.elasticsearch.xpack.sql.expression.predicate; +package org.elasticsearch.xpack.sql.expression.predicate.operator.comparison; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.expression.Literal; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 62cb60ac98fa6..9b0d688a16261 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -31,7 +31,7 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.math.E; import org.elasticsearch.xpack.sql.expression.function.scalar.math.Floor; import org.elasticsearch.xpack.sql.expression.predicate.BinaryOperator; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNull; import org.elasticsearch.xpack.sql.expression.predicate.Range; import org.elasticsearch.xpack.sql.expression.predicate.logical.And; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/tree/NodeSubclassTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/tree/NodeSubclassTests.java index caacee0f4bada..fe6fb7ee83f6a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/tree/NodeSubclassTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/tree/NodeSubclassTests.java @@ -24,7 +24,7 @@ import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor; import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; -import org.elasticsearch.xpack.sql.expression.predicate.In; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.fulltext.FullTextPredicate; import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.InPipe; import org.elasticsearch.xpack.sql.expression.predicate.regex.LikePattern; From 6bd1dc57ce3a191c964a753ed71b1769f40b508c Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 30 Oct 2018 18:07:17 +0100 Subject: [PATCH 3/7] Call doFold() from InProcessor to have the logic in one place. --- .../sql/expression/function/scalar/Processors.java | 10 +++++++++- .../predicate/operator/comparison/InProcessor.java | 13 ++----------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Processors.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Processors.java index ae35f9c760c43..1d038aeedd432 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Processors.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Processors.java @@ -73,4 +73,12 @@ public static List getNamedWriteables() { entries.add(new Entry(Processor.class, SubstringFunctionProcessor.NAME, SubstringFunctionProcessor::new)); return entries; } -} \ No newline at end of file + + public static List process(List processors, Object input) { + List values = new ArrayList<>(processors.size()); + for (Processor p : processors) { + values.add(p.process(input)); + } + return values; + } +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java index 0a901b5b5e6fe..9e8ded603ed09 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java @@ -7,6 +7,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.xpack.sql.expression.function.scalar.Processors; import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; import java.io.IOException; @@ -40,17 +41,7 @@ public final void writeTo(StreamOutput out) throws IOException { @Override public Object process(Object input) { Object leftValue = processsors.get(processsors.size() - 1).process(input); - Boolean result = false; - - for (int i = 0; i < processsors.size() - 1; i++) { - Boolean compResult = Comparisons.eq(leftValue, processsors.get(i).process(input)); - if (compResult == null) { - result = null; - } else if (compResult) { - return true; - } - } - return result; + return In.doFold(leftValue, Processors.process(processsors.subList(0, processsors.size() - 1), input)); } @Override From 8d8e0cb9139b5b30d063bddc467a776cd3304fe2 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 30 Oct 2018 22:18:02 +0100 Subject: [PATCH 4/7] Handle nulls in painless script --- .../predicate/operator/comparison/In.java | 3 +- .../xpack/sql/querydsl/agg/AggFilter.java | 4 +- .../sql/planner/QueryTranslatorTests.java | 58 ++++++++++++------- .../xpack/qa/sql/rest/RestSqlTestCase.java | 3 +- 4 files changed, 44 insertions(+), 24 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 3af5d1bd81c93..b0216bcf98e82 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 @@ -124,9 +124,8 @@ public Attribute toAttribute() { public ScriptTemplate asScript() { ScriptTemplate leftScript = asScript(value); // remove duplicates - // TODO: Don't exclude nulls, painless script should handle them List values = new ArrayList<>( - list.stream().map(Expression::fold).filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new))); + list.stream().map(Expression::fold).collect(Collectors.toCollection(LinkedHashSet::new))); return new ScriptTemplate(String.format(Locale.ROOT, formatTemplate("{sql}.in(%s, {})"), leftScript.template(), "%s"), paramsBuilder() diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java index 14b51a942ad4f..47ab30c976941 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/AggFilter.java @@ -8,6 +8,7 @@ import org.elasticsearch.script.Script; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; +import org.elasticsearch.xpack.sql.expression.gen.script.Scripts; import org.elasticsearch.xpack.sql.util.Check; import java.util.Collection; @@ -26,7 +27,8 @@ public class AggFilter extends PipelineAgg { public AggFilter(String name, ScriptTemplate scriptTemplate) { super(BUCKET_SELECTOR_ID_PREFIX + name); Check.isTrue(scriptTemplate != null, "a valid script is required"); - this.scriptTemplate = scriptTemplate; + // make script null safe + this.scriptTemplate = Scripts.nullSafeFilter(scriptTemplate); this.aggPaths = scriptTemplate.aggPaths(); } 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 58d16af81fbeb..db04605edae11 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 @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.plan.logical.Project; import org.elasticsearch.xpack.sql.planner.QueryTranslator.QueryTranslation; +import org.elasticsearch.xpack.sql.querydsl.agg.AggFilter; import org.elasticsearch.xpack.sql.querydsl.query.Query; import org.elasticsearch.xpack.sql.querydsl.query.RangeQuery; import org.elasticsearch.xpack.sql.querydsl.query.ScriptQuery; @@ -198,48 +199,65 @@ public void testTranslateInExpressionInvalidValues_WhereClause() { "offender [keyword] in [keyword IN(foo, bar, keyword)]", ex.getMessage()); } - public void testTranslateInExpression_HavingClause_Painless() { - LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, 20, 30 - 10)"); + public void testTranslateInExpression_WhereClause_Painless() { + LogicalPlan p = plan("SELECT int FROM test WHERE POWER(int, 2) IN (10, null, 20, 30 - 10)"); assertTrue(p instanceof Project); assertTrue(p.children().get(0) instanceof Filter); Expression condition = ((Filter) p.children().get(0)).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, false); + assertNull(translation.aggFilter); assertTrue(translation.query instanceof ScriptQuery); - ScriptQuery sq = (ScriptQuery) translation.query; + ScriptQuery sc = (ScriptQuery) translation.query; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(" + + "InternalSqlScriptUtils.power(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1), params.v2))", + sc.script().toString()); + assertEquals("[{v=int}, {v=2}, {v=[10, null, 20]}]", sc.script().params().toString()); + } + + public void testTranslateInExpression_HavingClause_Painless() { + LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 20, 30 - 10)"); + assertTrue(p instanceof Project); + assertTrue(p.children().get(0) instanceof Filter); + Expression condition = ((Filter) p.children().get(0)).condition(); + assertFalse(condition.foldable()); + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", - sq.script().toString()); - assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); - assertThat(sq.script().params().toString(), endsWith(", {v=[10, 20]}]")); + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=[10, 20]}]")); } public void testTranslateInExpression_HavingClause_PainlessOneArg() { - LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, 30 - 20)"); + LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 30 - 20)"); assertTrue(p instanceof Project); assertTrue(p.children().get(0) instanceof Filter); Expression condition = ((Filter) p.children().get(0)).condition(); assertFalse(condition.foldable()); - QueryTranslation translation = QueryTranslator.toQuery(condition, false); - assertTrue(translation.query instanceof ScriptQuery); - ScriptQuery sq = (ScriptQuery) translation.query; - assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", sq.script().toString()); - assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); - assertThat(sq.script().params().toString(), endsWith(", {v=[10]}]")); + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=[10]}]")); } public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() { - LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) in (10, null, 20, 30, null, 30 - 10)"); + LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, null, 20, 30, null, 30 - 10)"); assertTrue(p instanceof Project); assertTrue(p.children().get(0) instanceof Filter); Expression condition = ((Filter) p.children().get(0)).condition(); assertFalse(condition.foldable()); - QueryTranslation translation = QueryTranslator.toQuery(condition, false); - assertTrue(translation.query instanceof ScriptQuery); - ScriptQuery sq = (ScriptQuery) translation.query; + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", - sq.script().toString()); - assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); - assertThat(sq.script().params().toString(), endsWith(", {v=[10, 20, 30]}]")); + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); + assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=[10, null, 20, 30]}]")); } } diff --git a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 4df82119e36d0..aa3dd72e5e65e 100644 --- a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java @@ -515,7 +515,8 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException { @SuppressWarnings("unchecked") Map filterScript = (Map) bucketSelector.get("script"); assertEquals(3, filterScript.size()); - assertEquals("InternalSqlScriptUtils.gt(params.a0,params.v0)", filterScript.get("source")); + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.gt(params.a0,params.v0))", + filterScript.get("source")); assertEquals("painless", filterScript.get("lang")); @SuppressWarnings("unchecked") Map filterScriptParams = (Map) filterScript.get("params"); From 9f0b212c57b1b349a6eb1d8222c990c024787a95 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 31 Oct 2018 11:22:13 +0100 Subject: [PATCH 5/7] Fix string formatting for script template --- .../xpack/sql/expression/predicate/operator/comparison/In.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 b0216bcf98e82..2bcac4c7800a8 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 @@ -127,7 +127,8 @@ public ScriptTemplate asScript() { List values = new ArrayList<>( list.stream().map(Expression::fold).collect(Collectors.toCollection(LinkedHashSet::new))); - return new ScriptTemplate(String.format(Locale.ROOT, formatTemplate("{sql}.in(%s, {})"), leftScript.template(), "%s"), + return new ScriptTemplate( + formatTemplate(String.format(Locale.ROOT, "{sql}.in(%s, {})", leftScript.template())), paramsBuilder() .script(leftScript.params()) .variable(values) From fb97b51c60560705bdbdb6f9c9d716c260fe9b5e Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 31 Oct 2018 23:41:33 +0100 Subject: [PATCH 6/7] Address comments --- .../whitelist/InternalSqlScriptUtils.java | 4 +-- .../predicate/operator/comparison/In.java | 26 ++++--------------- .../operator/comparison/InProcessor.java | 19 +++++++++++--- .../sql/planner/QueryTranslatorTests.java | 4 +-- 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java index 152df2399a56b..1c2ccfeeb29cb 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java @@ -21,13 +21,13 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.string.ReplaceFunctionProcessor; import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation; import org.elasticsearch.xpack.sql.expression.function.scalar.string.SubstringFunctionProcessor; -import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.In; import org.elasticsearch.xpack.sql.expression.predicate.IsNotNullProcessor; import org.elasticsearch.xpack.sql.expression.predicate.logical.BinaryLogicProcessor.BinaryLogicOperation; import org.elasticsearch.xpack.sql.expression.predicate.logical.NotProcessor; import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation; import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.UnaryArithmeticProcessor.UnaryArithmeticOperation; import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation; +import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.InProcessor; import org.elasticsearch.xpack.sql.expression.predicate.regex.RegexProcessor.RegexOperation; import org.elasticsearch.xpack.sql.util.StringUtils; @@ -116,7 +116,7 @@ public static Boolean notNull(Object expression) { } public static Boolean in(Object value, List values) { - return In.doFold(value, values); + return InProcessor.apply(value, values); } // 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 2bcac4c7800a8..41cbeee984206 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 @@ -81,27 +81,11 @@ public boolean foldable() { @Override public Boolean fold() { // Optimization for early return and Query folding to LocalExec - if (value.dataType() == DataType.NULL) { + if (value.dataType() == DataType.NULL || + list.size() == 1 && list.get(0).dataType() == DataType.NULL) { return null; } - if (list.size() == 1 && list.get(0).dataType() == DataType.NULL) { - return null; - } - - return doFold(value.fold(), Foldables.valuesOf(list, value.dataType())); - } - - public static Boolean doFold(Object value, List values) { - Boolean result = false; - for (Object v : values) { - Boolean compResult = Comparisons.eq(value, v); - if (compResult == null) { - result = null; - } else if (compResult) { - return true; - } - } - return result; + return InProcessor.apply(value.fold(), Foldables.valuesOf(list, value.dataType())); } @Override @@ -124,8 +108,8 @@ public Attribute toAttribute() { public ScriptTemplate asScript() { ScriptTemplate leftScript = asScript(value); // remove duplicates - List values = new ArrayList<>( - list.stream().map(Expression::fold).collect(Collectors.toCollection(LinkedHashSet::new))); + List values = new ArrayList<>(new LinkedHashSet<>(Foldables.valuesOf(list, value.dataType()))); + values.removeIf(Objects::isNull); return new ScriptTemplate( formatTemplate(String.format(Locale.ROOT, "{sql}.in(%s, {})", leftScript.template())), diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java index 9e8ded603ed09..a843ebee5b385 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java @@ -20,11 +20,11 @@ public class InProcessor implements Processor { private final List processsors; - public InProcessor(List processors) { + InProcessor(List processors) { this.processsors = processors; } - public InProcessor(StreamInput in) throws IOException { + InProcessor(StreamInput in) throws IOException { processsors = in.readNamedWriteableList(Processor.class); } @@ -41,7 +41,20 @@ public final void writeTo(StreamOutput out) throws IOException { @Override public Object process(Object input) { Object leftValue = processsors.get(processsors.size() - 1).process(input); - return In.doFold(leftValue, Processors.process(processsors.subList(0, processsors.size() - 1), input)); + return apply(leftValue, Processors.process(processsors.subList(0, processsors.size() - 1), leftValue)); + } + + public static Boolean apply(Object input, List values) { + Boolean result = Boolean.FALSE; + for (Object v : values) { + Boolean compResult = Comparisons.eq(input, v); + if (compResult == null) { + result = null; + } else if (compResult == Boolean.TRUE) { + return Boolean.TRUE; + } + } + return result; } @Override 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 217678bf08b3a..fbe0ab2aa8118 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 @@ -213,7 +213,7 @@ public void testTranslateInExpression_WhereClause_Painless() { assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(" + "InternalSqlScriptUtils.power(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1), params.v2))", sc.script().toString()); - assertEquals("[{v=int}, {v=2}, {v=[10, null, 20]}]", sc.script().params().toString()); + assertEquals("[{v=int}, {v=2}, {v=[10.0, 20.0]}]", sc.script().params().toString()); } public void testTranslateInExpression_HavingClause_Painless() { @@ -259,6 +259,6 @@ public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() { assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.in(params.a0, params.v0))", aggFilter.scriptTemplate().toString()); assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); - assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=[10, null, 20, 30]}]")); + assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=[10, 20, 30]}]")); } } From 8beefbcd5b94677038502326b33efed7692b6968 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 31 Oct 2018 23:54:44 +0100 Subject: [PATCH 7/7] fix constructor --- .../expression/predicate/operator/comparison/InProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java index a843ebee5b385..82233e250e364 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InProcessor.java @@ -24,7 +24,7 @@ public class InProcessor implements Processor { this.processsors = processors; } - InProcessor(StreamInput in) throws IOException { + public InProcessor(StreamInput in) throws IOException { processsors = in.readNamedWriteableList(Processor.class); }