Repository navigation
branch-4.1: [fix](fe) Preserve CTE column order for repeated slots (branch-4.1) - #69676
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69388 Problem Summary: A CTE selecting a key, a payload and the same key exposes [x,z,y] instead of [x,y,z] because the reverse producer-to-consumer multimap groups repeated producer ExprIds. Derive output positions from the ordered consumer-to-producer map and preserve entry order through nullability and variant subpath rewrites. Carry only this independent CTE repair to 4.1; leave USING binding unchanged and use the branch-native FE result API. ### Release note Preserve CTE result column positions when a source column appears more than once in its output. ### Check List (For Author) - Test: Unit Test - Reproduced the original column-order failure before the repair. - 31 focused FE tests passed; retry the pruning fixture after a random HTTP-port collision at startup. - FE server build with DISABLE_BUILD_UI=ON, Checkstyle and diff checks. - No BE execution or full regression suite was run locally. - Behavior changed: Yes, repeated CTE columns retain their positions. - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
924060929
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed all five changed files at b72cb88. No actionable correctness issue found and no existing blocking inline findings to retain. Review converged in two rounds, including independent ordering/execution, nullable/test, and additional ordered-map/variant risk reviews.
The forward consumer map preserves x/y/z positions for producer outputs a/b/a. Nullability reconstruction retains each consumer identity, and variant expansion preserves original keys and relative order. Copies, pruning, expression/type rewrites, CTE inlining and materialized translation preserve this invariant; physical translation resolves producer/consumer ExprIds rather than pairing reverse-map values by position.
Critical checkpoint conclusions:
- Goal and proof: the added analysis test asserts names x/y/z and the FE-computed row (1,10,1); direct consumer tests cover repeated slots, copies, pruning and nullable propagation. These tests were inspected, not executed.
- Scope and clarity: three focused production changes with two test files; the partial backport stays within CTE ordering and leaves branch-native USING binding unchanged.
- Concurrency and locking: local map builders and immutable plan snapshots introduce no new shared state, threads, locks, expensive critical sections or lock-order change.
- Lifecycle and initialization: no new resources, callbacks, ownership cycles or static initialization dependencies; existing property-copy behavior remains consistent.
- Configuration: none added or altered.
- Compatibility: no public symbol, persisted/storage format or protocol changes requiring rolling-upgrade handling.
- Thrift/protobuf: no changed definitions; cross-release field/enum comparison is inapplicable.
- Parallel paths: inline/materialized consumers, deep copies, output pruning, expression/type replacement, nested pruning and optimizer-created CTEs were traced; all applicable forward-map rebuilds preserve order.
- Conditional checks: existing variant guards and nullable debug checks remain intact; no new special condition needs justification.
- Test coverage: focused JUnit5 assertions cover the reported scalar ordering trigger and plan transformations; existing analysis negatives and nested/variant fixtures were also inspected. No added regression directly covers BE-executed materialized repeated outputs or combined nested/repeated variant ordering.
- Expected results: x/y/z, the row (1,10,1), pruned y/z and nullable true/false/true agree with the SQL and fixtures. No golden output file changes.
- Observability and errors: no new distributed operation or failure path requires logs/metrics; existing invariant/error handling remains in place.
- Transactions and persistence: no EditLog, commit/publish, metadata persistence or failover path changed.
- Data writes and crash safety: no storage mutation, transaction atomicity or resource-acquisition path changed.
- FE/BE variables: none added; existing translation preserves producer/consumer identity and matching projection/tuple order.
- Performance: consumer reconstruction is linear and removes repeated reverse-group visits for duplicate producers; ordered-map overhead is bounded per entry.
- Other invariants: slot identity/metadata, property copies, map equality and generated subpath slots were checked without finding another defect. Committed-data visibility, versions, delete bitmaps and BE memory accounting are unaffected.
No additional user focus was supplied. No unresolved code question or uncovered changed-file scope remains. Validation limits: static source review only; no builds, tests, benchmarks or BE execution were run, and no submodules were initialized. The absent direct execution cases above limit validation breadth and are not evidence of a verified bug.
Review converged after 2 round(s).
FE Regression Coverage ReportIncrement line coverage |
|
run feut |
|
run cloud_p0 |
FE UT Coverage ReportIncrement line coverage |
cherry pick from #69388