[refactor](storage) extract MowKeyProbe and HistoricalRowFetcher - #66151
Open
csun5285 wants to merge 1 commit into
Open
[refactor](storage) extract MowKeyProbe and HistoricalRowFetcher #66151csun5285 wants to merge 1 commit into
csun5285 wants to merge 1 commit into
Conversation
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
July 28, 2026 06:58
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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.
Automated review opinion: request changes.
The production refactor is coherent and the extracted policy/fetcher behavior matches the removed fixed, flexible, and row-binlog paths. One important proof gap remains: the only test of the new probe-side RowKeyEncoder constructor explicitly excludes cluster-key schemas, even though selecting the primary-key view instead of the cluster sort view is the overload's distinguishing contract. The inline comment requests that coverage.
Critical checkpoint conclusions:
- Goal and proof: The PR cleanly centralizes MoW lookup policy, historical-row reads, and probe-side key encoding. Static end-to-end tracing supports behavioral equivalence, but the cluster-key probe/index byte contract is not yet directly tested.
- Scope and clarity: The production changes are focused on the stated extraction; the larger diff is primarily targeted BE unit coverage.
- Concurrency: No new threads or lock ordering are introduced. Existing writer serialization and delete-bitmap synchronization assumptions are unchanged.
- Lifecycle and memory: Fixed-path rowsets remain pinned through their block-local historical read; the flexible path keeps its existing writer-owned pins; row binlog retains one fetcher through AFTER and BEFORE generation and clears it between blocks. No ownership cycle or stale-plan path was found.
- Configuration and observability: No configuration, logging, metric, or dynamic-update contract changes are introduced.
- Compatibility: No FE/BE protocol, persisted metadata, or storage-format change is introduced. Primary-key, nullable-marker, sequence-suffix, row-id, and row-cache key layouts were traced as unchanged.
- Parallel paths and conditions: SegmentWriter fixed partial update, VerticalSegmentWriter fixed/flexible partial update, sequence win/lose/equal, delete-sign, insert-after-delete, READ_ONLY/OLD_ROW/OLD_AND_NEW_ROW, row-cache, and row-binlog BEFORE/AFTER paths were checked. TEMP-version old/new-row marks and counters remain aligned with the former code.
- Transactions and data correctness: Visible-version lookup and
TEMP_VERSION_COMMONdelete-bitmap behavior are preserved; no new persistence or failover path is added. - Performance: The extraction adds no material hot-loop algorithmic regression or redundant I/O.
- Tests: The added component tests cover the main probe, fetch, cache, sequence, delete, BEFORE/AFTER, and clear/reuse branches. The cluster-key probe-view case called out inline is still required. This review was intentionally read-only and did not run local builds or tests.
- User focus: No additional review focus was supplied; the complete PR was reviewed.
csun5285
force-pushed
the
refactor/mow-key-probe
branch
from
July 28, 2026 10:48
d5d0148 to
c3a0734
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Automated review opinion: no additional changes requested from code review.
I found no new correctness issue at current head c3a07347983be225868fdbeeb473632154330630. The previous inline request for a cluster-key probe-encoder proof is represented in the current head by ProbeEncoderMatchesPrimaryKeyViewWithClusterKeys, which directly compares the probe-side encoding with the writer's primary-key-index view.
Critical checkpoint conclusions:
- Goal and proof: The PR cleanly centralizes MoW lookup policy and historical-row ownership while preserving writer and row-binlog behavior. The added unit tests cover encoder views, probe results and bitmap modes, historical reads/defaults, cache invalidation, row-binlog BEFORE/AFTER behavior, sequence loss, and per-block clear.
- Scope and clarity: The production change is focused on the stated extraction. The unused flexible-fetcher and previous-sequence helpers are preparatory surface but do not change a current runtime path.
- Concurrency: No new thread entry or lock ordering is introduced. Writer execution remains staged, and delete-bitmap access retains its existing synchronization.
- Lifecycle and memory: Fixed writer fetchers pin rowsets through their synchronous reads; the flexible path retains the writer-owned pin map needed by its aggregator/read plan; row binlog retains one fetcher through AFTER and BEFORE construction and resets it between blocks. No ownership cycle or stale-plan path was found.
- Compatibility: No wire, persisted metadata, or storage-format change is introduced. Primary-key prefixes, nullable markers, sequence suffixes, cluster-key rowid suffixes, supported sequence schema evolution, and seqless row-cache keys remain compatible with lookup/index behavior.
- Parallel paths and conditions: Horizontal fixed, vertical fixed/flexible, read-only row binlog, sequence win/equal/loss, delete-sign, and same-load insert-after-delete paths were checked. TEMP-version old/new-row marks, counters, error propagation, and old-value decisions match the pre-refactor branches.
- Transactions and data correctness: Visible-version lookup and
TEMP_VERSION_COMMONdelete-bitmap semantics are preserved; no new persistence, EditLog, or failover path is added. - Configuration and observability: No configuration, metric, or logging contract changes are introduced, and no new observability requirement was identified.
- Performance: The refactor adds no new lookup, historical I/O, cache operation, material allocation, or algorithmic scan in the hot loops.
- Tests and validation: The current head has passing compile, Linux BE UT, macOS BE UT, formatter, and style checks. This review was intentionally static because the review bundle forbids local builds. At review time, External Regression and BE coverage are failed, while several regression and performance checks remain pending, so CI is not fully green and those checks still need to be resolved independently.
- User focus: No additional review focus was supplied; the complete PR was reviewed.
Contributor
TPC-H: Total hot run time: 29441 ms |
Contributor
TPC-DS: Total hot run time: 177001 ms |
Contributor
ClickBench: Total hot run time: 25.15 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
refactor/mow-key-probe
branch
2 times, most recently
from
July 29, 2026 03:50
44c32f8 to
5a38487
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review opinion: request changes.
The production refactor is focused and the extracted probe policies, delete-bitmap behavior, key encoding, and rowset/read-plan ownership match the removed horizontal, vertical, flexible, and row-binlog paths. One test remains non-discriminating for the central lifecycle contract: ClearResetsThePerBlockState still passes if the old fixed read plan survives clear(). The inline comment requests a shape that actually detects stale state and covers the production consumer order.
Critical checkpoint conclusions:
- Goal and proof: The PR centralizes MoW key probing and historical-row ownership without changing the intended write behavior. Static tracing supports the production refactor, but the new clear/reuse test does not prove that the old per-block read plan is discarded.
- Scope and clarity: The production changes are focused on the stated extraction and share the same abstractions across the parallel writers and row-binlog retriever. Most of the larger diff is targeted BE unit coverage.
- Concurrency: No new thread entry, shared concurrent state, lock order, or atomic protocol is introduced. Writer/retriever state remains stage-local, and existing tablet-header/delete-bitmap synchronization is unchanged.
- Lifecycle and memory: Fixed writers keep a local fetcher and its rowset pins through the synchronous historical read; the flexible path retains its existing writer-owned pins; row binlog retains one fetcher through AFTER and BEFORE construction and recreates it at block clear. No ownership cycle or unsafe borrowed lifetime was found. The inline finding is specifically that the test would not catch a regression in that final reset step.
- Configuration and compatibility: No configuration, dynamic-update contract, FE/BE variable, wire protocol, persisted metadata, EditLog, or storage-format change is introduced. Primary-key markers, sequence and cluster-rowid suffixes, row-cache keys, and supported sequence-schema evolution remain compatible.
- Parallel paths and conditions: Horizontal fixed, vertical fixed/flexible, row-binlog read-only lookup, sequence win/equal/loss, delete-sign handling, same-load insert-after-delete, default/null filling, and cloud-mode guards were checked. TEMP-version bitmap marks, counters, callbacks, and checked error propagation match the former branches.
- Transactions and data correctness: Visible-version lookup, rowset pinning, and
TEMP_VERSION_COMMONdelete-bitmap ownership are preserved; no new transaction, failover, or persistence path is added. - Error handling, memory safety, and nullable handling: Returned statuses remain checked, unexpected lookup errors propagate, rowset ownership covers deferred reads, and BEFORE nullable insertion retains the established checked shape. No new speculative defensive continuation was added.
- Performance and observability: The extraction adds no material algorithmic scan, historical I/O, cache operation, lock-held work, or hot-loop allocation. Existing logging is sufficient for this internal refactor; no new metric is required.
- Tests and results: The added BE unit suites cover encoder views, probe policies, cache invalidation, historical fetches, sequence/delete behavior, BEFORE/AFTER construction, and reuse flags. The accepted inline issue is the remaining false-negative lifecycle test and missing production
AFTER -> BEFORE -> clearsequence. No generated regression result is involved. Per the review-only instructions, I did not run local builds or tests. - Other issues: A suspected cluster-key equal-prefix lookup problem was dismissed after tracing every supported segment producer: normal MemTable flush, schema change, compaction, and conflict rewrite all deduplicate or filter unique-key rows before persistence. It is not submitted as a speculative blocker.
- User focus: No additional review focus was supplied; the complete PR was reviewed.
| } | ||
| retriever.clear(); | ||
|
|
||
| Block second = key_block(schema, {2}); |
Contributor
There was a problem hiding this comment.
[P2] Make the clear test fail on a retained read plan
Both batches use the same rowset/segment and plan destination position 0. If clear() stopped recreating _row_fetcher, FixedReadPlan::read_columns_by_plan would read both row-ids and the later mapping would overwrite read_index[0], so this test would still return 22 and pass with stale per-block state. Please make the post-clear batch a miss and assert its BEFORE value is NULL (or otherwise choose a shape where the retained mapping is observable), and cover the production AFTER -> BEFORE -> clear order so the plan is proven to survive both consumers and then be discarded.
Contributor
TPC-H: Total hot run time: 29693 ms |
Contributor
TPC-DS: Total hot run time: 177829 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
refactor/mow-key-probe
branch
from
July 29, 2026 07:43
5a38487 to
1cbdc56
Compare
…ment writers and row-binlog retriever delegate probe, row-cache invalidation and old-value reads Part 2/6 of splitting apache#64674 (block transform chain) into independently reviewable PRs. Part 1 was apache#65492 (RowKeyEncoder). The merge-on-write primary key probe - look the key up in the load's rowset snapshot, mark the delete bitmap, decide whether the old row has to be read for the missing columns - existed in three near-verbatim copies: SegmentWriter::probe_key_for_mow, VerticalSegmentWriter::_probe_key_for_mow and PrimaryKeyModelRowRetriever. The key encoding around it and the row-cache invalidation were duplicated the same way. The three copies differ in exactly three rules, all of which were expressed by copy-editing the code. Extract them into storage/mow/: - MowKeyProbe: one probe behind a Policy (delete_bitmap_mode READ_ONLY / OLD_ROW / OLD_AND_NEW_ROW, skip_delete_sign, skip_seq_loses, skip_in_load_deleted), plus probe_previous_seq_value(), maybe_invalidate_row_cache() and encode_mow_key_invalidate_cache(), which encodes a key, invalidates the row cache and appends the sequence suffix in the one order that is correct. - HistoricalRowFetcher: owns the rowset pins and the read plans behind FixedReadPlan / FlexibleReadPlan. All three call sites delegate. Both writers keep a thin wrapper that turns a ProbeOutcome back into the out-parameters their fill loops use and run their fixed partial-update fill on a per-call HistoricalRowFetcher; the wrappers are bridge code, deleted when the fills move into chain stages. PrimaryKeyModelRowRetriever drops its own encode / probe / row-cache copies and holds a RowKeyEncoder plus a HistoricalRowFetcher, still driven by the existing RowBinlogSegmentWriter so the row-binlog regressions cover the rework. RowKeyEncoder gains a second constructor for the probe side: the key the primary key index is built on is always the schema key columns, while the writer's constructor builds whatever the segment sorts by (cluster keys when it has them) plus the separate primary-key view and the rowid suffix. The retriever is the first caller that has to encode primary keys for any mow table, with or without cluster keys. Equivalence notes: the Policy reproduces all three old copies term for term (delete-bitmap keys, marked rows and stat counters unchanged); SegmentWriter's cast_set<uint32_t>(max_version) truncation is dropped in favour of the int64 behavior the other two callers already had; lookup_row_key is called with an explicit with_rowid=false, which fixes the retriever on cluster-key mow tables (the default strips ROW_ID_LENGTH bytes off a probe key that carries no rowid suffix) and is inert for partial update, which FE forbids on those tables; num_rows_new_added is bumped inside probe(), before the writers' handle_new_key runs at the call site; fixed-path rowset pins move from writer-lifetime maps to the per-call fetcher, so SegmentWriter::_rsid_to_rowset is gone while the vertical writer keeps its map for the flexible fill and BlockAggregator; the retriever's clear() now also drops the fetcher that holds the read plan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
csun5285
force-pushed
the
refactor/mow-key-probe
branch
from
July 29, 2026 08:03
1cbdc56 to
a084c9d
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29364 ms |
Contributor
TPC-H: Total hot run time: 29723 ms |
Contributor
TPC-DS: Total hot run time: 177455 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
There was a problem hiding this comment.
Automated review opinion: no additional changes requested.
At reviewed head a084c9d5068d399e6569d3b013d923ee368202a7, I found no new
correctness issue. Both earlier inline proof gaps are addressed in the current code: the
cluster-key primary-key view is separated from the segment sort-key view with byte-level
primary/sequence/rowid coverage, and the clear test now exercises AFTER -> BEFORE -> clear before a
miss whose BEFORE value must remain NULL.
Critical checkpoint conclusions:
- Goal and proof: The PR consistently centralizes MoW key probing, primary-key encoding/cache
invalidation, and historical-row ownership. Component tests exercise real primary indexes and
the key policy/fetch/retriever contracts. - Scope and clarity: The production changes are focused on the extraction. The preparatory
previous-sequence and flexible-fetcher surfaces are not production-wired yet, but introduce no
current behavior change. - Concurrency: No new thread entry, shared concurrent state, lock order, or atomic protocol is
introduced. The affected writer/retriever stages remain sequential. - Lifecycle and memory: Fixed paths pin rowsets through their synchronous historical reads;
vertical flexible keeps its existing writer-owned pins; row binlog preserves one plan through
AFTER and BEFORE and discards it at per-block clear. No ownership cycle or premature release was
found. - Configuration and observability: No configuration or dynamic-update contract changes. Lookup
failures retain checked propagation and now include tablet/transaction context; no new metric is
required for this refactor. - Compatibility: No FE/BE protocol, persisted metadata, or storage-format change. Primary-key
markers, nullable markers, sequence suffixes, cluster rowid suffixes, and sequence-less row-cache
keys remain aligned with primary-index construction and lookup. - Parallel paths and conditions: Horizontal fixed, vertical fixed/flexible, and row-binlog paths
were checked across misses, sequence win/equal/loss, delete signs, same-load insert-after-delete,
row-store/column-store reads, and BEFORE/AFTER generation. The policy outcomes match the removed
branches. - Transactions and data correctness: Visible-version lookup and immediate
TEMP_VERSION_COMMONold/incoming-row marks, targets, and counters are preserved. Failures still
abort through checked statuses; no new commit, persistence, or failover path is added. - Performance: No additional lookup, historical I/O, asymptotic scan, lock-held work, or material
hot-loop allocation was introduced. - Tests and validation: The changed unit tests cover encoder views and suffixes, probe policies,
cache invalidation, historical reads/defaults, delete/sequence behavior, BEFORE/AFTER reuse, and
clear. No generated regression result is involved. Per the review-runner contract, this review
was static and did not run local builds or tests; repository CI remains an independent merge
gate. - User focus: No additional review focus was supplied; the complete PR was reviewed.
Contributor
There was a problem hiding this comment.
Automated review opinion: no additional changes requested.
At reviewed head a084c9d5068d399e6569d3b013d923ee368202a7, no new correctness issue survived the full review and convergence pass. The two earlier inline proof concerns are covered by the current code:
- Cluster-key encoding:
AllKeyTypesTableindependently compares the cluster-key primary view with canonical primary-key bytes across all key types, includes cluster-plus-sequence primary-index entries, and snapshots exact bytes.RowidSuffixseparately checks exact primary + sequence + rowid bytes. The narrower focused test is not the sole proof. - Retriever lifecycle:
ClearResetsThePerBlockStatenow follows the production AFTER -> BEFORE -> clear order, then probes a miss and requires its BEFORE value to remain NULL, so both premature plan consumption and stale-plan retention are observable.
Critical checkpoint conclusions:
- Goal and proof: The PR consistently centralizes MoW key probing, cache invalidation, and historical-row ownership while preserving the removed horizontal, vertical, flexible, and row-binlog behavior. The helper and retriever tests exercise real primary-index lookup and the policy/read-plan contracts.
- Scope and clarity: The production changes are focused on the stated extraction. Preparatory previous-sequence and flexible-fetcher APIs are not production-wired yet but do not alter a current runtime path.
- Concurrency: No new thread entry, lock order, atomic protocol, or independently shared mutable state is introduced. The affected writer/retriever stages remain sequential, and existing delete-bitmap synchronization assumptions are unchanged.
- Lifecycle and memory: Fixed writers keep each read plan and its
RowsetSharedPtrpins through the synchronous fill. Vertical flexible retains the writer-owned map used by its aggregator/read plan. Row binlog keeps one fetcher through AFTER and BEFORE, then recreates it at per-block clear. No ownership cycle, premature release, or stale-plan path remains. - Configuration and compatibility: No configuration, dynamic-update contract, FE/BE variable, wire protocol, persisted metadata, EditLog, or storage-format change is introduced. Primary-key markers, nullable markers, sequence suffixes, cluster rowid suffixes, and sequence-less row-cache keys remain aligned with primary-index construction and lookup.
- Parallel paths and special conditions: Horizontal fixed, vertical fixed/flexible, and read-only row-binlog paths were checked for misses, sequence win/equal/loss, delete signs, same-load insert-after-delete, row-store/column-store reads, and BEFORE/AFTER generation. Policy outcomes and
handle_new_keybehavior match the former branches. - Transactions and data correctness: Visible-version lookup and immediate
TEMP_VERSION_COMMONmarks preserve their targets, timing, and counters. Returned lookup/read errors remain checked and abort the write; no new commit, persistence, failover, or silent-continuation path is added. - Test coverage and results: The changed BE unit tests cover encoder views/suffixes, probe policies, cache invalidation, historical reads/defaults, delete/sequence behavior, BEFORE/AFTER reuse, and clear. No generated regression result is involved. Per the review-runner contract, this review was static and did not run local builds or tests. Repository CI is still an independent merge gate.
- Observability: Lookup failures now retain tablet/transaction context, and no new long-lived or distributed operation needs a metric or additional logging.
- Performance: The refactor adds no extra lookup, historical I/O, asymptotic scan, lock-held work, or material hot-loop allocation.
- Other concerns: No additional correctness, nullable-shape, memory-tracking, or compatibility issue was substantiated.
- User focus: No additional review focus was supplied; the complete PR was reviewed.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
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.
MowKeyProbe— one probe behind aPolicy: which rows to mark in the delete bitmap, whether a delete-signed row or a row that loses on sequence still needs its old values read, and flexible partial update's insert-after-delete rule.Those are exactly the rules the three copies differed by.
HistoricalRowFetcher— owns the rowset pins and the read plans used to read those old values.Both segment writers and the row binlog retriever now delegate to them，
RowKeyEncodergains a second constructor for the probe side, since the key the primary key index is probed with is always the schema key columns while thewriter's encoder builds whatever the segment sorn wrapper, deleted when the partial-update fills move into the transform chain in he follow-ups.
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)