[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 1 commit 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.
| logicalQualify(logicalHaving(aggregate())).then(qualify -> { | ||
| logicalQualify(logicalHaving(aggregate())).thenApply(ctx -> { | ||
| LogicalQualify<LogicalHaving<Aggregate<Plan>>> qualify = ctx.root; | ||
| checkWindow(qualify); |
There was a problem hiding this comment.
[P1] Preserve both correlated filters across the window project
The new outer-scope resolver makes this branch accept a correlated QUALIFY, but the legal case with a separately correlated HAVING still fails. For EXISTS (SELECT i.k FROM i GROUP BY i.k HAVING o.h=1 QUALIFY row_number() OVER (ORDER BY i.k)=1 AND o.q=1), normalization produces this reduced tree:
Filter[QUALIFY o.q]
Project[window]
Filter[HAVING o.h]
Aggregate[i.k]
Subquery unnesting runs before window extraction, and filter pushdown refuses a project containing a window expression. UnCorrelatedApplyFilter therefore records only o.q in the APPLY; ExistsApplyToJoin converts with that one predicate while o.h remains in the right subtree, which final slot validation rejects. Please decorrelate/conjoin both predicates across the window project (or reject the unsupported shape during analysis) and add the missing Qualify(Having(Aggregate)) regression.
| Optional<Scope> outerScope, PlanGenerator planGenerator) { | ||
| Set<Slot> projectOutputSet = project.getOutputSet(); | ||
| List<NamedExpression> newOutputSlots = Lists.newArrayList(); | ||
| Set<Expression> newConjuncts = new LinkedHashSet<>(); |
There was a problem hiding this comment.
[P1] Resolve correlated producers hidden by SELECT aliases
This check misses an outer dependency hidden behind a project alias. For EXISTS (SELECT i.k, o.flag AS f, row_number() OVER (ORDER BY i.k) AS rn FROM i QUALIFY rn=1 AND f=1), QUALIFY is bound to project-local f/rn, so both inputs are already in projectOutputSet and createPlan returns without exposing producer o.flag. Because the project contains a window expression, filter pushdown cannot rewrite f back to its producer before APPLY decorrelation. The filter therefore does not intersect correlation slot o.flag, and EXISTS takes the uncorrelated path although the right project still reads the outer slot; final validation rejects that plan. Please preserve or resolve alias-producer dependencies for correlation extraction and cover this alias form with a regression.
…ter GROUP BY / over project
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.
| Aggregate<Plan> agg = qualify.child(); | ||
| Resolver resolver = new Resolver(agg); | ||
| Resolver resolver = new Resolver(agg, ctx.cascadesContext.getOuterScope()); | ||
| qualify.getConjuncts().forEach(expr -> resolver.resolve(expr, ResolvePlanType.QUALIFY)); |
There was a problem hiding this comment.
[P1] Resolve grouped output aliases to their outer producer
Passing outerScope to Resolver does not expose an outer dependency hidden by an aggregate-output alias. A reduced legal EXISTS subquery is:
Qualify[f = 1, rn = 1]
Aggregate[groupBy=i.k; output=i.k, o.flag AS f, window AS rn]
Scan(i)
Resolver.lookUp(f) finds the local output slot and records only f -> f; it adds no aggregate output and this rule returns unchanged. NormalizeAggregate later encounters o.flag in the output project without a child/group producer, so analysis/final slot validation fails. The added tests cover a direct outer slot with GROUP BY and an alias without GROUP BY, but not this cross-product. Please resolve a safe outer-only aggregate-output alias to its producer while preserving valid aggregate outputs, or reject this shape explicitly, and add a complete-pipeline grouped-alias regression.
| if (inputSlots.isEmpty()) { | ||
| newHavingConjuncts.add(conjunct); | ||
| } else if (correlatedSlots.containsAll(inputSlots)) { | ||
| // the predicate only depends on the outer row, so it can safely be |
There was a problem hiding this comment.
[P1] Do not infer HAVING/window commutativity from visible input slots
This containment check is not proof that a conjunct is constant over aggregate rows or safe to move across window evaluation. For example:
Qualify[rn = 1]
Project[rn := row_number(order by i.k desc)]
Having[count(*) = o.h]
Aggregate[groupBy=i.k, count(*)]
count(*) = o.h reports only {o.h} as input slots, so the patch ranks every group before applying the predicate. With o.h=1, a one-row k=1 group and a two-row k=2 group, correct HAVING-first execution keeps k=1 and numbers it 1; the rewrite numbers k=2 first and returns no row. SubqueryExpr similarly hides a current-group correlation from getInputSlots(), and even a pure false outer predicate may be required to suppress an error-producing window key. Preserve HAVING below the window while making it decorrelatable, or conservatively reject shapes whose complete dependencies, determinism, and evaluation domain are not proven; add aggregate, nested-subquery, and error-gating regressions.
| .flatMap(Set::stream) | ||
| .filter(s -> !projects.contains(s)) | ||
| .filter(s -> !(outerScope.isPresent() | ||
| && outerScope.get().getCorrelatedSlots().contains(s))) |
There was a problem hiding this comment.
[P1] Keep both DISTINCT correlations reachable by the enclosing Apply
For a DISTINCT subquery with separately correlated HAVING and QUALIFY predicates, this branch builds the reduced tree:
Having[o.h]
ProjectDistinct[i.k]
Qualify[o.flag, rn]
Project[i.k, rn := window]
Scan(i)
Distinct conversion plus NormalizeAggregate inserts a Project -> Aggregate barrier between the two filters. UnCorrelatedApplyFilter first records o.h; the Apply's alreadyExecutedEliminateFilter fence then prevents pulling through the normalization project, and no aggregate-filter rule reaches the lower o.flag. Apply-to-Join therefore uses only o.h, leaving o.flag dangling in the right subtree. This is the DISTINCT/project parallel shape, not the existing Qualify(Having(Aggregate)) thread. Keep the correlated predicates on the same decorrelatable side of the distinct/window barriers, or reject the shape, and add the missing full-pipeline regression.
| Expression producer = entry.getValue(); | ||
| if (!producer.getInputSlots().isEmpty() | ||
| && correlatedSlots.containsAll(producer.getInputSlots())) { | ||
| correlatedAliasToProducer.put(entry.getKey(), producer); |
There was a problem hiding this comment.
[P1] Exclude window producers from this alias replacement cycle
A project alias can itself be a window over only outer slots:
Qualify[rn = 1]
Project[row_number() over(order by o.flag) AS rn]
Scan(i)
The map replaces rn with the window expression, then visitWindow immediately creates a fresh alias w1. The generated child is another Qualify(Project) whose w1 maps to the same window; the top-down analysis job revisits it, creates w2, and repeats without a fixed point before normalization or Apply unnesting. Restrict this repair to a producer class that cannot re-enter window extraction (or make the transformation explicitly idempotent), and add a full analyzer regression with a window partition/order key that is entirely correlated.
| if (!producer.getInputSlots().isEmpty() | ||
| && correlatedSlots.containsAll(producer.getInputSlots())) { | ||
| correlatedAliasToProducer.put(entry.getKey(), producer); | ||
| } |
There was a problem hiding this comment.
[P1] Do not copy a producer subquery into both plan nodes
A producer such as o.flag + (SELECT max(j.k) FROM j) has visible input slots {o.flag}, so it enters this map even though it also contains a SubqueryExpr. Replacement leaves the same subquery in the lower project and copied upper QUALIFY expression:
Qualify[o.flag + scalarSubquery > 0]
Project[o.flag + scalarSubquery AS f, window AS rn]
Bottom-up SubqueryToApply unnests the lower occurrence and marks that subquery analyzed. The upper occurrence is then replaced with the same output slot, but creation of its Apply is skipped by the global analyzed-expression fence; the intervening project does not output that slot, so final validation rejects the filter. Exclude producers containing SubqueryExpr from this substitution or materialize/unnest the producer exactly once at a shared level, with scalar and EXISTS producer regressions through CheckAfterRewrite.
| boolean conjunctsRewritten = false; | ||
| if (!correlatedAliasToProducer.isEmpty()) { | ||
| Set<Expression> rewrittenConjuncts = ExpressionUtils.replace(conjuncts, correlatedAliasToProducer); | ||
| conjunctsRewritten = !rewrittenConjuncts.equals(conjuncts); |
There was a problem hiding this comment.
[P1] Preserve slot ownership when replacement crosses IN boundaries
Replacing only the QUALIFY reference does not make the producer legal for every Apply caller. Two reduced failures remain:
Apply[IN, corr=o.flag]
Scan(o)
Project[o.flag AS f] -> Filter[o.flag=1, window]
Apply[EXISTS, corr=o.flag]
Scan(o)
Apply[IN, compare=o.flag](Project[f, window](Scan(i)), Scan(j))
In the first, IN/NOT IN must retain f as its build value; the IN-specific Project(Filter) fence preserves the right project, and InApplyToJoin leaves it consuming the left-only o.flag. In the second, ExpressionUtils.replace descends into InSubquery.child(), then SubqueryToApply removes that filter and stores o.flag only in the nested IN Apply's compareExpr; InApplyToJoin emits a nested join condition whose slot is produced by neither child. Both fail final slot validation (mark/OR has the same ownership issue). Reject unsupported outer-dependent IN outputs/nested compares, or materialize/pass the value at the correct Apply level, and add IN, NOT IN, nested-IN, and mark regressions.
| // union, so pushing the project into the union would leave a dangling slot reference. | ||
| for (NamedExpression ne : project.getProjects()) { | ||
| if (!union.getOutputSet().containsAll(ne.getInputSlots())) { | ||
| return false; |
There was a problem hiding this comment.
[P1] Enforce this ownership invariant in the earlier UNION rewrite
This late guard is bypassed by PushProjectThroughUnion, which runs before MergeOneRowRelationIntoUnion and this rule in both rewrite pipelines. For a width-matched project over a constant UNION ALL:
Project[i.k, o.flag AS f]
UnionAll[OneRow(...), OneRow(...)]
PushProjectThroughUnion.canPushProject admits the project from width and bare/cast-slot shape alone; its replacement leaves o.flag unmapped in each one-row child. ProjectProcessor merges those projects and MergeOneRowRelationIntoUnion converts them to arity-zero constant rows, so this new check never executes and final validation sees a dangling outer slot. Apply the same child-output containment invariant before PushProjectThroughUnion rewrites the children (including the CTE registration), and add an end-to-end constant-UNION regression whose normalized project width matches the UNION width.
TPC-H: Total hot run time: 16851 ms |
TPC-DS: Total hot run time: 82544 ms |
ClickBench: Total hot run time: 14.62 s |
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)