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 062ab0c81b99c..3ca8878ad5ee1 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 @@ -471,7 +471,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException { @SuppressWarnings("unchecked") Map aggregations2 = (Map) groupby.get("aggregations"); - assertEquals(3, aggregations2.size()); + assertEquals(2, aggregations2.size()); List aggKeys = new ArrayList<>(2); String aggFilterKey = null; @@ -491,7 +491,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException { } } Collections.sort(aggKeys); - assertEquals("having." + aggKeys.get(1), aggFilterKey); + assertEquals("having." + aggKeys.get(0), aggFilterKey); @SuppressWarnings("unchecked") Map having = (Map) aggregations2.get(aggFilterKey); @@ -505,7 +505,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException { @SuppressWarnings("unchecked") Map bucketsPath = (Map) bucketSelector.get("buckets_path"); assertEquals(1, bucketsPath.size()); - assertEquals(aggKeys.get(1).toString(), bucketsPath.get("a0")); + assertEquals(aggKeys.get(0).toString(), bucketsPath.get("a0")); @SuppressWarnings("unchecked") Map filterScript = (Map) bucketSelector.get("script"); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 3d407bef2862c..4a4e13a8c24ae 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -931,14 +931,15 @@ protected LogicalPlan rule(LogicalPlan plan) { if (!condition.resolved()) { // that's why try to resolve the condition Aggregate tryResolvingCondition = new Aggregate(agg.location(), agg.child(), agg.groupings(), - singletonList(new Alias(f.location(), ".having", condition))); + combine(agg.aggregates(), new Alias(f.location(), ".having", condition))); - LogicalPlan conditionResolved = analyze(tryResolvingCondition, false); + tryResolvingCondition = (Aggregate) analyze(tryResolvingCondition, false); // if it got resolved - if (conditionResolved.resolved()) { + if (tryResolvingCondition.resolved()) { // replace the condition with the resolved one - condition = ((Alias) ((Aggregate) conditionResolved).aggregates().get(0)).child(); + condition = ((Alias) tryResolvingCondition.aggregates() + .get(tryResolvingCondition.aggregates().size() - 1)).child(); } else { // else bail out return plan; @@ -954,6 +955,8 @@ protected LogicalPlan rule(LogicalPlan plan) { // preserve old output return new Project(f.location(), newFilter, f.output()); } + + return new Filter(f.location(), f.child(), condition); } return plan; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java index de4b6dac47879..2ed68def135ec 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java @@ -152,6 +152,9 @@ protected ExecutionInfo executeWithInfo(TreeType plan) { batchRuns++; for (Rule rule : batch.rules) { + if (log.isTraceEnabled()) { + log.trace("About to apply rule {}", rule); + } Transformation tf = new Transformation(currentPlan, rule); tfs.add(tf); currentPlan = tf.after; @@ -192,4 +195,4 @@ protected ExecutionInfo executeWithInfo(TreeType plan) { return new ExecutionInfo(plan, currentPlan, transformations); } -} \ No newline at end of file +} 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 46e19bd349cb1..e790705d4b25f 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 @@ -209,7 +209,7 @@ public void testGroupByOrderByScalarOverNonGrouped_WithHaving() { } public void testGroupByHavingNonGrouped() { - assertEquals("1:48: Cannot filter by non-grouped column [int], expected [text]", + assertEquals("1:48: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead", error("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10")); } 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 7461078fa9067..f595e9d2e892b 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 @@ -249,9 +249,8 @@ public void testTranslateIsNotNullExpression_WhereClause_Painless() { public void testTranslateIsNullExpression_HavingClause_Painless() { LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NULL"); - assertTrue(p instanceof Project); - assertTrue(p.children().get(0) instanceof Filter); - Expression condition = ((Filter) p.children().get(0)).condition(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -263,9 +262,8 @@ public void testTranslateIsNullExpression_HavingClause_Painless() { public void testTranslateIsNotNullExpression_HavingClause_Painless() { LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NOT NULL"); - assertTrue(p instanceof Project); - assertTrue(p.children().get(0) instanceof Filter); - Expression condition = ((Filter) p.children().get(0)).condition(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -332,9 +330,8 @@ public void testTranslateInExpression_WhereClause_Painless() { 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(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -347,9 +344,8 @@ public void testTranslateInExpression_HavingClause_Painless() { 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(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -363,9 +359,8 @@ public void testTranslateInExpression_HavingClause_PainlessOneArg() { 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(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -382,9 +377,8 @@ public void testTranslateMathFunction_HavingClause_Painless() { LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING " + operation.name() + "(max(int)) > 10"); - assertTrue(p instanceof Project); - assertTrue(p.children().get(0) instanceof Filter); - Expression condition = ((Filter) p.children().get(0)).condition(); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); assertFalse(condition.foldable()); QueryTranslation translation = QueryTranslator.toQuery(condition, true); assertNull(translation.query); @@ -396,6 +390,21 @@ public void testTranslateMathFunction_HavingClause_Painless() { assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]")); } + public void testGroupByAndHavingWithFunctionOnTopOfAggregation() { + LogicalPlan p = plan("SELECT keyword, MAX(int) FROM test GROUP BY 1 HAVING ABS(MAX(int)) > 10"); + assertTrue(p instanceof Filter); + Expression condition = ((Filter) p).condition(); + assertFalse(condition.foldable()); + QueryTranslation translation = QueryTranslator.toQuery(condition, true); + assertNull(translation.query); + AggFilter aggFilter = translation.aggFilter; + assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.gt(InternalSqlScriptUtils.abs" + + "(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 testTranslateCoalesce_GroupBy_Painless() { LogicalPlan p = plan("SELECT COALESCE(int, 10) FROM test GROUP BY 1"); assertTrue(p instanceof Aggregate);