Skip to content

Commit 2c3484a

Browse files
matrivmatriv
committed
SQL: Fix issue with complex HAVING and GROUP BY ordinal (#36594)
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
1 parent 19b7ab0 commit 2c3484a

File tree

5 files changed

+42
-27
lines changed

5 files changed

+42
-27
lines changed

‎x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
471471

472472
@SuppressWarnings("unchecked")
473473
Map<String, Object> aggregations2 = (Map<String, Object>) groupby.get("aggregations");
474-
assertEquals(3, aggregations2.size());
474+
assertEquals(2, aggregations2.size());
475475

476476
List<Integer> aggKeys = new ArrayList<>(2);
477477
String aggFilterKey = null;
@@ -491,7 +491,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
491491
}
492492
}
493493
Collections.sort(aggKeys);
494-
assertEquals("having." + aggKeys.get(1), aggFilterKey);
494+
assertEquals("having." + aggKeys.get(0), aggFilterKey);
495495

496496
@SuppressWarnings("unchecked")
497497
Map<String, Object> having = (Map<String, Object>) aggregations2.get(aggFilterKey);
@@ -505,7 +505,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
505505
@SuppressWarnings("unchecked")
506506
Map<String, Object> bucketsPath = (Map<String, Object>) bucketSelector.get("buckets_path");
507507
assertEquals(1, bucketsPath.size());
508-
assertEquals(aggKeys.get(1).toString(), bucketsPath.get("a0"));
508+
assertEquals(aggKeys.get(0).toString(), bucketsPath.get("a0"));
509509

510510
@SuppressWarnings("unchecked")
511511
Map<String, Object> filterScript = (Map<String, Object>) bucketSelector.get("script");

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -935,14 +935,15 @@ protected LogicalPlan rule(LogicalPlan plan) {
935935
if (!condition.resolved()) {
936936
// that's why try to resolve the condition
937937
Aggregate tryResolvingCondition = new Aggregate(agg.location(), agg.child(), agg.groupings(),
938-
singletonList(new Alias(f.location(), ".having", condition)));
938+
combine(agg.aggregates(), new Alias(f.location(), ".having", condition)));
939939

940-
LogicalPlan conditionResolved = analyze(tryResolvingCondition, false);
940+
tryResolvingCondition = (Aggregate) analyze(tryResolvingCondition, false);
941941

942942
// if it got resolved
943-
if (conditionResolved.resolved()) {
943+
if (tryResolvingCondition.resolved()) {
944944
// replace the condition with the resolved one
945-
condition = ((Alias) ((Aggregate) conditionResolved).aggregates().get(0)).child();
945+
condition = ((Alias) tryResolvingCondition.aggregates()
946+
.get(tryResolvingCondition.aggregates().size() - 1)).child();
946947
} else {
947948
// else bail out
948949
return plan;
@@ -958,6 +959,8 @@ protected LogicalPlan rule(LogicalPlan plan) {
958959
// preserve old output
959960
return new Project(f.location(), newFilter, f.output());
960961
}
962+
963+
return new Filter(f.location(), f.child(), condition);
961964
}
962965
return plan;
963966
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,9 @@ protected ExecutionInfo executeWithInfo(TreeType plan) {
152152
batchRuns++;
153153

154154
for (Rule<?, TreeType> rule : batch.rules) {
155+
if (log.isTraceEnabled()) {
156+
log.trace("About to apply rule {}", rule);
157+
}
155158
Transformation tf = new Transformation(currentPlan, rule);
156159
tfs.add(tf);
157160
currentPlan = tf.after;
@@ -192,4 +195,4 @@ protected ExecutionInfo executeWithInfo(TreeType plan) {
192195

193196
return new ExecutionInfo(plan, currentPlan, transformations);
194197
}
195-
}
198+
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ public void testGroupByOrderByScalarOverNonGrouped_WithHaving() {
227227
}
228228

229229
public void testGroupByHavingNonGrouped() {
230-
assertEquals("1:48: Cannot filter by non-grouped column [int], expected [text]",
230+
assertEquals("1:48: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead",
231231
error("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10"));
232232
}
233233

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java‎

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -252,9 +252,8 @@ public void testTranslateIsNotNullExpression_WhereClause_Painless() {
252252

253253
public void testTranslateIsNullExpression_HavingClause_Painless() {
254254
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NULL");
255-
assertTrue(p instanceof Project);
256-
assertTrue(p.children().get(0) instanceof Filter);
257-
Expression condition = ((Filter) p.children().get(0)).condition();
255+
assertTrue(p instanceof Filter);
256+
Expression condition = ((Filter) p).condition();
258257
assertFalse(condition.foldable());
259258
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
260259
assertNull(translation.query);
@@ -266,9 +265,8 @@ public void testTranslateIsNullExpression_HavingClause_Painless() {
266265

267266
public void testTranslateIsNotNullExpression_HavingClause_Painless() {
268267
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NOT NULL");
269-
assertTrue(p instanceof Project);
270-
assertTrue(p.children().get(0) instanceof Filter);
271-
Expression condition = ((Filter) p.children().get(0)).condition();
268+
assertTrue(p instanceof Filter);
269+
Expression condition = ((Filter) p).condition();
272270
assertFalse(condition.foldable());
273271
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
274272
assertNull(translation.query);
@@ -335,9 +333,8 @@ public void testTranslateInExpression_WhereClause_Painless() {
335333

336334
public void testTranslateInExpression_HavingClause_Painless() {
337335
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 20, 30 - 10)");
338-
assertTrue(p instanceof Project);
339-
assertTrue(p.children().get(0) instanceof Filter);
340-
Expression condition = ((Filter) p.children().get(0)).condition();
336+
assertTrue(p instanceof Filter);
337+
Expression condition = ((Filter) p).condition();
341338
assertFalse(condition.foldable());
342339
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
343340
assertNull(translation.query);
@@ -350,9 +347,8 @@ public void testTranslateInExpression_HavingClause_Painless() {
350347

351348
public void testTranslateInExpression_HavingClause_PainlessOneArg() {
352349
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 30 - 20)");
353-
assertTrue(p instanceof Project);
354-
assertTrue(p.children().get(0) instanceof Filter);
355-
Expression condition = ((Filter) p.children().get(0)).condition();
350+
assertTrue(p instanceof Filter);
351+
Expression condition = ((Filter) p).condition();
356352
assertFalse(condition.foldable());
357353
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
358354
assertNull(translation.query);
@@ -366,9 +362,8 @@ public void testTranslateInExpression_HavingClause_PainlessOneArg() {
366362

367363
public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() {
368364
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, null, 20, 30, null, 30 - 10)");
369-
assertTrue(p instanceof Project);
370-
assertTrue(p.children().get(0) instanceof Filter);
371-
Expression condition = ((Filter) p.children().get(0)).condition();
365+
assertTrue(p instanceof Filter);
366+
Expression condition = ((Filter) p).condition();
372367
assertFalse(condition.foldable());
373368
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
374369
assertNull(translation.query);
@@ -385,9 +380,8 @@ public void testTranslateMathFunction_HavingClause_Painless() {
385380

386381
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING " +
387382
operation.name() + "(max(int)) > 10");
388-
assertTrue(p instanceof Project);
389-
assertTrue(p.children().get(0) instanceof Filter);
390-
Expression condition = ((Filter) p.children().get(0)).condition();
383+
assertTrue(p instanceof Filter);
384+
Expression condition = ((Filter) p).condition();
391385
assertFalse(condition.foldable());
392386
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
393387
assertNull(translation.query);
@@ -399,6 +393,21 @@ public void testTranslateMathFunction_HavingClause_Painless() {
399393
assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]"));
400394
}
401395

396+
public void testGroupByAndHavingWithFunctionOnTopOfAggregation() {
397+
LogicalPlan p = plan("SELECT keyword, MAX(int) FROM test GROUP BY 1 HAVING ABS(MAX(int)) > 10");
398+
assertTrue(p instanceof Filter);
399+
Expression condition = ((Filter) p).condition();
400+
assertFalse(condition.foldable());
401+
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
402+
assertNull(translation.query);
403+
AggFilter aggFilter = translation.aggFilter;
404+
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.gt(InternalSqlScriptUtils.abs" +
405+
"(params.a0),params.v0))",
406+
aggFilter.scriptTemplate().toString());
407+
assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->"));
408+
assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]"));
409+
}
410+
402411
public void testTranslateCoalesce_GroupBy_Painless() {
403412
LogicalPlan p = plan("SELECT COALESCE(int, 10) FROM test GROUP BY 1");
404413
assertTrue(p instanceof Aggregate);

0 commit comments

Comments
 (0)