Skip to content

[fix](fe) Fix correlated outer column in QUALIFY being mis-handled after GROUP BY / over project - #67152

Closed
starocean999 wants to merge 3 commits into
apache:masterfrom
starocean999:master_0415
Closed

starocean999 wants to merge 3 commits into
apache:masterfrom
starocean999:master_0415

Conversation

@starocean999

Copy link
Copy Markdown
Contributor

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
FillUpQualifyMissingSlot plan shapes:

  1. 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:

    SELECT o.k
    FROM (
      SELECT CAST(10 AS INT) AS k, CAST(1 AS INT) AS flag
      UNION ALL
      SELECT CAST(20 AS INT) AS k, CAST(0 AS INT) AS flag
    ) AS o
    WHERE EXISTS (
      SELECT i.k
      FROM (
        SELECT CAST(1 AS INT) AS k
        UNION ALL
        SELECT CAST(2 AS INT) AS k
      ) AS i
      GROUP BY i.k
      QUALIFY row_number() OVER (ORDER BY i.k) = 1
              AND o.flag = 1
    );

    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.

  2. 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
    PushProjectIntoUnion with a NullPointerException when the project is pushed
    into a UNION. The same query shape (without GROUP BY) reproduces this NPE;
    after the fix it also returns 10.

Root cause:

  • BindExpression binds the outer column from the enclosing outer Scope and records
    it in the outer Scope's correlated slots.
  • In FillUpQualifyMissingSlot:
    • The FILL_UP_QUALIFY_AGGREGATE and FILL_UP_QUALIFY_HAVING_AGGREGATE rules built
      the Resolver with new Resolver(agg) WITHOUT the outer scope (unlike the
      HAVING/SORT missing-slot paths which pass ctx.cascadesContext.getOuterScope()).
      Without it, the Resolver cannot tell a correlated outer slot from an inner
      missing group-by column, so under ONLY_FULL_GROUP_BY it throws.
    • The FILL_UP_QUALIFY_PROJECT and FILL_UP_QUALIFY_HAVING_PROJECT rules collect
      missing slots in createPlan with filter(s -> !projectOutputSet.contains(s))
      which also does not exclude correlated outer slots (unlike
      FillUpMissingSlots.collectNotExistsSlotAndAggFunc), so the outer column gets
      added 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: the Resolver
    now receives ctx.cascadesContext.getOuterScope() and skips slots found in the
    outer scope's correlated slots, matching the normal missing-slot paths.
  • FILL_UP_QUALIFY_PROJECT / FILL_UP_QUALIFY_HAVING_PROJECT: createPlan now
    receives the outer scope and filters correlated slots out of the project's
    notExistedInProject (and the distinct-branch missingSlots), so outer columns
    are never pushed into the inner project's output.

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Scope and 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.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PushProjectThroughUnion path 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_p0 is 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.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 16851 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit a6283f84be3356b7ce004e62f594f56fa2af3c2a, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17549	3053	3039	3039
q2	2114	255	232	232
q3	10243	871	526	526
q4	4668	246	199	199
q5	7685	581	386	386
q6	139	115	94	94
q7	527	519	392	392
q8	9233	871	895	871
q9	3498	2438	2427	2427
q10	6527	853	700	700
q11	392	198	181	181
q12	618	259	196	196
q13	18158	1550	1168	1168
q14	160	152	142	142
q15	q16	432	395	370	370
q17	1355	943	821	821
q18	3071	2228	2231	2228
q19	1119	955	765	765
q20	373	286	203	203
q21	5245	1678	1906	1678
q22	323	274	233	233
Total cold run time: 93429 ms
Total hot run time: 16851 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3465	3368	3350	3350
q2	521	400	378	378
q3	2260	2385	2168	2168
q4	1196	1156	890	890
q5	2208	2122	2131	2122
q6	168	125	89	89
q7	1051	956	901	901
q8	1628	1448	1437	1437
q9	3166	3121	3122	3121
q10	1854	1819	1624	1624
q11	361	276	256	256
q12	459	431	342	342
q13	1493	1565	1162	1162
q14	168	170	171	170
q15	q16	393	399	366	366
q17	3601	3430	3266	3266
q18	4874	4446	4730	4446
q19	1516	901	860	860
q20	986	970	846	846
q21	3768	3049	3249	3049
q22	404	347	327	327
Total cold run time: 35540 ms
Total hot run time: 31170 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 82544 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit a6283f84be3356b7ce004e62f594f56fa2af3c2a, data reload: false

query5	4270	438	339	339
query6	384	135	124	124
query7	4929	421	236	236
query8	299	127	120	120
query9	8674	2907	2954	2907
query10	378	215	190	190
query11	5370	1025	910	910
query12	122	70	70	70
query13	1175	437	338	338
query14	6113	2229	2110	2110
query14_1	2008	1995	1981	1981
query15	180	115	111	111
query16	929	380	391	380
query17	812	454	373	373
query18	2328	330	237	237
query19	160	140	114	114
query20	78	70	73	70
query21	201	104	93	93
query22	5447	5298	5534	5298
query23	6651	6217	6022	6022
query23_1	5974	6121	6064	6064
query24	7296	1101	783	783
query24_1	783	784	781	781
query25	407	287	231	231
query26	1236	251	127	127
query27	2775	416	256	256
query28	4697	1484	1487	1484
query29	953	443	343	343
query30	250	157	124	124
query31	820	414	326	326
query32	133	77	69	69
query33	444	210	165	165
query34	987	867	497	497
query35	411	400	332	332
query36	564	579	551	551
query37	120	79	69	69
query38	1001	855	820	820
query39	505	475	488	475
query39_1	496	450	474	450
query40	200	89	75	75
query41	55	55	52	52
query42	74	79	71	71
query43	242	246	212	212
query44	1019	547	555	547
query45	113	107	100	100
query46	778	827	515	515
query47	780	770	712	712
query48	303	332	221	221
query49	548	235	192	192
query50	766	255	195	195
query51	8308	8258	8218	8218
query52	76	71	69	69
query53	200	207	154	154
query54	244	282	179	179
query55	78	65	57	57
query56	219	180	182	180
query57	688	676	663	663
query58	222	163	162	162
query59	1209	1229	1100	1100
query60	279	181	197	181
query61	118	130	128	128
query62	353	210	179	179
query63	171	145	142	142
query64	2763	683	585	585
query65	1610	1655	1612	1612
query66	1837	272	216	216
query67	10141	9634	9777	9634
query68	2764	1117	751	751
query69	357	222	189	189
query70	675	611	594	594
query71	254	176	171	171
query72	2328	1747	1618	1618
query73	666	590	339	339
query74	1587	1229	1149	1149
query75	1174	1101	971	971
query76	2293	748	568	568
query77	257	274	208	208
query78	4097	3713	3241	3241
query79	2414	822	579	579
query80	1615	340	273	273
query81	489	158	132	132
query82	622	139	94	94
query83	307	214	192	192
query84	291	108	92	92
query85	797	351	299	299
query86	386	174	174	174
query87	1000	967	903	903
query88	2807	2124	2105	2105
query89	285	197	170	170
query90	1949	133	128	128
query91	132	123	108	108
query92	84	70	70	70
query93	1469	1152	716	716
query94	656	264	226	226
query95	536	251	307	251
query96	812	584	263	263
query97	1070	1073	1011	1011
query98	148	134	131	131
query99	420	346	306	306
Total cold run time: 177926 ms
Total hot run time: 82544 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.62 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit a6283f84be3356b7ce004e62f594f56fa2af3c2a, data reload: false

query1	0.01	0.00	0.01
query2	0.07	0.04	0.03
query3	0.25	0.12	0.11
query4	1.60	0.10	0.10
query5	0.18	0.16	0.15
query6	1.26	0.70	0.66
query7	0.03	0.00	0.00
query8	0.05	0.03	0.04
query9	0.27	0.22	0.21
query10	0.34	0.34	0.34
query11	0.17	0.11	0.12
query12	0.14	0.12	0.12
query13	0.31	0.31	0.31
query14	0.46	0.45	0.46
query15	0.36	0.37	0.35
query16	0.23	0.24	0.22
query17	0.71	0.74	0.70
query18	0.18	0.17	0.17
query19	1.15	1.21	1.08
query20	0.02	0.01	0.01
query21	15.46	0.17	0.12
query22	5.05	0.04	0.05
query23	16.23	0.26	0.10
query24	3.00	0.31	0.27
query25	0.10	0.04	0.04
query26	0.84	0.18	0.12
query27	0.04	0.04	0.02
query28	3.67	0.52	0.29
query29	12.73	3.13	2.54
query30	0.25	0.12	0.12
query31	2.75	0.37	0.17
query32	3.53	0.31	0.23
query33	1.36	1.42	1.55
query34	15.36	2.23	1.77
query35	1.75	1.70	1.73
query36	0.46	0.29	0.29
query37	0.06	0.04	0.04
query38	0.05	0.03	0.03
query39	0.03	0.03	0.02
query40	0.12	0.07	0.08
query41	0.07	0.03	0.02
query42	0.04	0.02	0.02
query43	0.04	0.03	0.03
Total cold run time: 90.78 s
Total hot run time: 14.62 s

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SubqueryExpr exclusions 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_37 through select_45 are consistent with their SQL. The new JUnit cases are deterministic but do not exercise the full rewrite/validation pipeline. The negative regression sends SET; SELECT as 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.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)), and Qualify(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.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

1 similar comment
@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anchor correction for review 5040700338. This resubmits the single accepted inline issue on the opening line of the overbroad condition; the comprehensive checkpoint summary and review conclusion remain in the original review.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_45 does 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.txt supplied 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.

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 65.85% (135/205) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 16782 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 9c0fb5c5725d1852091b8b15996496752a87ba2d, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17556	3032	3012	3012
q2	2116	260	231	231
q3	10209	924	503	503
q4	4667	245	200	200
q5	7684	584	381	381
q6	134	115	92	92
q7	521	497	399	399
q8	9258	903	883	883
q9	3457	2418	2396	2396
q10	6524	852	726	726
q11	404	194	174	174
q12	603	258	200	200
q13	18139	1512	1160	1160
q14	157	149	139	139
q15	q16	432	394	369	369
q17	1380	877	728	728
q18	3138	2254	2242	2242
q19	1125	890	805	805
q20	363	293	218	218
q21	4824	1692	1794	1692
q22	321	273	232	232
Total cold run time: 93012 ms
Total hot run time: 16782 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3353	3334	3294	3294
q2	488	389	369	369
q3	2208	2348	2199	2199
q4	1184	1155	881	881
q5	2173	2097	2099	2097
q6	164	120	86	86
q7	994	952	837	837
q8	1584	1381	1397	1381
q9	3103	3094	3063	3063
q10	1849	1799	1632	1632
q11	356	265	252	252
q12	442	431	338	338
q13	1476	1515	1147	1147
q14	170	177	163	163
q15	q16	388	399	354	354
q17	3541	3345	3303	3303
q18	4769	4414	4703	4414
q19	880	787	800	787
q20	1171	984	832	832
q21	3829	3110	3291	3110
q22	384	335	314	314
Total cold run time: 34506 ms
Total hot run time: 30853 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 81619 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 9c0fb5c5725d1852091b8b15996496752a87ba2d, data reload: false

query5	4249	406	331	331
query6	377	132	125	125
query7	4973	391	224	224
query8	284	122	120	120
query9	8684	2878	2890	2878
query10	387	219	182	182
query11	5366	1048	912	912
query12	111	69	73	69
query13	1207	462	355	355
query14	6137	2185	2104	2104
query14_1	1964	1952	1994	1952
query15	178	120	119	119
query16	931	355	353	353
query17	790	465	366	366
query18	2331	332	245	245
query19	170	141	113	113
query20	73	72	68	68
query21	198	101	88	88
query22	5464	5254	5363	5254
query23	6746	6322	6015	6015
query23_1	6193	6217	5944	5944
query24	7319	1126	792	792
query24_1	773	784	782	782
query25	428	306	258	258
query26	1219	221	131	131
query27	2805	420	260	260
query28	4714	1492	1488	1488
query29	929	449	369	369
query30	251	156	130	130
query31	835	399	332	332
query32	124	78	78	78
query33	480	215	201	201
query34	986	834	478	478
query35	407	403	342	342
query36	564	549	523	523
query37	123	82	73	73
query38	994	845	821	821
query39	483	499	467	467
query39_1	468	479	481	479
query40	204	92	80	80
query41	56	56	55	55
query42	78	71	73	71
query43	236	239	216	216
query44	1016	575	554	554
query45	110	110	96	96
query46	793	858	513	513
query47	738	767	719	719
query48	324	287	226	226
query49	531	239	189	189
query50	760	271	205	205
query51	7993	8000	7993	7993
query52	66	71	57	57
query53	204	191	152	152
query54	229	215	150	150
query55	79	59	53	53
query56	185	168	169	168
query57	726	687	641	641
query58	197	167	162	162
query59	1211	1201	1078	1078
query60	240	184	191	184
query61	130	128	113	113
query62	358	225	171	171
query63	164	143	141	141
query64	2771	746	544	544
query65	1636	1597	1562	1562
query66	1923	254	216	216
query67	9645	9706	9619	9619
query68	2990	1187	700	700
query69	332	216	199	199
query70	662	620	619	619
query71	244	173	159	159
query72	2303	1801	1552	1552
query73	628	547	361	361
query74	1995	1214	1118	1118
query75	1183	1108	969	969
query76	2356	733	536	536
query77	258	257	209	209
query78	3906	3727	3193	3193
query79	2746	836	589	589
query80	1558	315	273	273
query81	508	153	131	131
query82	627	126	94	94
query83	284	206	192	192
query84	293	112	90	90
query85	842	375	299	299
query86	477	197	175	175
query87	1007	972	882	882
query88	2862	2103	2105	2103
query89	281	191	175	175
query90	2021	127	124	124
query91	132	118	99	99
query92	86	66	70	66
query93	1701	1187	641	641
query94	624	248	203	203
query95	524	250	297	250
query96	819	568	263	263
query97	1108	1099	1010	1010
query98	171	137	133	133
query99	419	341	305	305
Total cold run time: 178682 ms
Total hot run time: 81619 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.55 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 9c0fb5c5725d1852091b8b15996496752a87ba2d, data reload: false

query1	0.00	0.01	0.00
query2	0.07	0.04	0.03
query3	0.26	0.11	0.09
query4	1.60	0.09	0.09
query5	0.17	0.15	0.15
query6	1.26	0.70	0.70
query7	0.03	0.01	0.00
query8	0.04	0.03	0.03
query9	0.29	0.22	0.21
query10	0.36	0.35	0.36
query11	0.16	0.10	0.11
query12	0.14	0.12	0.11
query13	0.30	0.30	0.31
query14	0.46	0.45	0.45
query15	0.38	0.34	0.35
query16	0.24	0.23	0.23
query17	0.68	0.68	0.72
query18	0.18	0.17	0.16
query19	1.13	1.11	1.15
query20	0.02	0.01	0.01
query21	15.45	0.17	0.11
query22	5.02	0.04	0.04
query23	16.19	0.24	0.10
query24	2.99	0.32	0.26
query25	0.12	0.04	0.03
query26	0.76	0.17	0.12
query27	0.03	0.03	0.03
query28	3.62	0.56	0.26
query29	12.47	3.22	2.59
query30	0.26	0.11	0.13
query31	2.75	0.38	0.17
query32	3.52	0.31	0.23
query33	1.45	1.38	1.46
query34	15.37	2.16	1.78
query35	1.74	1.76	1.70
query36	0.45	0.30	0.28
query37	0.06	0.04	0.04
query38	0.04	0.03	0.02
query39	0.03	0.03	0.03
query40	0.12	0.09	0.07
query41	0.08	0.02	0.03
query42	0.04	0.02	0.02
query43	0.03	0.03	0.02
Total cold run time: 90.36 s
Total hot run time: 14.55 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 31.52% (180/571) 🎉
Increment coverage report
Complete coverage report

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

…ter GROUP BY / over project

fix comments

fix comments

fix

fix comment

fix comment

fix comment

fix comment

fix compile

fix compile
@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 16862 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 92a31fa1e716701b7a7e568d2dd632ee1f618143, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17627	3047	3041	3041
q2	2145	255	258	255
q3	10163	864	515	515
q4	4677	252	200	200
q5	7677	564	377	377
q6	137	113	93	93
q7	522	510	396	396
q8	9243	890	886	886
q9	3422	2415	2385	2385
q10	6496	877	719	719
q11	390	196	181	181
q12	605	257	193	193
q13	18176	1537	1150	1150
q14	156	159	145	145
q15	q16	439	390	380	380
q17	1332	925	840	840
q18	3084	2250	2255	2250
q19	1264	895	782	782
q20	391	294	199	199
q21	5649	1639	1866	1639
q22	330	264	236	236
Total cold run time: 93925 ms
Total hot run time: 16862 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3407	3346	3325	3325
q2	508	401	366	366
q3	2200	2246	2169	2169
q4	1174	1151	883	883
q5	2167	2074	2105	2074
q6	167	122	87	87
q7	1082	908	905	905
q8	1582	1393	1404	1393
q9	3133	3093	3085	3085
q10	1867	1789	1605	1605
q11	355	269	247	247
q12	452	425	350	350
q13	1468	1559	1165	1165
q14	169	162	168	162
q15	q16	387	395	357	357
q17	3563	3273	3205	3205
q18	4774	4368	4693	4368
q19	842	766	913	766
q20	1045	985	836	836
q21	3799	3092	3310	3092
q22	389	329	341	329
Total cold run time: 34530 ms
Total hot run time: 30769 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 81961 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 92a31fa1e716701b7a7e568d2dd632ee1f618143, data reload: false

query5	4260	400	340	340
query6	387	132	142	132
query7	4944	426	232	232
query8	288	128	113	113
query9	8698	2887	2888	2887
query10	383	206	181	181
query11	5374	1033	911	911
query12	116	68	75	68
query13	1224	431	329	329
query14	6052	2217	2093	2093
query14_1	2008	1956	1964	1956
query15	183	121	112	112
query16	927	386	364	364
query17	809	465	375	375
query18	2339	337	240	240
query19	163	144	111	111
query20	70	73	70	70
query21	200	103	85	85
query22	5334	5401	5481	5401
query23	6746	6275	6059	6059
query23_1	6162	6104	6122	6104
query24	7347	1083	789	789
query24_1	740	749	786	749
query25	451	301	264	264
query26	1230	231	131	131
query27	2775	422	256	256
query28	4696	1512	1505	1505
query29	946	462	350	350
query30	253	153	131	131
query31	811	400	325	325
query32	127	78	78	78
query33	462	218	187	187
query34	990	825	488	488
query35	405	402	346	346
query36	588	589	518	518
query37	120	81	73	73
query38	1013	842	794	794
query39	494	491	467	467
query39_1	457	466	473	466
query40	208	99	82	82
query41	58	55	55	55
query42	75	74	72	72
query43	245	237	211	211
query44	1028	543	560	543
query45	140	104	99	99
query46	770	866	527	527
query47	756	774	710	710
query48	321	310	223	223
query49	551	239	179	179
query50	741	263	190	190
query51	8262	8109	7918	7918
query52	67	67	58	58
query53	189	203	158	158
query54	231	163	167	163
query55	81	71	56	56
query56	190	191	150	150
query57	661	658	637	637
query58	199	164	156	156
query59	1208	1221	1106	1106
query60	237	184	182	182
query61	118	115	119	115
query62	351	199	171	171
query63	170	139	143	139
query64	2666	715	566	566
query65	1609	1612	1614	1612
query66	1784	259	201	201
query67	9770	9889	9506	9506
query68	2879	1236	751	751
query69	341	224	196	196
query70	679	601	609	601
query71	260	175	155	155
query72	2331	1748	1557	1557
query73	683	595	344	344
query74	1975	1226	1149	1149
query75	1165	1095	967	967
query76	2296	728	542	542
query77	251	259	217	217
query78	4043	3779	3226	3226
query79	2321	849	593	593
query80	1624	313	284	284
query81	505	156	132	132
query82	693	119	94	94
query83	286	215	198	198
query84	294	116	91	91
query85	790	377	293	293
query86	400	175	173	173
query87	1012	977	904	904
query88	2770	2103	2114	2103
query89	287	201	174	174
query90	1967	131	126	126
query91	128	119	97	97
query92	78	69	71	69
query93	1490	1081	719	719
query94	655	257	220	220
query95	528	263	310	263
query96	806	580	269	269
query97	1052	1101	1001	1001
query98	171	143	130	130
query99	417	346	305	305
Total cold run time: 177852 ms
Total hot run time: 81961 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.65 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 92a31fa1e716701b7a7e568d2dd632ee1f618143, data reload: false

query1	0.00	0.00	0.00
query2	0.08	0.04	0.03
query3	0.24	0.10	0.10
query4	1.60	0.10	0.10
query5	0.17	0.15	0.15
query6	1.24	0.70	0.71
query7	0.03	0.00	0.00
query8	0.05	0.04	0.03
query9	0.28	0.21	0.22
query10	0.37	0.36	0.34
query11	0.18	0.12	0.12
query12	0.16	0.12	0.12
query13	0.31	0.30	0.31
query14	0.45	0.45	0.45
query15	0.37	0.35	0.35
query16	0.21	0.23	0.22
query17	0.72	0.64	0.68
query18	0.19	0.17	0.17
query19	1.20	1.10	1.20
query20	0.02	0.01	0.01
query21	15.44	0.15	0.11
query22	5.11	0.05	0.04
query23	16.18	0.26	0.10
query24	3.02	0.32	0.26
query25	0.10	0.05	0.04
query26	0.78	0.16	0.13
query27	0.04	0.03	0.02
query28	3.66	0.53	0.28
query29	12.46	3.14	2.57
query30	0.25	0.11	0.12
query31	2.76	0.36	0.18
query32	3.54	0.32	0.23
query33	1.33	1.43	1.49
query34	15.37	2.16	1.78
query35	1.77	1.73	1.74
query36	0.46	0.29	0.28
query37	0.06	0.04	0.04
query38	0.04	0.03	0.02
query39	0.02	0.02	0.02
query40	0.11	0.08	0.07
query41	0.08	0.03	0.02
query42	0.04	0.02	0.02
query43	0.03	0.03	0.03
Total cold run time: 90.52 s
Total hot run time: 14.65 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 48.52% (180/371) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 3.08% (180/5841) 🎉
Increment coverage report
Complete coverage report

1 similar comment
@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 3.08% (180/5841) 🎉
Increment coverage report
Complete coverage report

@starocean999
starocean999 marked this pull request as ready for review September 10, 2026 01:38
@morrySnow morrySnow closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants