[refactor](storage) move flexible partial-update fill into the block transform - #66774
Open
csun5285 wants to merge 2 commits into
Open
[refactor](storage) move flexible partial-update fill into the block transform#66774csun5285 wants to merge 2 commits into
csun5285 wants to merge 2 commits into
Conversation
… decouple BlockAggregator; segment writers become pure storage-format sinks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 14, 2026 08:14
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes
I found two blocking issues:
- A flexible sequence block can shrink to zero after its segment ID is allocated. Because neither flusher publishes that ID, a later segment leaves a permanent numbering gap; default metadata construction can then declare zero segments and readers omit valid later data.
- The existing horizontal-writer regression still requires the exact
NotSupportederror removed here, so the intended newly successful path fails that suite deterministically.
Checkpoint conclusions:
- Goal, scope, and tests: The storage-only refactor consistently moves flexible fill before writer selection and adds broad BE semantic/persistence coverage. The tests miss the zero-then-nonempty segment sequence, and the existing docker regression was not updated. I did not run local builds or tests because the review-runner instructions prohibit them.
- Correctness, persistence, and errors: Sequence/default/null/delete/skip-bitmap behavior and ordinary
Statuspropagation otherwise match the legacy vertical path. The unresolved zero-row success path breaks the rowset's physical segment/collector/metadata contract and can hide committed rows. - Concurrency and lifecycle: Per-flush transform state, atomic counters, delete-bitmap synchronization, historical-row pins, and RowStore/Variant COW generator lifetimes are sound outside the reported segment-publication boundary.
- Configuration, compatibility, and parallel paths: No new configuration, FE-BE variable, wire protocol, or storage format is introduced. Both writer modes and the V2, group/binlog, cloud, compaction, schema-change, transient, and segcompaction paths were traced; the existing
enable_vertical_segment_writer=falsemode is where the PR newly exposes both reported problems. - Observability and performance: Existing probe counters, logging, and error reporting are otherwise preserved, and I found no separate material performance regression.
No additional user review focus was supplied.
The horizontal SegmentWriter accepts flexible partial update now that the transform chain fills blocks before any writer runs, so make the coverage follow: - test_f_segment_writer.groovy pinned the deleted NotSupported rejection. It now requires the load to succeed and checks the updated rows and skip bitmaps; the expected values match test_flexible_partial_update.groovy, which runs the same load on the vertical path. It also reads the rows once more right before the load, so an unexpected change shows up there instead of in the post-update check. - The golden suite replays flexible partial update through both writers (kFlexiblePartialWriterModes). The vertical baselines stay the ones recorded from the legacy in-writer fill; the horizontal baselines are new, since the legacy horizontal writer rejected flexible and no legacy baseline can exist for it. - enable_vertical_segment_writer joins the fuzzy config set so pipeline runs exercise both writers. The only two suites that enable a VerticalSegmentWriter debug point either switch the config themselves or use a point that now lives in the fill stage, so neither depends on the default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated follow-up review completed. Round 1 converged with no new distinct inline findings.
Outcome: no new valuable findings beyond the existing review context. The horizontal regression concern from discussion_r3782501198 is addressed at this head. The empty-post-transform segment-numbering condition from discussion_r3782501187 is still reproducible in the code, although that thread is now marked resolved; I did not duplicate it.
Checkpoint conclusions:
- Goal and proof: the change moves flexible partial-update fill and aggregation into the shared block-transform chain so both segment writers consume the same full-width block. The changed unit, format, and regression coverage exercises horizontal success, fill semantics, row accounting, skip bitmaps, primary-key keys, row-store data, sequence behavior, and two-segment persistence.
- Scope: the production changes are confined to the storage transform/writer path and the tests and golden data needed for the newly supported horizontal behavior. The PR description remains an unfilled template, so the intended goal was validated from the title, implementation, and tests.
- Data correctness and transactions: the MoW snapshot/max-version boundary, temporary delete marks, sequence-loser behavior, insert-after-delete handling, default/null fill, and final row positions match the former vertical path. The previously reported empty-first-segment gap remains the only substantiated correctness concern.
- Concurrency and lifecycle: transform state is local per flush, shared counters remain atomic, and existing publication locks are unchanged. Historical rowsets remain pinned through fill, and the RowStore COW generator and probe/fetcher/aggregator reference lifetimes are sound.
- Configuration, compatibility, and parallel paths: no new configuration, protocol, or storage-format contract is introduced. The existing mutable writer-selection flag is exercised, and partial fill is fenced to the context-rich
flush_single_blockpath; streaming, V2, row-binlog, compaction, segcompaction, schema-change, and transient paths were checked for accidental reachability. - Errors, observability, and performance: transform and writer statuses cross the existing error boundary without being discarded. Existing counters and logging remain appropriate, and no separate concrete performance regression was found.
- Test coverage: both writer modes, aggregation shrink, sequence/self marks, row-store, logical persistence, input-row accounting, and two segments have direct coverage. The absent all-empty-first-then-nonempty case is the same boundary already described by
discussion_r3782501187. - User focus: no additional focus was supplied, so the whole PR was reviewed.
Review completion: converged after one complete normal and risk-focused round; all candidate points were adjudicated, with zero new inline comments.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17746 ms |
Contributor
TPC-DS: Total hot run time: 84901 ms |
Contributor
ClickBench: Total hot run time: 14.71 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
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)