[refactor](nereids)Remove defer materialize#62917
Conversation
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found a blocking compatibility regression in the session-variable migration. The PR removes the old defer-materialize TopN rule and documents enable_two_phase_read_opt as replaced by topn_lazy_materialization_threshold, but the old variable is still accepted and still used by existing code/tests to disable two-phase read. After this change, setting enable_two_phase_read_opt=false no longer prevents PlanPostProcessors from adding LazyMaterializeTopN because that path only checks topn_lazy_materialization_threshold > 0.
Critical checkpoint conclusions: goal is clear, but the migration does not fully preserve the old disable contract; change is mostly focused but misses compatibility wiring; no new concurrency/lifecycle/static-init concerns were found; this is a session configuration change and the old config no longer affects the replacement path; no storage-format or FE/BE protocol compatibility issue was found; the parallel old/new TopN lazy-materialization paths were considered and the removed path is replaced by the post-processor path; test coverage regresses because the old two-phase-read regression tests are deleted and no replacement covers enable_two_phase_read_opt=false; no result-file correctness issue found in the modified/deleted test files; no additional observability requirement; no transaction/persistence/data-write impact found; no new FE-BE variables added; performance impact is not the concern here, correctness/compatibility is.
User focus: no additional user-provided review focus was specified.
|
run buildall |
|
run external |
englefly
dismissed
github-actions[bot]’s stale review
the old session var is deprecated.
|
run buildall |
3 similar comments
|
run buildall |
|
run buildall |
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Remove the legacy DeferMaterializeTopN rewrite and implementation path while keeping enable_two_phase_read_opt itself. ### Release note None ### Check List (For Author) - Test: No need to test (waiting for manual compile/test confirmation) - Behavior changed: Yes (the legacy DeferMaterializeTopN path and its dedicated tests are removed) - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
run buildall |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29401 ms |
TPC-DS: Total hot run time: 169828 ms |
FE UT Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4 participants
What problem does this PR solve?
defer materialization is replaced by topn-lazy-materialization
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)