[refactor](storage) derive row binlog in the transform chain; delete RowBinlogSegmentWriter - #66883
Open
csun5285 wants to merge 2 commits into
Open
[refactor](storage) derive row binlog in the transform chain; delete RowBinlogSegmentWriter#66883csun5285 wants to merge 2 commits into
csun5285 wants to merge 2 commits into
Conversation
variant_assembler.cpp and variant_storage_cell.cpp each keep an identical publish_encoded() in an anonymous namespace. The unity build merges both files into one translation unit, where the two anonymous namespaces become one, so the pair only compiles while the files land in different unity batches -- adding or removing any source file in be/src/storage/ can shift them together and break the build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 18, 2026 07:23
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: no blocking issues found; approval from the code-review perspective.
Critical checkpoint conclusions:
- Goal and proof: The PR cleanly moves direct row-binlog derivation into the block-transform chain and leaves the ordinary horizontal/vertical segment writers as sinks. The new derive tests cover Plain and MoW behavior, partial updates, BEFORE/AFTER images, sequence/delete handling, hidden/complex/nullable columns, source-tablet selection, and negative invariants. Storage-format tests cover both writers; all six added vertical segment files are byte-identical to their legacy horizontal counterparts.
- Scope: The change is focused on this ownership transfer. Deleted writer code and tests are replaced by the transform implementation/tests; no stale production or build references remain. The Variant helper rename only resolves an anonymous-namespace unity-build collision.
- Concurrency: Concurrent memtable/group flushes allocate distinct atomic segment ids. The shared LSN map is mutex-protected, lock scope is limited to lookup/erase, and the derive retains shared ownership after erase. I found no nested-lock ordering or deadlock issue.
- Lifecycle/static initialization: Each direct flush owns one converter/retriever/read plan; the converter outlives the retriever accessors and all historical values are materialized before rowset pins are released. Failed attempts do not reuse consumed segment state. No cross-TU static initialization or reference cycle was introduced.
- Configuration: No configuration item was added. The existing vertical-writer flag now reaches row-binlog segments through the common writer path; no dynamic-config propagation contract changed.
- Compatibility/storage format: Supported schema construction, replica creation/recovery, edit-log replay, local/cloud metadata, and rolling upgrades keep
need_historical_value,write_before, and the row-binlog schema paired. The deleted NULL-fill compatibility branch only tolerated an unsupported divergent metadata state. Existing horizontal golden formats remain stable and the new vertical output matches them byte-for-byte; no FE/BE protocol or persistent metadata format changed. - Parallel and conditional paths: Direct Plain/MoW selection matches the legacy predicates. Fixed partial updates, full upserts, transient publish rewrites, local/cloud builders, and non-direct compaction/schema-change/snapshot inputs were traced. Non-direct paths already supply final binlog-shaped blocks; the common vertical writer has no row-binlog-specific branch at this seam.
- Tests and results: The changed unit/golden suite has strong positive and negative coverage, and the checked-in manifest/results are internally consistent. A dedicated non-direct + vertical row-binlog golden would add matrix coverage, but no distinct failure mechanism was found. Per the review-only bundle, I did not build or run tests locally. Current completed CI checks for style, clang format, license, dependency review, title, secrets, and large files pass; macOS BE UT is still pending at review time.
- Observability/error handling: New transform failures propagate through the flush/publish status path, and schema/source-context errors include tablet context. Invariants fail loudly rather than allowing a malformed mixed-width block to continue. This refactor does not introduce a new operational state that requires metrics or routine INFO logging.
- Transactions/persistence/data writes: No EditLog or persistent metadata write changed. Version-scoped MoW lookup, publish-time row/LSN permutation, child segment alignment, rollback, and visibility semantics are preserved. A data/binlog child failure aborts the owning load or publish attempt; retries reconstruct fresh writer state, so I found no partial-visibility or same-segment retry defect.
- Variables/protocol plumbing: No new FE-to-BE variable or RPC field was added, so there are no additional sending/replay paths to update.
- Performance: Plain and all non-direct paths retain one conversion. MoW derives probe keys before the common writer converts them for persistence, but this is bounded adapter work within the established transform/writer contract and there is no evidence of a material regression. Historical lookup remains single-pass, and heavy work occurs outside the short LSN-map lock.
- Other issues: No additional correctness, memory-safety, nullability, compatibility, lifecycle, performance, or coverage defect was substantiated.
User focus: no additional focus points were supplied; the full PR was reviewed.
Convergence: two full review rounds completed. In Round 2, three normal reviewers and a separate residual-risk reviewer all returned NO_NEW_VALUABLE_FINDINGS; there are zero inline findings.
Contributor
TPC-H: Total hot run time: 17552 ms |
Contributor
TPC-DS: Total hot run time: 82268 ms |
…RowBinlogSegmentWriter Last PR of the block-transform-chain stack. RowBinlogDeriveStage rebuilds the load block into a full-width block over the binlog schema -- key + AFTER values, optional __BEFORE__* values, and the TSO/LSN/op columns -- so the ordinary segment writers write it like any other block. build_transform_chain picks PlainRowBinlogDerive (no history probe) or MowRowBinlogDerive (partial update and/or BEFORE image) per flush, and keeps an empty chain for every non-direct write type, whose rows are already binlog shaped. RowBinlogSegmentWriter is deleted, and with it the dead VerticalRowBinlogRowsetWriter template, the retriever's clear() bridge and SegmentWriter::build_key_index's protected visibility. SegmentFlusher no longer special-cases binlog when it picks a writer. Three later fixes to the deleted writer are carried into the stage, since deleting the file would drop them silently: - apache#66046: the op revision from the old delete sign runs between the BEFORE fill and the op column fill, the order the writer used. - apache#65810: the cloud-mode rejection is gone, and the historical lookup uses source.base_tablet. The writer fell back to its own tablet when that was unset; the stage requires it instead, because on a binlog write the writer context's tablet is the binlog tablet, which holds no source rows. Every site that fills source.tablet_schema fills source.base_tablet too. - apache#66553: AFTER value columns are wrapped nullable before they land in the binlog block, which declares them nullable. Behavior change to ack: binlog segments are written by the ordinary segment writer, which is the vertical one under the default enable_vertical_segment_writer=true; the deleted writer was always horizontal. The derived rows are identical -- the plain, MoW and BEFORE binlog goldens recorded through the vertical writer are byte-identical to the baselines recorded from the legacy writer -- but the writer type and the physical page layout are visible to operational tooling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
csun5285
force-pushed
the
refactor/row-binlog-derive-stage
branch
from
August 18, 2026 11:31
f1df78e to
5771ed0
Compare
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17622 ms |
Contributor
TPC-DS: Total hot run time: 85139 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
Contributor
TPC-H: Total hot run time: 17444 ms |
Contributor
TPC-DS: Total hot run time: 84391 ms |
Contributor
ClickBench: Total hot run time: 14.69 s |
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?
Related PR: #66774 (stack PR 5/6), #66609 (4/6), #66528 (3/6), #66151 (2/6), #65492 (1/6)
Last PR of the block-transform-chain refactor stack, and the one that pays it off: with the binlog derivation in the chain, no segment writer derives or fills anything any more -- both writers are pure storage-format sinks.
RowBinlogDeriveStage: rebuilds the load block into a full-width block over the binlog schema -- key +AFTER values, optional
__BEFORE__*values, and the TSO / LSN / op columnsRelease note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)