Skip to content

Commit

Permalink
SQL: Fix issue with complex HAVING and GROUP BY ordinal (#36594)
Browse files Browse the repository at this point in the history 
When trying to analyse a HAVING condition, we crate a temp Aggregate
Plan which wasn't created correctly (missing the expressions from the
SELECT clause) and as a result the ordinal (1, 2, etc) in the GROUP BY
couldn't be resolved correctly.

Also after successfully analysing the HAVING condition, still the
original plan was returned.

Fixes: #36059
  • Loading branch information
@matriv
matriv authored Dec 17, 2018
1 parent f7567b8 commit c086639
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 27 deletions.
6 changes: 3 additions & 3 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {

@SuppressWarnings("unchecked")
Map<String, Object> aggregations2 = (Map<String, Object>) groupby.get("aggregations");
assertEquals(3, aggregations2.size());
assertEquals(2, aggregations2.size());

List<Integer> aggKeys = new ArrayList<>(2);
String aggFilterKey = null;
Expand All @@ -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<String, Object> having = (Map<String, Object>) aggregations2.get(aggFilterKey);
Expand All @@ -505,7 +505,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
@SuppressWarnings("unchecked")
Map<String, Object> bucketsPath = (Map<String, Object>) 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<String, Object> filterScript = (Map<String, Object>) bucketSelector.get("script");
Expand Down
11 changes: 7 additions & 4 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -935,14 +935,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;
Expand All @@ -958,6 +959,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;
}
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ protected ExecutionInfo executeWithInfo(TreeType plan) {
batchRuns++;

for (Rule<?, TreeType> 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;
Expand Down Expand Up @@ -192,4 +195,4 @@ protected ExecutionInfo executeWithInfo(TreeType plan) {

return new ExecutionInfo(plan, currentPlan, transformations);
}
}
}
2 changes: 1 addition & 1 deletion ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,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"));
}

Expand Down
45 changes: 27 additions & 18 deletions ...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,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);
Expand All @@ -266,9 +265,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);
Expand Down Expand Up @@ -335,9 +333,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);
Expand All @@ -350,9 +347,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);
Expand All @@ -366,9 +362,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);
Expand All @@ -385,9 +380,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);
Expand All @@ -399,6 +393,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);
Expand Down

0 comments on commit c086639

Please sign in to comment.