Skip to content

Commit dead7c6

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 405ff61 commit dead7c6

File tree

5 files changed

+41
-26
lines changed

5 files changed

+41
-26
lines changed

‎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
@@ -934,14 +934,15 @@ protected LogicalPlan rule(LogicalPlan plan) {
934934
if (!condition.resolved()) {
935935
// that's why try to resolve the condition
936936
Aggregate tryResolvingCondition = new Aggregate(agg.location(), agg.child(), agg.groupings(),
937-
singletonList(new Alias(f.location(), ".having", condition)));
937+
combine(agg.aggregates(), new Alias(f.location(), ".having", condition)));
938938

939-
LogicalPlan conditionResolved = analyze(tryResolvingCondition, false);
939+
tryResolvingCondition = (Aggregate) analyze(tryResolvingCondition, false);
940940

941941
// if it got resolved
942-
if (conditionResolved.resolved()) {
942+
if (tryResolvingCondition.resolved()) {
943943
// replace the condition with the resolved one
944-
condition = ((Alias) ((Aggregate) conditionResolved).aggregates().get(0)).child();
944+
condition = ((Alias) tryResolvingCondition.aggregates()
945+
.get(tryResolvingCondition.aggregates().size() - 1)).child();
945946
} else {
946947
// else bail out
947948
return plan;
@@ -957,6 +958,8 @@ protected LogicalPlan rule(LogicalPlan plan) {
957958
// preserve old output
958959
return new Project(f.location(), newFilter, f.output());
959960
}
961+
962+
return new Filter(f.location(), f.child(), condition);
960963
}
961964
return plan;
962965
}

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

Lines changed: 3 additions & 0 deletions
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;

‎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
@@ -183,7 +183,7 @@ public void testGroupByOrderByScalarOverNonGrouped_WithHaving() {
183183
}
184184

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

‎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
@@ -244,9 +244,8 @@ public void testTranslateIsNotNullExpression_WhereClause_Painless() {
244244

245245
public void testTranslateIsNullExpression_HavingClause_Painless() {
246246
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NULL");
247-
assertTrue(p instanceof Project);
248-
assertTrue(p.children().get(0) instanceof Filter);
249-
Expression condition = ((Filter) p.children().get(0)).condition();
247+
assertTrue(p instanceof Filter);
248+
Expression condition = ((Filter) p).condition();
250249
assertFalse(condition.foldable());
251250
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
252251
assertNull(translation.query);
@@ -258,9 +257,8 @@ public void testTranslateIsNullExpression_HavingClause_Painless() {
258257

259258
public void testTranslateIsNotNullExpression_HavingClause_Painless() {
260259
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IS NOT NULL");
261-
assertTrue(p instanceof Project);
262-
assertTrue(p.children().get(0) instanceof Filter);
263-
Expression condition = ((Filter) p.children().get(0)).condition();
260+
assertTrue(p instanceof Filter);
261+
Expression condition = ((Filter) p).condition();
264262
assertFalse(condition.foldable());
265263
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
266264
assertNull(translation.query);
@@ -327,9 +325,8 @@ public void testTranslateInExpression_WhereClause_Painless() {
327325

328326
public void testTranslateInExpression_HavingClause_Painless() {
329327
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 20, 30 - 10)");
330-
assertTrue(p instanceof Project);
331-
assertTrue(p.children().get(0) instanceof Filter);
332-
Expression condition = ((Filter) p.children().get(0)).condition();
328+
assertTrue(p instanceof Filter);
329+
Expression condition = ((Filter) p).condition();
333330
assertFalse(condition.foldable());
334331
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
335332
assertNull(translation.query);
@@ -342,9 +339,8 @@ public void testTranslateInExpression_HavingClause_Painless() {
342339

343340
public void testTranslateInExpression_HavingClause_PainlessOneArg() {
344341
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, 30 - 20)");
345-
assertTrue(p instanceof Project);
346-
assertTrue(p.children().get(0) instanceof Filter);
347-
Expression condition = ((Filter) p.children().get(0)).condition();
342+
assertTrue(p instanceof Filter);
343+
Expression condition = ((Filter) p).condition();
348344
assertFalse(condition.foldable());
349345
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
350346
assertNull(translation.query);
@@ -358,9 +354,8 @@ public void testTranslateInExpression_HavingClause_PainlessOneArg() {
358354

359355
public void testTranslateInExpression_HavingClause_PainlessAndNullHandling() {
360356
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING max(int) IN (10, null, 20, 30, null, 30 - 10)");
361-
assertTrue(p instanceof Project);
362-
assertTrue(p.children().get(0) instanceof Filter);
363-
Expression condition = ((Filter) p.children().get(0)).condition();
357+
assertTrue(p instanceof Filter);
358+
Expression condition = ((Filter) p).condition();
364359
assertFalse(condition.foldable());
365360
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
366361
assertNull(translation.query);
@@ -377,9 +372,8 @@ public void testTranslateMathFunction_HavingClause_Painless() {
377372

378373
LogicalPlan p = plan("SELECT keyword, max(int) FROM test GROUP BY keyword HAVING " +
379374
operation.name() + "(max(int)) > 10");
380-
assertTrue(p instanceof Project);
381-
assertTrue(p.children().get(0) instanceof Filter);
382-
Expression condition = ((Filter) p.children().get(0)).condition();
375+
assertTrue(p instanceof Filter);
376+
Expression condition = ((Filter) p).condition();
383377
assertFalse(condition.foldable());
384378
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
385379
assertNull(translation.query);
@@ -390,4 +384,19 @@ public void testTranslateMathFunction_HavingClause_Painless() {
390384
assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->"));
391385
assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]"));
392386
}
387+
388+
public void testGroupByAndHavingWithFunctionOnTopOfAggregation() {
389+
LogicalPlan p = plan("SELECT keyword, MAX(int) FROM test GROUP BY 1 HAVING ABS(MAX(int)) > 10");
390+
assertTrue(p instanceof Filter);
391+
Expression condition = ((Filter) p).condition();
392+
assertFalse(condition.foldable());
393+
QueryTranslation translation = QueryTranslator.toQuery(condition, true);
394+
assertNull(translation.query);
395+
AggFilter aggFilter = translation.aggFilter;
396+
assertEquals("InternalSqlScriptUtils.nullSafeFilter(InternalSqlScriptUtils.gt(InternalSqlScriptUtils.abs" +
397+
"(params.a0),params.v0))",
398+
aggFilter.scriptTemplate().toString());
399+
assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->"));
400+
assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]"));
401+
}
393402
}

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

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

477477
@SuppressWarnings("unchecked")
478478
Map<String, Object> aggregations2 = (Map<String, Object>) groupby.get("aggregations");
479-
assertEquals(3, aggregations2.size());
479+
assertEquals(2, aggregations2.size());
480480

481481
List<Integer> aggKeys = new ArrayList<>(2);
482482
String aggFilterKey = null;
@@ -496,7 +496,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
496496
}
497497
}
498498
Collections.sort(aggKeys);
499-
assertEquals("having." + aggKeys.get(1), aggFilterKey);
499+
assertEquals("having." + aggKeys.get(0), aggFilterKey);
500500

501501
@SuppressWarnings("unchecked")
502502
Map<String, Object> having = (Map<String, Object>) aggregations2.get(aggFilterKey);
@@ -510,7 +510,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
510510
@SuppressWarnings("unchecked")
511511
Map<String, Object> bucketsPath = (Map<String, Object>) bucketSelector.get("buckets_path");
512512
assertEquals(1, bucketsPath.size());
513-
assertEquals(aggKeys.get(1).toString(), bucketsPath.get("a0"));
513+
assertEquals(aggKeys.get(0).toString(), bucketsPath.get("a0"));
514514

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

0 commit comments

Comments
 (0)