[fix](nereids) allow constant output column not in GROUP BY under only_full_group_by#64458
Open
924060929 wants to merge 2 commits into
Open
[fix](nereids) allow constant output column not in GROUP BY under only_full_group_by#64458924060929 wants to merge 2 commits into
924060929 wants to merge 2 commits into
Conversation
…y_full_group_by Under only_full_group_by, Nereids rejected any non-aggregated select expression that was neither grouped nor wrapped in an aggregate function, even when that expression is constant for every input row. MySQL accepts such expressions via functional dependency, e.g. SELECT a AS b, b AS c FROM (SELECT 1 AS a, 2 AS b) t1 GROUP BY b, c where the output alias 'b' collides with the derived-table column 'b', so column-first resolution groups by the column 'b' and leaves the constant column 'a' ungrouped. NormalizeAggregate now only rejects non-constant missing slots; constant (uniform) ones flow through the existing any_value() wrapping (any_value of a constant is that constant), so the result stays unambiguous and the query returns (1, 2). The same shape over a real table, where the ungrouped column is non-constant, is still rejected, matching MySQL.
924060929
requested review from
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found blocking issues in the only_full_group_by relaxation.
Critical checkpoint conclusions:
- Goal/test: The intended constant-output case is covered, but constant group-key elimination and nullable outer-join uniform propagation are not covered; both affect correctness of the new acceptance path.
- Scope/focus: The change is small, but it relies on DataTrait.isUniform in a context that needs a stronger "same value for every aggregate input row" guarantee.
- Concurrency/lifecycle/config/compatibility/transactions/persistence/FE-BE protocol: Not involved in this PR.
- Parallel paths: Sort/HAVING missing-slot paths were considered; no additional issue found for the stated select-output scope.
- Test coverage/results: Unit and regression tests were added, but the regression test violates Doris regression-test conventions and misses the blocking edge cases above.
- Observability/performance: No material concern found.
- User focus: No additional user-provided review focus was supplied.
| } | ||
| // Under only_full_group_by, a non-aggregated output expression is allowed only when it | ||
| // is constant for every input row (a uniform slot). This matches MySQL, which accepts | ||
| // such expressions by functional dependency, e.g. |
Contributor
There was a problem hiding this comment.
This predicate is too weak for only_full_group_by. DataTrait.isUniform() can be propagated from the nullable side of an outer join via LogicalJoin.computeUniform() -> addUniformSlotForOuterJoinNullableSide(), where a slot can still be NULL on unmatched rows and non-NULL on matched rows in the same later group. For example, with nullable t_v.v: SELECT r.v FROM (SELECT 1 AS g, 1 AS k UNION ALL SELECT 1 AS g, 2 AS k) l LEFT JOIN (SELECT 1 AS k, v FROM t_v LIMIT 1) r ON l.k = r.k GROUP BY l.g. The right LIMIT 1 marks r.v uniform, the left join propagates it, and this branch now wraps r.v in any_value() even though the group can contain both v and NULL. That changes a query that should remain rejected into an ambiguous result. Please require a trait that proves one value for every aggregate input row, or otherwise account for nullable outer-join extension, before allowing the slot.
Contributor
Author
There was a problem hiding this comment.
Fixed in a8e7db0: switched the predicate to isUniformAndNotNull. addUniformSlotForOuterJoinNullableSide only adds entries that are nullable-without-const-value or a NULL literal, so requiring not-null excludes the outer-join case while still allowing genuine non-null constants like 1 AS a. (isUniformAndHasConstValue would be too strict here, since LogicalOneRowRelation.computeUniform records uniform slots without a const value.) Added unit test outerJoinNullableUniformRejected using your repro.
| // For any slots missing in aggregate's output, we add an any_value(slot) into aggregate's | ||
| // output list and the slot itself into the bottom project's output list. When | ||
| // only_full_group_by is enabled, all remaining missing slots are constant (checked above). | ||
| bottomProjects = Sets.union(bottomProjects, missingSlotsInAggregate); |
Contributor
There was a problem hiding this comment.
Appending the any_value alias to the builder is not enough when eliminateGroupByConstant() runs. normalizedAggOutput was built before missing slots are added, and that stale list is passed into eliminateGroupByConstant(). If the group key is a foldable constant, for example SELECT a, b FROM (SELECT 1 AS a, 2 AS b) t GROUP BY 'g', upperProjects is rewritten to these new any_value slots here, but constant-key elimination rebuilds the aggregate from the stale normalizedAggOutput and drops the aliases. The resulting upper project can reference slots the aggregate no longer outputs. Please pass the final aggregate output, or update constant-key elimination to preserve these newly added aliases, and add a test for this combination.
Contributor
Author
There was a problem hiding this comment.
Fixed in a8e7db0, in two parts:
- Pass
newAggregate.getOutputExpressions()(the current output, including the any_value aliases) toeliminateGroupByConstantinstead of the stalenormalizedAggOutputsnapshot. - That alone was not enough. I probed the rewritten plans: a later pass collapses the aggregate after constant-key elimination and does not preserve the any_value aliases. This happens for any constant group-by key (e.g.
GROUP BY k2, 'g'), not only the all-constant case. So the relaxation now skips cases with a constant group-by key and keeps the original rejection there — same behavior as before this PR, and no dangling plan. Added unit testconstantGroupKeyStillRejected.
| // column 'b' is grouped and the constant column 'a' is left ungrouped. Because 'a' is constant for | ||
| // every input row it is accepted (MySQL functional-dependency behavior), and the query returns (1, 2). | ||
| def r1 = sql "SELECT a as b, b as c FROM (SELECT 1 as a, 2 as b) t1 GROUP BY b, c" | ||
| assertEquals(1, r1.size()) |
Contributor
There was a problem hiding this comment.
The added regression test is checking deterministic result rows with assertEquals. The repository regression-test rules require determined expected results to be generated via qt_sql/order_qt_* and the generated .out file, rather than manual assertions. Please convert these positive cases to query tests with deterministic ordering as needed.
Contributor
Author
There was a problem hiding this comment.
Fixed in a8e7db0: converted the positive result cases to qt_* with a generated .out file.
| exception "must appear in the GROUP BY" | ||
| } | ||
|
|
||
| sql "DROP TABLE IF EXISTS test_gb_const_t" |
Contributor
There was a problem hiding this comment.
Please do not drop the regression table at the end. The repository test rules require dropping before use and preserving the final table state for debugging after a failure. The DROP TABLE IF EXISTS before create is enough; remove this final drop.
Contributor
Author
There was a problem hiding this comment.
Fixed in a8e7db0: removed the final DROP and kept only the DROP-before-create.
…-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
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review summary: no new blocking issues found in the current PR diff.
Critical checkpoint conclusions:
- Goal/test coverage: The change now allows non-aggregated output slots only when the aggregate child proves they are uniform and not null under
only_full_group_by. The added FE unit tests cover the accepted constant cases, non-constant rejection, mixed missing-slot reporting, nullable outer-join rejection, and constant group-key rejection. The regression test usesqt_*with a generated.outfile and drops the table before use. - Scope: The current GitHub PR diff is focused on
NormalizeAggregateand matching tests. - Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle, config, persistence, storage-format, or FE-BE protocol concerns in the current diff.
- Parallel paths: This is Nereids aggregate analysis behavior; I did not find another equivalent modified path that also needs the same change.
- Data correctness: The prior outer-join nullable uniform and constant group-key alias-loss concerns are addressed in the current head and covered by tests.
- Observability/performance: No new observability requirement; added work is small trait checks and alias rewrites in analysis.
- Test execution: I did not run the full FE/regression suites in this runner; review was static plus diff/context inspection.
User focus: review_focus.txt says there are no additional user-provided focus points; no extra issue was found there.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29417 ms |
Contributor
TPC-DS: Total hot run time: 168881 ms |
Contributor
TPC-H: Total hot run time: 29368 ms |
Contributor
TPC-DS: Total hot run time: 169534 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
Under
only_full_group_by(the default), Nereids rejected any non-aggregated selectexpression that was neither in
GROUP BYnor wrapped in an aggregate function, even whenthat expression is constant for every input row. MySQL accepts such expressions via
functional dependency. For example:
The output alias
bcollides with the derived-table columnb, so column-first nameresolution groups by the column
band leaves the constant columnaungrouped. Thispreviously failed with:
NormalizeAggregatenow only rejects non-constant missing slots. Constant (uniform)slots flow through the existing
any_value()wrapping (any_value of a constant is thatconstant), so the result stays unambiguous and the query returns
(1, 2), matching MySQL.The same shape over a real table, where the ungrouped column is non-constant, is still
rejected.
Release note
Allow a constant (uniform) non-aggregated column that is not in
GROUP BYunderonly_full_group_by, matching MySQL functional-dependency behavior.Check List (For Author)
Test
Behavior changed:
accepted under
only_full_group_by(previously rejected). Non-constant columns arestill rejected.
Does this need documentation?