[fix](fe) Fix NPE when materialized view aggregate rewrite treats a derived group by projection as a group by key - #67362
[fix](fe) Fix NPE when materialized view aggregate rewrite treats a derived group by projection as a group by key#67362starocean999 wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Review completed on head 3aa9795ba081dd30bc5b4160e0ae0986b6964474 after two convergence rounds. Two required fixes are called out inline.
Critical checkpoint conclusions:
- Goal/correctness: rebuilding from the normalized bottom aggregate fixes the reported derived-projection NPE for the covered plan, but the project-elision gate still discards valid leading-subset sync/async MV rewrites.
- Scope: the change is localized and focused.
- Concurrency/lifecycle: no threads, locks, shared mutable state, static initialization, or managed-resource lifecycle changes.
- Configuration/compatibility/persistence: no new config, FE-BE protocol, storage-format, transaction, or persistence boundary.
- Parallel paths: aggregate-on-none, project/filter, Limit/TopN, HAVING, and sourceRepeat paths were traced; NormalizeRepeat safely reconstructs GROUPING SETS/ROLLUP/CUBE keys and grouping-id state.
- Tests: the helper proves MV selection and ordered result execution for the reported shape, but the TIMESTAMPTZ golden is not portable without a pinned session time zone, and the leading-subset shape is uncovered. CI observed on this head proves style/static gates only; no regression execution check is reported, and this review ran no build/tests per the runner instructions.
- Error handling/observability: existing MV fail-reason reporting remains intact; no new runtime observability is needed.
- Performance: no material new planner complexity was found, but the leading-subset bug forfeits valid MV acceleration by falling back to the base plan.
- User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
|
/review |
|
run builall |
There was a problem hiding this comment.
Review completed on head dad90ff1b96d4ac17d1e8892dd6081cf0ce9cc7b after two convergence rounds. I am requesting changes for two distinct issues: a duplicate-output identity failure that skips the mandatory whole-tree rewrite, and avoidable repeated lineage traversal on the common non-Repeat path.
Critical checkpoint conclusions:
- Goal and correctness: the current head fixes the reported derived-group projection failure and the previously reported strict-leading-subset boundary, but separately aliased equal outputs are rebuilt with one ExprId and can bypass aggregate normalization and partition pruning.
- Scope and minimality: the functional change is localized to aggregate MV plan reconstruction, but the new Project-over-Aggregate shape must preserve per-position output identity and avoid unnecessary plan-wide work.
- Conditions and parallel paths:
sourceRepeatandneedTopProjectconditions were traced through ordinary roll-up, aggregate-on-detail/direct-all, Project/Filter/HAVING shapes, Repeat/grouping sets, sync/async partition union, whole-tree rewriting, project merging, and inherited Limit/TopN validation. No distinct issue remains beyond the two inline findings. - Concurrency, lifecycle, and configuration: no threads, locks, shared mutable lifecycle, static initialization, or product configuration change is involved; the session variables are test-only controls.
- Compatibility, persistence, transactions, writes, and FE/BE boundaries: no protocol, storage format, journal, transaction, data-write, or FE-to-BE variable contract changes are present.
- Tests: the new suite pins the time zone, asserts the exact MV is chosen, orders results, and covers the intended derived-projection and strict-subset fixes. It does not cover the duplicate-output failure with an observable skipped-rewrite consequence. This review ran no build or tests per the review-runner instructions; current CI shows style/static checks passing while build/test jobs are skipped.
- Error handling and observability: existing MV failure-reason reporting is adequate; no new runtime observability is needed.
- Performance: the common non-Repeat branch retains an unused per-output classification walk and adds repeated bottom/top lineage shuttles for every candidate and relation mapping.
- User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
Review status: complete and converged, with changes requested for the two inline findings.
dad90ff to
3a7068c
Compare
|
/review |
There was a problem hiding this comment.
Request changes: one output-identity boundary remains.
Critical checkpoint conclusions:
- Goal and correctness: rebuilding the rewritten aggregate from the normalized bottom aggregate and recomputing derived outputs in a top project addresses the reported derived-group projection failure. The strict-prefix, time-zone, separately aliased duplicate-output, and added-lineage-cost concerns from existing threads are addressed on this head. The repeated-bare-output case in the inline comment still violates the downstream output-set invariant.
- Scope and clarity: the production change is focused in the shared aggregate-MV rewrite helper, with a targeted regression suite.
- Concurrency and lifecycle: no concurrency, locking, static initialization, or non-intuitive resource lifecycle is introduced.
- Configuration and compatibility: no production configuration, FE-BE protocol, persisted metadata, storage format, or rolling-upgrade contract changes. The test-only session settings are scoped to exercising the intended rule and deterministic time-zone rendering.
- Parallel and conditional paths: ordinary sync/async aggregate rewrite, aggregate-on-detail, Repeat/grouping sets, output normalization, whole-tree RBO, partition pruning, and union compensation were traced. Repeat remains behaviorally unchanged apart from lazy classification; the accepted issue is in the shared non-Repeat output fan-out.
- Tests and results: the new suite asserts the intended MV is chosen and checks deterministic results for the original derived projection, strict-prefix outputs, and separately aliased duplicate aggregates/groups. It does not cover repeated unaliased output slots, which is the missed boundary. No local build or regression run was performed because the review runner forbids builds; current GitHub CheckStyle/license/hygiene checks pass, while FE build and regression jobs are skipped on this draft.
- Observability, transactions, persistence, and data writes: not applicable to this planner-only change; no new observability is needed.
- Performance: batching the non-Repeat lineage walks removes the PR-added repeated traversals. The residual per-aggregate roll-up lineage walk is already covered by existing thread 3905142846 and is not duplicated here.
- User focus: no additional focus was provided; the full PR was reviewed.
The review converged after two rounds of normal and risk-focused passes. One distinct inline issue remains.
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review conclusion for head 8779c9bfd87672c28f296044466c72436080e1e3: no distinct new inline findings.
The current code addresses the stated failure. It rebuilds the non-Repeat aggregate from the normalized bottom aggregate keys/functions, then recomputes derived query outputs in a project above that aggregate. The new originalExprIdToRewritten mapping preserves the original output ExprId equivalence classes: repeated bare outputs reuse one rewritten identity, separately aliased duplicates remain distinct, and reordered or strict-subset outputs force the required top project. The resulting project expressions depend only on outputs produced by the rebuilt aggregate.
Critical checkpoint conclusions:
- Goal and proof: the derived-group-key projection no longer becomes an aggregate key or leaves a dangling output. The regression covers the original derived projection, a leading output subset, separately aliased duplicate aggregate/group outputs, and repeated bare output; ordering and time-zone output are deterministic.
- Scope: the implementation is confined to the shared aggregate-MV reconstruction plus its regression/golden result. The widened protected return type is compatible with the aggregate rule callers.
- Parallel paths and conditions: aggregate-on-aggregate ROLL_UP and aggregate-on-detail DIRECT_ALL were traced; both leave child-valid expressions. The Repeat/grouping-sets block is moved intact under the same predicate, including hidden-key compensation and grouping-id propagation. Limit/TopN and project/filter rule families continue to accept the returned Plan.
- Concurrency and lifecycle: all new collections and identities are query-local planner state. No locks, asynchronous state, static initialization, or special resource lifecycle is introduced.
- Configuration, compatibility, and persistence: no new configuration, FE/BE protocol, storage format, rolling-upgrade boundary, EditLog, transaction, or data-write behavior is involved.
- Error handling and observability: existing MV failure-reason recording and downstream output validation remain in place; no new user-facing error or observability requirement was identified.
- Performance: batched lineage expansion removes the former per-output top-level traversals. The remaining per-aggregate roll-up traversal is already covered by existing thread
r3905142846, so it was not duplicated here. - Test coverage: the added success/result cases are valid. The fact that the bare-duplicate case does not itself prove whole-tree normalization or partition pruning ran is already covered by existing threads
r3911463813andr3905142841; no duplicate inline comment was added. - User focus: no additional review focus was provided.
No builds or tests were run locally because the review-runner instructions prohibit build execution and source changes. All three required Round 1 scans returned NO_NEW_VALUABLE_FINDINGS; every candidate was independently validated, dismissed with code evidence, or deduplicated against the existing review threads. Review coverage is complete for this head.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Given a sync materialized view on
sync_tz_base:the following query fails during planning with an NPE:
Root cause
In
AbstractMaterializedViewAggregateRule.aggregateRewriteByView, the group by keys of the rewritten aggregate were collected from the query top plan output expressions. For aproject -> aggregatestructure,topPlanSplitToGroupAndFunctionclassifies the derived projectioncast(date_trunc(ts, 'day') AS STRING)as a group-by expression, because it is derived from the real group key. This derived projection was then wrongly added as a group by key of the rewritten aggregate:This led to two problems:
cast(day_ts#7 as TEXT)which is only a projection of the real group keyday_ts#7.day_ts#7was added by the group-by compensation logic but was not present in the aggregate output expressions, so the top project referenced a slot that the physical aggregate never produced, causing the"root" is nullNPE during physical plan translation.Fix
The rewritten aggregate is now built directly from the query bottom aggregate:
cast(date_trunc(ts, 'day') AS STRING)— are recomputed by aLogicalProjectabove the rewritten aggregate when they cannot be produced by the aggregate directly.After the fix the rewritten plan is valid and the query returns correct results:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)