From 0edaf2b6c1c541d61587edb1fe6fae3b50a7126d Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 24 Jan 2019 18:40:20 +0200 Subject: [PATCH] SQL: Fix issue with complex expression as args of PERCENTILE/_RANK (#37102) When the arguements of PERCENTILE and PERCENTILE_RANK can be folded, the `ConstantFolding` rule kicks in and calls the `replaceChildren()` method on `InnerAggregate` which is created from the aggregation rules of the `Optimizerz. `InnerAggregate` in turn, cannot implement the method as the logic of creating a new `InnerAggregate` instance from a list of `Expression`s resides in the Optimizer. So, instead, `ConstantFolding` should be applied before any of the aggregations related rules. Fixes: #37099 --- .../sql/qa/src/main/resources/agg.csv-spec | 4 ++-- .../xpack/sql/optimizer/Optimizer.java | 2 +- .../xpack/sql/planner/QueryFolderTests.java | 21 +++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec index bdb94321b76d5..d1b1a0e0e8866 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -3,7 +3,7 @@ // singlePercentileWithoutComma -SELECT gender, PERCENTILE(emp_no, 97) p1 FROM test_emp GROUP BY gender; +SELECT gender, PERCENTILE(emp_no, 90 + 7) p1 FROM test_emp GROUP BY gender; gender:s | p1:d null |10019.0 @@ -48,7 +48,7 @@ M |10084.349 |10093.502 ; percentileRank -SELECT gender, PERCENTILE_RANK(emp_no, 10025) rank FROM test_emp GROUP BY gender; +SELECT gender, PERCENTILE_RANK(emp_no, 10000 + 25) rank FROM test_emp GROUP BY gender; gender:s | rank:d null |100.0 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 706fb4853626d..aeaca9ea4ddd6 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 @@ -159,7 +159,7 @@ protected Iterable.Batch> batches() { Batch label = new Batch("Set as Optimized", Limiter.ONCE, new SetAsOptimized()); - return Arrays.asList(aggregate, operators, local, label); + return Arrays.asList(operators, aggregate, local, label); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java index c20f4e9d632af..16416d2965e4a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.xpack.sql.analysis.index.EsIndex; import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; +import org.elasticsearch.xpack.sql.expression.function.aggregate.AggregateFunctionAttribute; import org.elasticsearch.xpack.sql.optimizer.Optimizer; import org.elasticsearch.xpack.sql.parser.SqlParser; import org.elasticsearch.xpack.sql.plan.physical.EsQueryExec; @@ -316,4 +317,24 @@ public void testConcatIsNotFoldedForNull() { assertEquals(1, ee.output().size()); assertThat(ee.output().get(0).toString(), startsWith("keyword{f}#")); } + + public void testFoldingOfPercentileSecondArgument() { + PhysicalPlan p = plan("SELECT PERCENTILE(int, 1 + 2) FROM test"); + assertEquals(EsQueryExec.class, p.getClass()); + EsQueryExec ee = (EsQueryExec) p; + assertEquals(1, ee.output().size()); + assertEquals(AggregateFunctionAttribute.class, ee.output().get(0).getClass()); + AggregateFunctionAttribute afa = (AggregateFunctionAttribute) ee.output().get(0); + assertThat(afa.propertyPath(), endsWith("[3.0]")); + } + + public void testFoldingOfPercentileRankSecondArgument() { + PhysicalPlan p = plan("SELECT PERCENTILE_RANK(int, 1 + 2) FROM test"); + assertEquals(EsQueryExec.class, p.getClass()); + EsQueryExec ee = (EsQueryExec) p; + assertEquals(1, ee.output().size()); + assertEquals(AggregateFunctionAttribute.class, ee.output().get(0).getClass()); + AggregateFunctionAttribute afa = (AggregateFunctionAttribute) ee.output().get(0); + assertThat(afa.propertyPath(), endsWith("[3.0]")); + } }