[fix](fe) Fix correlated outer column in QUALIFY being mis-handled after GROUP BY / over project - #67152
[fix](fe) Fix correlated outer column in QUALIFY being mis-handled after GROUP BY / over project#67152starocean999 wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two blocking FE/Nereids correctness issues remain in correlated QUALIFY handling: a dual HAVING/QUALIFY correlation can leave one outer predicate stranded below the window project, and a SELECT alias can hide an outer producer so EXISTS is lowered as uncorrelated. Both end in invalid plans rather than the intended rows.
Critical checkpoint conclusions:
- Goal and functional proof: direct outer-slot references in the aggregate, project, and HAVING-project shapes are addressed, but the implementation does not cover the two legal variants called out inline. The fourth
Qualify(Having(Aggregate))branch is not proven by the added tests. - Plan semantics and parallel paths: binding uses the correct shared outer
Scopeand ExprIds. Direct references, DISTINCT reconstruction, HAVING/project, UNION-backed inputs, EXISTS/IN differences, window-project barriers, and APPLY-to-join lowering were traced end to end; the two inline failures are the surviving gaps. - Change scope: the patch is small and focused. No concurrency, lifecycle, configuration, protocol/storage compatibility, persistence, transaction, data-write, or FE/BE variable-passing concerns apply.
- Tests and expected results: the three success outputs are deterministic and coherent, and the ONLY_FULL_GROUP_BY negative is preserved. Missing regressions are the dual correlated HAVING+QUALIFY aggregate shape and the outer-slot SELECT-alias shape. Per the review-only runner instructions, no build or regression test was run; live Checkstyle, formatting, license, title, and secret checks are passing.
- Performance and observability: the added analysis-time set membership is negligible; no new runtime observability is required.
- User focus: no additional focus was supplied.
Please address both inline blockers and add end-to-end regressions before merge.
2532fec to
a6283f8
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Capped/incomplete after the mandatory three review rounds: this patch addresses the two existing correlated-QUALIFY threads, but ten additional correctness failures remain, consolidated into seven inline comments. The final round still produced a new accepted nested-IN ownership issue, so this review cannot claim convergence. The two earlier live P1 threads remain applicable and are not duplicated here.
Critical checkpoint conclusions:
- Goal and proof: direct outer-slot cases improve, but grouped aliases, HAVING/window ordering, DISTINCT, computed producers, IN callers, and an earlier UNION path are not handled end to end. The added tests therefore do not prove the stated goal.
- Scope and focus: the source change is small and focused, and no extra user focus was supplied; the semantic blast radius is nevertheless broad because the helper is shared by all four QUALIFY shapes and multiple Apply callers.
- Concurrency: these are single-threaded analyzer/rewrite rules; no shared-state or locking concern applies.
- Lifecycle: no resource or non-intuitive object lifecycle is introduced.
- Configuration: no configuration item is added or changed.
- Compatibility: no storage, protocol, symbol, rolling-upgrade, or FE/BE compatibility surface changes.
- Parallel paths: the earlier
PushProjectThroughUnionpath bypasses the new late guard, and EXISTS, scalar, IN/NOT IN, and mark callers have different output-ownership contracts. These paths were traced through Apply-to-Join and final validation. - Conditional checks and error behavior:
getInputSlots()containment is not sufficient proof that a HAVING predicate or alias producer can cross a window/subquery boundary; several cases change rows/errors or create dangling slots. - Test coverage: the JUnit tests stop after selected rewrites rather than final executable-plan validation. Regressions omit the grouped-alias, DISTINCT dual-correlation, HAVING phase, window/subquery producer, IN/mark, nested-IN, and width-matched UNION cases.
- Test results: the committed expected outputs and expected-error form are coherent. Per the review-only prompt, no build or test was run locally.
- Observability: no new runtime observability is needed for these planner-only changes.
- Transactions, persistence, and data writes: not involved.
- FE/BE variables: none are added or transmitted.
- Performance: the new analysis-time set scans are small; no material performance issue was found.
- Other current state: live head/base still match the authoritative bundle. Checkstyle, formatting, compile, BE UT, Cloud UT, and coverage are passing;
cloud_p0is currently failing and several regression/FE-UT/performance jobs remain pending, without enough evidence here to attribute that failure to this patch.
Please address the existing threads and all inline blockers, add full-pipeline regressions, and rerun review from a fresh authoritative bundle.
TPC-H: Total hot run time: 16851 ms |
TPC-DS: Total hot run time: 82544 ms |
ClickBench: Total hot run time: 14.62 s |
|
/review |
There was a problem hiding this comment.
Requesting changes. Two complete review rounds were reconciled, and every Round 2 normal/risk pass returned NO_NEW_VALUABLE_FINDINGS. The UNION ownership guards look sound, but three new blockers remain: correlated alias repair can duplicate volatile producers, the DISTINCT repair can relocate a volatile QUALIFY predicate across the distinct barrier, and the new negative regression cannot reach the analyzer error it intends to assert. I did not repeat the nine existing inline issues; those remain part of the review context.
Critical checkpoint conclusions:
- Goal and proof: The goal is to make correlated HAVING/QUALIFY aliases and set-operation rewrites survive analysis and Apply decorrelation. The revision only partially accomplishes that goal: the two volatile-expression cases change query semantics, and the combined-statement regression does not prove its target path. The FE tests stop before final Apply-to-Join/slot validation.
- Scope and clarity: The changes are localized, but one rule now performs several dependency classifications and predicate motions whose safety conditions are incomplete. The new
WindowExpression/SubqueryExprexclusions explain two hazards but omit evaluation identity and movability. - Concurrency and lifecycle: No concurrent state, locks, static initialization, or resource lifecycle is introduced. I traced the relevant analyzer lifecycle through binding,
FillUpQualifyMissingSlot, aggregate/window normalization,SubqueryToApply, Apply decorrelation/conversion, and final slot validation. - Configuration and compatibility: No production configuration, FE-BE protocol, function symbol, or storage-format change is present. Rolling-upgrade compatibility is not applicable. The test-only SQL-mode setup is malformed for the standard single-statement JDBC path.
- Parallel paths and conditions: Scalar, EXISTS, IN/NOT IN, and mark callers plus regular/constant UNION children and the
LogicalSetOperation/ProjectProcessor path were checked. The UNION ownership guard is shared correctly, but alias substitution and DISTINCT predicate classification need volatile/non-movable handling. - Tests and expected results: The ordered output blocks for
select_37throughselect_45are consistent with their SQL. The new JUnit cases are deterministic but do not exercise the full rewrite/validation pipeline. The negative regression sendsSET; SELECTas one prepared statement, and there is no coverage for the two volatile-expression semantics reported inline. No build or test was run because the review prompt explicitly prohibited it; conclusions are from the authoritative diff and inspected execution paths. - Observability, persistence, writes, and FE-BE variables: Not applicable; this is an FE analyzer/rewrite-only change with no logging/metrics need, EditLog/transaction path, data-write path, or transmitted variable.
- Performance and other risks: No separate performance defect was found. Duplicate volatile evaluation is primarily a correctness failure. Existing HAVING/window, alias ownership, aggregate alias, DISTINCT ownership, nested Apply, and UNION-test concerns were deduplicated against the live threads.
- User focus: No additional focus was supplied; the complete six-file PR scope was reviewed.
4f1f8fc to
7be64cb
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes. Two complete review rounds converged: all three Round 2 scopes returned NO_NEW_VALUABLE_FINDINGS, leaving exactly two new P1 blockers. An unrelated subquery can globally disable the safe correlated-alias repair, and the DISTINCT branch can relocate a predicate while ignoring dependencies hidden inside its nested subquery plan. Existing live inline concerns were deduplicated rather than repeated.
Critical checkpoint conclusions:
- Goal and correctness: The patch improves direct correlated QUALIFY ownership, but the two inline cases still produce dangling slots or invalid Apply placement, so the stated goal is incomplete for mixed subqueries.
- Scope and parallel paths: All four
Qualify(Project),Qualify(Aggregate),Qualify(Having(Aggregate)), andQualify(Having(Project))shapes were traced through binding, window/DISTINCT normalization,SubqueryToApply, Apply conversion, and final slot validation. Scalar, EXISTS, IN/NOT IN, and mark variants were checked. The regular/constant UNION guards are shared by the normal and CTE schedules,ProjectProcessor, and batch-insert path; no additional bypass survived. - Conditions and errors: The query-wide subquery fence is too broad, while
getInputSlots()is too narrow to prove nested-subquery ownership across DISTINCT. Explicit analysis rejection is acceptable where complete dependencies cannot be preserved. - Tests and expected results: Added unit/regression cases cover direct correlations, aggregate rejection, volatility, and basic UNION shapes, but not either accepted mixed-subquery failure. Other test weaknesses were fenced against existing live threads. Per the review-only instructions, no build or test command was run.
- Concurrency, lifecycle, configuration, and compatibility: These are statement-local FE rewrite changes with no shared-state, locking, lifecycle, configuration, FE-BE protocol, storage-format, persistence, transaction, data-write, or rolling-upgrade impact.
- Performance and observability: The added scans are linear over small expression/project lists; no material performance, memory-accounting, logging, or metrics issue was found.
- User focus and completion: No additional focus was supplied. The review is complete and converged after the final changed-file and duplicate sweep.
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
Requesting changes for one new current-head correctness issue.
Goal and data correctness: the UNION ownership guards are sound, and the QUALIFY work repairs several correlation paths, but the new nested-subquery fence rejects a legal non-DISTINCT plain-project plan even when the same child-owned slot is already scheduled to be carried as a support column. The task goal is therefore incomplete.
Scope, conditions, and parallel paths: I traced all four FillUpQualifyMissingSlot shapes through binding, window extraction, subquery-to-Apply, Apply-to-Join, and final ExprId validation, and traced both UNION guards through regular, constant, CTE/column-pruning, rewrite-pipeline, and batch-insert callers. No additional non-duplicate issue survived. The broad line-433 condition, rather than the focused UNION guards, is the remaining problem.
Tests and results: the changed Java and regression expectations were inspected statically. They cover the DISTINCT rejection but omit the supportable non-DISTINCT branch in the inline comment. No build or test was run because this review task explicitly prohibits execution.
Other critical checkpoints: this is stateless FE analyzer/rewrite logic; it introduces no concurrency, lifecycle, configuration, FE-BE protocol, persistence, transaction/data-write, memory-safety, BE-nullability, or observability concern. The added projection/conjunct scans are bounded and present no material performance risk. The analysis error text is clear, but its triggering condition is too broad.
Review completion: converged after two rounds; every normal and risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS. Existing alias, DISTINCT, volatility, HAVING/window, IN/mark, producer, UNION-test, and SQL-mode threads were treated as duplicate fences and were not reposted. No additional user focus was supplied.
|
/review |
There was a problem hiding this comment.
Request changes: one additional P1 correctness gap remains in the grouped correlated-QUALIFY path. The review is complete after three rounds: every candidate was independently verified and deduplicated, all Round 3 reviewers returned NO_NEW_VALUABLE_FINDINGS, and existing threads were treated as hard duplicate fences.
Critical checkpoint conclusions:
- Goal and data correctness: the change repairs several correlated-slot ownership cases across Project, Aggregate, HAVING, DISTINCT, nested subqueries, and UNION rewrites, but it still rejects a legal grouped correlated query when the outer column is selected directly rather than through an Alias. The inline comment identifies that distinct omission.
- Scope and parallel paths: all four changed QUALIFY shapes, scalar/EXISTS/IN/NOT IN/mark ownership, nested correlations, normal/CTE/ProjectProcessor/VALUES set-operation paths, and UNION/INTERSECT/EXCEPT variants were traced. Other concrete residual failures are already covered by existing review threads and were not duplicated.
- Tests and result artifacts: static review found the 15 added regression labels, ordering, exception forms, and expected outputs internally consistent. The FE tests are intermediate structural checks rather than complete Apply-to-Join/final-validation proofs, and no test selects a raw correlated outer slot in grouped aggregate output. No build or test was run, as required by the review task.
- Error handling and observability: recognized unsupported forms throw AnalysisException, but the accepted issue bypasses the targeted classification and reaches a misleading GROUP BY error. No new runtime or distributed path needs metrics or logging.
- Concurrency, lifecycle, and memory: not applicable; these are synchronous, query-local immutable Java planner rewrites with bounded query-lifetime collections.
- Configuration and compatibility: no product configuration, public API, serialized/storage format, FE-BE protocol, or mixed-version contract changes. The sql_mode edits are regression setup only.
- Persistence, transactions, writes, and FE-BE variables: not applicable; no EditLog, failover, transaction, visible-version, metadata/data-write, crash-atomicity, or transmitted-variable path is touched.
- Performance: added scans and set-containment checks are bounded by project/conjunct width during FE planning; no separate performance issue was found.
- User focus: no additional focus point was provided.
Existing review threads remain the authoritative locations for previously raised issues; this review adds only the distinct raw aggregate-output case below.
|
/review |
There was a problem hiding this comment.
Requesting changes for one new current-head P1: the HAVING-bearing QUALIFY rules repair outer-dependent output aliases only when QUALIFY consumes them, so an alias used solely by HAVING remains hidden and the correlated EXISTS reaches a dangling-slot or false GROUP BY failure. Existing inline discussions were treated as hard duplicate fences and were not reposted.
Review completion: capped/incomplete. Round 3 Normal A and Normal B returned NO_NEW_VALUABLE_FINDINGS, but the adversarial Round 3 pass produced this accepted issue; the required three-round cap therefore prevents claiming convergence.
Critical checkpoints:
- Goal and tests: the patch targets correlated QUALIFY ownership across project, aggregate, HAVING, DISTINCT, and UNION paths. The added JUnit cases exercise intermediate Apply state and the regressions exercise the full planner pipeline, but they miss the HAVING-alias cross-product reported inline. Expected rows, ordering, exception patterns, and SQL-mode statement boundaries were inspected.
- Correctness and parallel paths: all four QUALIFY child shapes, scalar/EXISTS/IN/NOT IN/mark Apply paths, normal/CTE unnesting, final ExprId validation, both UNION guards, processor/column-pruning, and insert paths were traced. Remaining previously known mechanisms stay in their existing threads.
- Scope and conditions: the change is focused per-statement FE analysis/rewrite logic. Error paths use AnalysisException or retain the original plan; this newly found path instead falls through to a late invalid-plan failure.
- Concurrency, lifecycle, configuration, compatibility, persistence, transactions/data writes, FE/BE wire state, and observability: not applicable to this analysis-only change.
- Performance: the added expression, projection, correlation, and set-width scans are bounded and introduce no material hot-path concern.
- Validation: static bundle/source review only; no builds or tests were run because the review runner explicitly prohibited them.
User focus: no additional focus points were supplied.
…ter GROUP BY / over project fix comments fix comments fix fix comment fix comment fix comment
673f6e1 to
2796f10
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review complete for head 2796f10c5d3bef8b6fa02b6f8e65e4260f704d9e against base 3ba29643fa0aa1ca5c86ae831d64ee888b0b212f.
No new distinct inline issue survived two complete review rounds. The two additional reachable shapes found during this pass—a mixed inner/outer SELECT-alias producer and a strict-grouping EXISTS with an unused outer-owned aggregate output—are substantially the same ownership failures at the same changed locations already covered by discussions r3860340415/r3864355033 and r3864355004/r3877275427. I am not reposting them under the hard duplicate fence. The other candidates were either unreachable through normal binding, rejected by existing scalar/IN/set-operation validators, or covered by the remaining live P1 threads. This is therefore a comment-only review, not an approval of unresolved existing P1 threads.
Checkpoint conclusions:
- Goal and correctness: the patch targets correlation and slot ownership across QUALIFY/HAVING, window, aggregate, DISTINCT, Apply, and UNION rewrites. The newly handled direct cases are coherent, but broader ownership/evaluation-domain concerns remain represented by the existing inline threads.
- Scope and parallel paths: all six authoritative changed files were reviewed. Both UNION project-push entry points and their rewriter/CTE/column-pruning callers use the ownership invariant; direct branch-local correlations are rejected before those rewrites.
- Conditions and compatibility: project/aggregate, QUALIFY/HAVING, DISTINCT/non-DISTINCT, strict/relaxed grouping, EXISTS/NOT EXISTS/scalar/IN/NOT IN/mark, nested subqueries, and regular/constant UNION paths were traced. There are no configuration-schema, persistence, FE-BE protocol, storage/wire, transaction, or data-write changes.
- Concurrency, lifecycle, observability, and performance: these are statement-local immutable logical-plan rewrites with no shared-state, locking, resource-lifecycle, or new observability contract. The added expression/output scans are linear over small planner collections and do not introduce a material planning hot-path concern.
- Tests: the JUnit and regression additions, expected rows, negative exception checks, ordering, volatility stabilization, and SQL-mode transitions are internally consistent. The FE unit tests stop before final Apply-to-Join validation, and
select_45does not reach the new UNION containment branch due to its width mismatch; those concrete coverage limitations are already covered by existing threads and were not duplicated. No build or test command was run because this review workflow explicitly prohibits builds. - User focus:
review_focus.txtsupplied no additional focus beyond the complete PR review.
Completion status: complete for this exact head; zero new inline comments after strict deduplication against all 17 live review threads.
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
When a correlated subquery uses QUALIFY and references an outer column inside the
QUALIFY clause, the Nereids analyzer mishandles that outer column in all four
FillUpQualifyMissingSlotplan shapes:Qualify(Aggregate)/Qualify(Having, Aggregate)(explicit GROUP BY):Under the default ONLY_FULL_GROUP_BY SQL mode, the outer column is treated as an
inner non-grouped column and the query is rejected.
Reproduction:
Before the fix this fails with:
error 1105: QUALIFY expression 'flag' must appear in the GROUP BY clause or be used in an aggregate function.After the fix it returns a single row
10.Qualify(Project)/Qualify(Having, Project)(no GROUP BY):The correlated outer column is incorrectly pushed into the inner project's
output, which the inner query cannot produce. This later crashes in
PushProjectIntoUnionwith aNullPointerExceptionwhen the project is pushedinto a UNION. The same query shape (without
GROUP BY) reproduces this NPE;after the fix it also returns
10.Root cause:
BindExpressionbinds the outer column from the enclosing outer Scope and recordsit in the outer Scope's correlated slots.
FillUpQualifyMissingSlot:FILL_UP_QUALIFY_AGGREGATEandFILL_UP_QUALIFY_HAVING_AGGREGATErules builtthe
Resolverwithnew Resolver(agg)WITHOUT the outer scope (unlike theHAVING/SORT missing-slot paths which pass
ctx.cascadesContext.getOuterScope()).Without it, the
Resolvercannot tell a correlated outer slot from an innermissing group-by column, so under ONLY_FULL_GROUP_BY it throws.
FILL_UP_QUALIFY_PROJECTandFILL_UP_QUALIFY_HAVING_PROJECTrules collectmissing slots in
createPlanwithfilter(s -> !projectOutputSet.contains(s))which also does not exclude correlated outer slots (unlike
FillUpMissingSlots.collectNotExistsSlotAndAggFunc), so the outer column getsadded to the inner project output.
The fix passes the outer scope into all four rules (via
thenApply(ctx -> ...)):FILL_UP_QUALIFY_AGGREGATE/FILL_UP_QUALIFY_HAVING_AGGREGATE: theResolvernow receives
ctx.cascadesContext.getOuterScope()and skips slots found in theouter scope's correlated slots, matching the normal missing-slot paths.
FILL_UP_QUALIFY_PROJECT/FILL_UP_QUALIFY_HAVING_PROJECT:createPlannowreceives the outer scope and filters correlated slots out of the project's
notExistedInProject(and the distinct-branchmissingSlots), so outer columnsare never pushed into the inner project's output.
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)