Skip to content

Commit a8e7db0

Browse files
924060929924060929
committed
[fix](nereids) address review: outer-join nullable and constant group-by key
- require isUniformAndNotNull (not isUniform) so a uniform-but-nullable slot from the nullable side of an outer join is not wrapped in any_value (it can be NULL on unmatched rows of a group) - skip the relaxation when a constant group-by key is present: such keys are eliminated and the all-constant aggregate is later collapsed without preserving the any_value aliases, so keep the original rejection in that case (no behavior change vs before the relaxation) - pass the current aggregate output to eliminateGroupByConstant instead of the stale snapshot - regression test: use qt_* with a generated .out instead of manual assertEquals; drop the table only before use, not at the end - add unit tests for the outer-join-nullable and constant-group-by-key cases
1 parent aa46e75 commit a8e7db0

4 files changed

Lines changed: 60 additions & 20 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -301,16 +301,25 @@ public LogicalPlan normalizeAgg(LogicalAggregate<Plan> aggregate, Optional<Logic
301301
if (!missingSlotsInAggregate.isEmpty()) {
302302
if (SqlModeHelper.hasOnlyFullGroupBy()) {
303303
// Under only_full_group_by, a non-aggregated output expression is allowed only when it
304-
// is constant for every input row (a uniform slot). This matches MySQL, which accepts
305-
// such expressions by functional dependency, e.g.
304+
// has a single value for every input row of the aggregate. This matches MySQL, which
305+
// accepts such expressions by functional dependency, e.g.
306306
// SELECT a AS b, b AS c FROM (SELECT 1 AS a, 2 AS b) t GROUP BY b, c
307-
// here 'a' is a constant column of the derived table. Non-constant missing slots are
308-
// still rejected. The constant ones fall through to the any_value() wrapping below
309-
// (any_value of a constant is that constant), so the result stays unambiguous.
307+
// where 'a' is a constant column of the derived table.
308+
//
309+
// Two guards keep this relaxation correct:
310+
// 1. isUniformAndNotNull (not plain isUniform): a uniform slot may come from the nullable
311+
// side of an outer join (see DataTrait.addUniformSlotForOuterJoinNullableSide), where it
312+
// holds the uniform value on matched rows but NULL on unmatched rows of the same group,
313+
// so any_value() would be ambiguous.
314+
// 2. no constant group-by key: a constant key is removed by eliminateGroupByConstant below
315+
// and the all-constant aggregate is later collapsed, which does not preserve the
316+
// any_value() aliases we would add here. In that case keep the original rejection (this
317+
// matches the behavior before this relaxation).
318+
boolean hasConstantGroupByKey = groupingByExprs.stream().anyMatch(Expression::isConstant);
310319
DataTrait childTrait = aggregate.child().getLogicalProperties().getTrait();
311320
List<String> invalidMissingSlots = new ArrayList<>();
312321
for (Slot slot : missingSlotsInAggregate) {
313-
if (!childTrait.isUniform(slot)) {
322+
if (hasConstantGroupByKey || !childTrait.isUniformAndNotNull(slot)) {
314323
invalidMissingSlots.add("'" + slot.getName() + "'");
315324
}
316325
}
@@ -359,8 +368,12 @@ public LogicalPlan normalizeAgg(LogicalAggregate<Plan> aggregate, Optional<Logic
359368
LogicalAggregate<?> newAggregate =
360369
aggregate.withNormalized(normalizedGroupExprs, normalizedAggOutputBuilder.build(), bottomPlan);
361370
ExpressionRewriteContext rewriteContext = new ExpressionRewriteContext(ctx);
371+
// Use the current aggregate output (newAggregate already contains any any_value aliases added for
372+
// missing constant slots above), not the stale normalizedAggOutput snapshot, so constant group-key
373+
// elimination does not drop those aliases and leave dangling references in upperProjects.
362374
LogicalProject<Plan> project = eliminateGroupByConstant(groupByExprContext, rewriteContext,
363-
normalizedGroupExprs, normalizedAggOutput, bottomProjects, aggregate, upperProjects, newAggregate);
375+
normalizedGroupExprs, newAggregate.getOutputExpressions(), bottomProjects, aggregate,
376+
upperProjects, newAggregate);
364377

365378
if (!having.isPresent()) {
366379
return project;

‎fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/OnlyFullGroupByConstantTest.java‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@
2525
import org.junit.jupiter.api.Test;
2626

2727
/**
28-
* Under only_full_group_by, a non-aggregated select expression that is constant for every input row
29-
* (a uniform slot) is allowed, matching MySQL functional-dependency behavior. Non-constant
30-
* non-aggregated expressions are still rejected.
28+
* Under only_full_group_by, a non-aggregated select expression that has a single value for every input
29+
* row (uniform and not null) is allowed, matching MySQL functional-dependency behavior. Non-constant
30+
* expressions, and uniform-but-nullable expressions from the nullable side of an outer join, are rejected.
3131
*/
3232
public class OnlyFullGroupByConstantTest extends TestWithFeService {
3333
@Override
@@ -38,6 +38,9 @@ protected void runBeforeAll() throws Exception {
3838
createTable("CREATE TABLE test.t_ab (a INT, b INT) ENGINE=OLAP\n"
3939
+ "DUPLICATE KEY(a) DISTRIBUTED BY HASH(a) BUCKETS 1\n"
4040
+ "PROPERTIES ('replication_num' = '1');");
41+
createTable("CREATE TABLE test.t_v (k INT, v INT) ENGINE=OLAP\n"
42+
+ "DUPLICATE KEY(k) DISTRIBUTED BY HASH(k) BUCKETS 1\n"
43+
+ "PROPERTIES ('replication_num' = '1');");
4144
}
4245

4346
@Test
@@ -55,6 +58,19 @@ public void constantColumnNotInGroupBy() {
5558
.analyze("SELECT a, b FROM (SELECT 1 as a, 2 as b) t1 GROUP BY a"));
5659
}
5760

61+
@Test
62+
public void constantGroupKeyStillRejected() {
63+
// a constant group-by key ('g') is eliminated and the all-constant aggregate is later collapsed,
64+
// which would not preserve any_value() aliases. The relaxation is skipped in that case, so the
65+
// query keeps the original rejection (same behavior as before this change) rather than producing
66+
// a dangling plan.
67+
AnalysisException ex = Assertions.assertThrows(AnalysisException.class,
68+
() -> PlanChecker.from(connectContext)
69+
.analyze("SELECT a, b FROM (SELECT 1 as a, 2 as b) t1 GROUP BY 'g'"));
70+
Assertions.assertTrue(ex.getMessage().contains("must appear in the GROUP BY"),
71+
"unexpected message: " + ex.getMessage());
72+
}
73+
5874
@Test
5975
public void nonConstantColumnStillRejected() {
6076
// same shape over a real table: 'a' is non-constant and ungrouped -> still rejected (MySQL parity)
@@ -74,4 +90,16 @@ public void mixedMissingSlotsReportOnlyNonConstant() {
7490
Assertions.assertTrue(ex.getMessage().contains("'a'"), "should report 'a': " + ex.getMessage());
7591
Assertions.assertFalse(ex.getMessage().contains("'k'"), "should NOT report constant 'k': " + ex.getMessage());
7692
}
93+
94+
@Test
95+
public void outerJoinNullableUniformRejected() {
96+
// r.v is uniform (LIMIT 1) but propagated through the nullable side of a LEFT JOIN, so it is the
97+
// uniform value on matched rows and NULL on unmatched rows of the same group -> must stay rejected.
98+
AnalysisException ex = Assertions.assertThrows(AnalysisException.class,
99+
() -> PlanChecker.from(connectContext).analyze(
100+
"SELECT r.v FROM (SELECT 1 AS g, 1 AS k UNION ALL SELECT 1 AS g, 2 AS k) l "
101+
+ "LEFT JOIN (SELECT k, v FROM test.t_v LIMIT 1) r ON l.k = r.k GROUP BY l.g"));
102+
Assertions.assertTrue(ex.getMessage().contains("must appear in the GROUP BY"),
103+
"unexpected message: " + ex.getMessage());
104+
}
77105
}

‎regression-test/data/nereids_syntax_p0/test_group_by_constant_output.out‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
-- This file is automatically generated. You should know what you did if you want to edit this
2+
-- !const_alias_collision --
3+
1 2
4+
5+
-- !const_not_in_groupby --
6+
1 2
7+

‎regression-test/suites/nereids_syntax_p0/test_group_by_constant_output.groovy‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,10 @@ suite("test_group_by_constant_output") {
2222
// The output alias 'b' collides with the derived-table column 'b', so under only_full_group_by the
2323
// column 'b' is grouped and the constant column 'a' is left ungrouped. Because 'a' is constant for
2424
// every input row it is accepted (MySQL functional-dependency behavior), and the query returns (1, 2).
25-
def r1 = sql "SELECT a as b, b as c FROM (SELECT 1 as a, 2 as b) t1 GROUP BY b, c"
26-
assertEquals(1, r1.size())
27-
assertEquals(1, (r1[0][0] as int))
28-
assertEquals(2, (r1[0][1] as int))
25+
qt_const_alias_collision "SELECT a as b, b as c FROM (SELECT 1 as a, 2 as b) t1 GROUP BY b, c"
2926

3027
// A constant column not present in GROUP BY is allowed even though it is neither grouped nor aggregated.
31-
def r2 = sql "SELECT a, b FROM (SELECT 1 as a, 2 as b) t1 GROUP BY a"
32-
assertEquals(1, r2.size())
33-
assertEquals(1, (r2[0][0] as int))
34-
assertEquals(2, (r2[0][1] as int))
28+
qt_const_not_in_groupby "SELECT a, b FROM (SELECT 1 as a, 2 as b) t1 GROUP BY a"
3529

3630
// The same shape over a real table leaves a non-constant column ungrouped, which is still rejected
3731
// (matching MySQL, where only constant / functionally-dependent columns are allowed).
@@ -46,6 +40,4 @@ suite("test_group_by_constant_output") {
4640
sql "SELECT a as b, b as c FROM test_gb_const_t GROUP BY b, c"
4741
exception "must appear in the GROUP BY"
4842
}
49-
50-
sql "DROP TABLE IF EXISTS test_gb_const_t"
5143
}

0 commit comments

Comments
 (0)