[fix](subquery) Resolve correlated qualified columns before dereference - #67438
[fix](subquery) Resolve correlated qualified columns before dereference#67438CalvinKirs wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review of PR #67438 at head 3e6ad0f found two blocking correctness issues, recorded inline below.
Review checkpoints:
- Goal: relation-qualified correlated references should win over inner column-field dereference. The direct scalar and variant conflicts are implemented and covered by deterministic FE/regression tests, but the new lookup is too broad and breaks lambda lexical scoping and nested outer-field correlation metadata.
- Data correctness: not satisfied until MF-1 and MF-2 are fixed; both can change query results or produce an invalid uncorrelated Apply.
- Scope/concurrency/lifecycle: no thread or lifecycle changes; the relevant scope hierarchy is covered by the inline findings.
- Compatibility/configuration/protocol: no configuration, persistence, or FE-BE protocol changes identified.
- Error handling/observability: no new ignored Status or RPC boundary; the main concern is silent misbinding.
- Tests: added tests follow ordering and setup conventions and exercise the intended direct conflicts, but do not cover lambda-argument shadowing or nested outer fields.
- User focus: review_focus.txt contains no additional focus.
No builds or tests were run because the review prompt explicitly forbids builds/code modifications in this runner.
3e6ad0f to
3a350a0
Compare
|
/review |
There was a problem hiding this comment.
Review status: complete after 2 rounds. All round-2 normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS; one distinct correctness issue remains and is requested inline.
Critical checkpoint conclusions:
- Goal and data correctness: The ordinary correlated-filter conflict, lambda lexical-precedence case, and nested correlation-slot propagation are addressed, but the goal is incomplete for custom HAVING/QUALIFY binders because an outer relation can preempt a valid inner binding.
- Scope and clarity: The production change is localized and the relation-only helper is clear, but the generic prepass assumes
getScope()represents every analyzer's complete local lookup policy; that assumption is false for custom binders. - Concurrency: Not involved; analysis uses per-query scopes and this change adds no shared concurrent state or locking.
- Lifecycle/static initialization: Not involved; no new static/global lifetime or ownership behavior.
- Configuration: No configuration item is added or changed.
- Compatibility: No FE-BE protocol, storage-format, persisted-metadata, symbol, or rolling-upgrade surface changes.
- Parallel paths: Blocking issue found. HAVING, QUALIFY, and related custom secondary-scope paths are not correctly covered by the new ordering.
- Special conditions: The lambda-specific
shouldPrioritizeRelationQualifier=falseguard is justified and fixes the prior lexical-scope thread, but equivalent complete-local-scope handling is missing for custom binders. - Tests: The five targeted FE tests are reported passing by the author, and the added unit/regression cases distinguish scalar conflict, complex-column conflict, lambda binding, and nested correlation. The regression suite was not run by the author, and there is no reused-alias HAVING/QUALIFY case for the blocking path. Per the review-runner instruction, this review did not build or modify source.
- Test results: The added deterministic expectations and existing negative dereference case are consistent with the intended behavior; no incorrect expected result was found.
- Observability: No new runtime or distributed operational path; additional logs or metrics are not applicable.
- Transactions/persistence and data writes: Not involved.
- FE-BE variables: No variable or request-field propagation is added.
- Performance: The extra bounded scope lookups do not establish a distinct performance defect; no hot runtime path is changed.
- Other issues: The two existing inline reports (lambda precedence and nested correlation extraction) are verified fixed at head
3a350a08727a9b62b78470b056c71a9a6c5cf981; no duplicate was submitted. - User focus: No additional user-provided focus was specified; the full PR was reviewed.
|
The intended precedence is correct: for a multipart name such as The remaining issue is not that relation-qualified lookup is prioritized, but that the new prepass assumes That assumption does not hold for analyzers such as HAVING/QUALIFY, where I suggest making the relation-only phase follow the same analyzer-specific local lookup policy as the full binding phase, for example with a protected This keeps the compatibility fix while avoiding special-case regressions caused by bypassing custom local binders. Please also add a reused-alias HAVING/QUALIFY test so the scope contract is fixed by coverage rather than by the current implementation shape. |
3a350a0 to
902e8b9
Compare
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: In a correlated subquery, a multipart reference could be interpreted as a nested field of an inner column before checking whether its first part was an outer table alias. This caused scalar inner columns to raise a no-such-field error and complex inner columns to bind silently to the wrong expression. Resolve relation-qualified columns across the current and outer scopes before falling back to column-field dereference, while preserving lambda lexical scope precedence, custom analyzer local-scope precedence, and recording the input slots of correlated nested-field expressions.
### Release note
Fix incorrect Nereids column binding when an outer table alias conflicts with an inner column name in a correlated subquery.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.rules.analysis.TestDereference
- Added query_p0 regression coverage for scalar and complex inner columns, lambda lexical scope, nested correlated fields, and reused aliases in HAVING
- Behavior changed: Yes (relation-qualified correlated references now take priority over inner column-field dereference)
- Does this need documentation: No
902e8b9 to
9fdbf32
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review status: complete after 2 rounds. Both normal reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; two distinct issues remain, so I am requesting changes.
Critical checkpoint conclusions:
- Goal and data correctness: the direct correlated-filter conflict, lambda lexical precedence, nested correlation extraction, and custom HAVING/QUALIFY scope ordering are addressed, but qualifier occupancy is still lost when a nearer reused alias lacks the requested column. That can silently turn a local nested-field binding into an outer correlation.
- Scope and minimality: the production change is localized, and the custom binder callback plumbing is mechanically consistent. The remaining defect is in the relation-only lookup's inability to distinguish an absent qualifier from an occupied qualifier with no matching column.
- Parallel paths and special conditions: aggregate/generic HAVING, project/aggregate QUALIFY, GROUP BY, ORDER BY, lambda, and join rejection were audited. Their variants either behave consistently or reduce to the first inline finding; no separate issue remains.
- Concurrency, lifecycle, configuration, compatibility, persistence, transactions, data writes, and FE-BE propagation: not involved in this analyzer-only change.
- Error behavior: the first finding can replace a local bind/error path with a valid correlated plan, so failure semantics are not preserved.
- Tests and results: the added unit expectations and deterministic row values are semantically coherent, and the author reports
TestDereferencepassing 7/7. This review did not run builds or tests because the review prompt prohibits them. Live compile and CheckStyle pass; FE UT is pending, and the regression jobs currently report immediate TeamCity failures. Coverage still lacks the qualifier-present/column-absent collision, and the six new success cases do not follow the required generated-snapshot convention. - Performance and observability: the extra analyzer lookups do not establish a material performance issue; runtime observability is not applicable.
- Existing threads: lambda precedence, nested correlation bookkeeping, and the custom HAVING/QUALIFY omission were verified and treated as hard duplicate fences on this head.
- User focus: no additional review focus was provided, so the full PR was reviewed.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16701 ms |
FE Regression Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 81807 ms |
ClickBench: Total hot run time: 14.87 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#67438 Problem Summary: Relation-only lookup in correlated subqueries did not distinguish an absent local qualifier from a local relation alias whose requested column was absent. That allowed an outer alias with the same name to win before local nested-field binding or local error handling. Propagate qualifier occupancy through normal and custom analyzer scopes, preserve same-scope nested-field fallback, and add unit and regression coverage for local columns, nested fields, and missing fields. ### Release note Fix incorrect correlated column binding when an inner relation alias shadows an outer alias. ### Check List (For Author) - Test: Regression test / Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.rules.analysis.TestDereference - ./run-regression-test.sh --run -d query_p0 -s test_dereference - Behavior changed: Yes (inner relation aliases now shadow outer aliases while same-scope nested-field fallback remains supported) - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review outcome: no new blocking issues found at PR head c649dac66a7478637b9678deab74b235fd16c34b.
The change meets its stated goal: relation-qualified references in correlated subqueries are resolved before nested STRUCT/MAP/VARIANT dereference, while local qualifier shadowing, custom HAVING/QUALIFY/GROUP BY/ORDER BY binders, and lambda lexical scope remain intact. The earlier review concerns in the existing threads (lambda precedence, nested-expression correlation bookkeeping, custom-scope precedence, local alias shadowing, and regression snapshot format) are addressed in this head and were treated as duplicate fences; no distinct residual issue was substantiated.
Critical checkpoints:
- Goal and behavior: the new relation-only phase and qualifier-occupancy gate preserve nearest-scope binding and valid outer correlation; the added plan assertions and generated regression cases cover the changed behavior.
- Scope/concurrency/lifecycle: this is per-query analyzer state; no new cross-thread lifecycle or lock interaction was introduced.
- Error handling: missing scalar/struct fields still fail during analysis; no status, RPC, transaction, persistence, or protocol paths were changed.
- Compatibility and parallel paths: normal and custom analyzer paths, nested expressions, aggregate scopes, and lambda analysis were reviewed; no configuration or storage-format change is involved.
- Tests and performance: unit coverage plus deterministic
order_qtsnapshots were inspected; no build or test command was run in accordance with the review-only prompt. The extra relation-only pass is limited to multipart names when an outer scope exists and reuses the existing matching helpers. - User focus:
review_focus.txtspecifies no additional focus, so the full PR was reviewed.
No inline comments are proposed; this is a summary-only review.
TPC-H: Total hot run time: 16702 ms |
TPC-DS: Total hot run time: 81967 ms |
ClickBench: Total hot run time: 14.66 s |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
In a correlated subquery, a multipart reference can represent either a relation-qualified column (
table_alias.column) or a nested-field dereference (column.field). Nereids previously searched the inner scope completely before checking the outer scope. If an inner table had a physical column with the same name as an outer table alias, the outer reference could therefore be interpreted as a nested field of that inner column.For scalar inner columns this raised a
No such fieldanalysis error. For complex inner columns it could bind successfully to the wrong expression and produce incorrect results.Example
Before this fix, the outer alias
t1conflicted with the physical inner columninner_events.t1. Nereids treated ``t1.`@event_name``` as a nested-field access on the inner scalar column and failed during analysis:After this fix, ``t1.`@event_name``` is correctly bound to the outer relation alias and the query returns:
This change resolves relation-qualified columns in the current and outer scopes before falling back to first-part-as-column dereference. The relation-only phase follows each analyzer's complete local scope order before searching the outer scope, so custom HAVING and QUALIFY scopes preserve normal nearest-relation shadowing.
Lambda lexical scope is preserved: in
array_map(x -> x.value, x.items), the firstxinside the lambda resolves to the lambda argument while the secondxresolves to the enclosing table alias. When an outer qualified reference contains nested fields, such asouter_alias.payload.k, the underlyingpayloadslot is also recorded as a correlated slot.Release note
Fix incorrect Nereids column binding when an outer table alias conflicts with an inner column name in a correlated subquery.
Check List (For Author)
./run-fe-ut.sh --run org.apache.doris.nereids.rules.analysis.TestDereferencequery_p0/test_dereferenceregression coverage for scalar and complex inner columns, lambda lexical scope, outer nested-field correlation, reused inner/outer aliases in HAVING and QUALIFY, and local qualifier shadowing./run-regression-test.sh --run -d query_p0 -s test_dereference