From bf6f521827d4784a22313aee0fc7ea5823ff03d5 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 31 Oct 2018 10:32:16 +0100 Subject: [PATCH] SQL: Fix NPE thrown if HAVING filter evals to null (#35108) Surround script with `nullSafeFilter` also for `AggFilter` translation. Fixed query translation tests to properly set `onAggs`. Fixes: #35107 --- .../xpack/sql/qa/rest/RestSqlTestCase.java | 3 +- .../xpack/sql/querydsl/agg/AggFilter.java | 4 +- .../sql/planner/QueryTranslatorTests.java | 57 +++++++++++++++---- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java index 7dcfc26ef5f..73fa3ef1d77 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java @@ -515,7 +515,8 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe @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"); 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 14b51a942ad..47ab30c9769 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 @@ package org.elasticsearch.xpack.sql.querydsl.agg; 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 b0a15f22226..05f9c136515 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.Filter; 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,29 +199,63 @@ public class QueryTranslatorTests extends ESTestCase { "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; - assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20)", sq.script().toString()); - assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); + assertEquals("InternalSqlScriptUtils.nullSafeFilter(" + + "InternalSqlScriptUtils.power(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1)==10 || " + + "InternalSqlScriptUtils.power(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1)==20)", + sq.script().toString()); + assertEquals("[{v=int}, {v=2}]", sq.script().params().toString()); } - 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_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, false); - assertTrue(translation.query instanceof ScriptQuery); - ScriptQuery sq = (ScriptQuery) translation.query; - assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20)", sq.script().toString()); - assertThat(sq.script().params().toString(), startsWith("[{a=MAX(int){a->")); + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20)", + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); + } + + 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(); + assertFalse(condition.foldable()); + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10)", + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); + + } + + 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, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(params.a0==10 || params.a0==20 || params.a0==30)", + aggFilter.scriptTemplate().toString()); + assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); } }