Uh oh!
There was an error while loading. Please reload this page.
[fix](table stream) Fix multi-segment row-binlog reads for MoW tables - #66338
Conversation
hello-stephen
commented
Jul 31, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
TsukiokaKogane
commented
Jul 31, 2026
run buildall |
hello-stephen
commented
Jul 31, 2026
TPC-H: Total hot run time: 29218 ms |
hello-stephen
commented
Jul 31, 2026
TPC-DS: Total hot run time: 169216 ms |
hello-stephen
commented
Jul 31, 2026
ClickBench: Total hot run time: 23.94 s |
hello-stephen
commented
Jul 31, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TsukiokaKogane
commented
Aug 4, 2026
run buildall |
HappenLee
commented
Aug 4, 2026
/review |
There was a problem hiding this comment.
Two issues need resolution; see the inline comments.
Critical checkpoint conclusions:
- Goal and proof: The APPEND_ONLY crash trigger is correctly identified, and the DUP/MoW sections cover that direct block path. However, the blanket union-mode change leaves the supported MIN_DELTA/DETAIL row-wise child handoff able to crash (P1), so the fix is not complete across parallel consumers. The schema-derived
is_uniquechange is correct, but its distinct event-preservation behavior has no direct regression oracle (P2). - Scope and parallel paths: The production edits are localized. Ordinary/projected UNIQUE reads, AGG paths, row-binlog DUP semantics, compaction, schema-change, checksum, and cloud/manual uniqueness contexts were traced. No storage-format, protocol, or rolling-upgrade incompatibility was introduced.
- Concurrency, lifecycle, and errors: The iterator state is scanner-local and sequential; no new threads, locks, atomics, or lock-order concerns apply. The surviving defect is lifecycle initialization at union-child handoff: a later child can reach a null block before reader status propagation. Shared-block ownership, same bits, EOF handling, row locations, reverse traversal, and empty-child erasure are otherwise coherent once the child is initialized.
- Configuration and observability: No production configuration is added. The regression's temporary BE setting and debug point are scoped and restored, and the non-cloud restriction matches existing row-binlog support. No new metric or log is required for this local correction.
- Tests and results: The added output is internally consistent and deterministically ordered for the APPEND_ONLY cases. Coverage is missing for a row-wise multi-rowset handoff and for same-key/same-TSO events split across segments of one forced-merge rowset. No build or test was run during this static-only review.
- Persistence and writes: The production change is read-time only; it adds no transaction, persistence, edit-log, write-atomicity, or FE-BE variable work.
- Performance: No separate substantiated performance blocker survived review; correctness initialization must be fixed first.
- User focus: No additional user-provided focus was present, so the whole PR received the normal complete review.
Review completion: complete after two review rounds; all candidates were accepted, deduplicated, or dismissed before freezing this request-changes payload.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Aug 4, 2026
TPC-H: Total hot run time: 28727 ms |
hello-stephen
commented
Aug 4, 2026
TPC-DS: Total hot run time: 166289 ms |
hello-stephen
commented
Aug 4, 2026
ClickBench: Total hot run time: 23.9 s |
TsukiokaKogane
commented
Aug 5, 2026
run buildall |
1 similar comment
TsukiokaKogane
commented
Aug 5, 2026
run buildall |
Uh oh!
There was an error while loading. Please reload this page.
820270f to
4c7c828CompareTsukiokaKogane
commented
Aug 11, 2026
run buildall |
4c7c828 to
4966a71CompareTsukiokaKogane
commented
Aug 11, 2026
run buildall |
1 similar comment
TsukiokaKogane
commented
Aug 13, 2026
run buildall |
hello-stephen
commented
Aug 13, 2026
TPC-H: Total hot run time: 29557 ms |
hello-stephen
commented
Aug 13, 2026
TPC-DS: Total hot run time: 158011 ms |
hello-stephen
commented
Aug 13, 2026
ClickBench: Total hot run time: 24.21 s |
hello-stephen
commented
Aug 20, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
commented
Aug 21, 2026
/review |
There was a problem hiding this comment.
Review result: request changes for two nonduplicate findings (one rolling-upgrade correctness issue and one persistent performance regression). The review converged after two rounds; all three Round 2 passes returned NO_NEW_VALUABLE_FINDINGS, and the final sweep covered all 18 changed files.
Critical checkpoints:
- Goal and proof: The PR correctly improves row-binlog key/TSO ordering for newly produced data, but it does not fully accomplish safe deployment because an upgraded reader can misread pre-existing or concurrently old-produced horizontal-compaction rowsets. The new tests prove current-producer cases, not that compatibility case.
- Scope and clarity: The production changes are focused and their new conditions are documented; no unrelated source change was found.
- Concurrency and thread safety: No new shared mutable state, thread entry, lock order, or atomic protocol is introduced. Existing writer statistics remain protected while copied into metadata; no deadlock or heavy-under-lock issue was found.
- Lifecycle and static initialization: Rowset-reader acquire/release and iterator ownership remain balanced. No new static/global initialization dependency, circular ownership, or abnormal release path was found.
- Configuration: No configuration item is added. The existing dynamically selected horizontal-compaction path is part of the P1 compatibility trigger.
- Compatibility and persistence: P1 below is blocking: the removed read-side fallback leaves stale
NONOVERLAPPINGmetadata unsafe during upgrade. Current snapshot rename may recompute the bit, but ordinary persisted rowsets, mixed-version production, and index manual-build remain uncovered. - Parallel paths and conditions: Horizontal, vertical, LMax quick-merge, direct-load, APPEND_ONLY, MIN_DELTA/DETAIL, local/cloud writer, empty/single-segment, and parallel-scanner paths were checked. Current producers publish safe metadata; no additional parallel-path defect was found.
- Tests and expected results: Added BE unit/regression expectations are internally consistent and cover grouping, cross-rowset order, forced segment boundaries, and quick merge. Missing negative coverage remains for stale pre-upgrade metadata and disjoint string keys sharing a 36-byte prefix. No local build/test was run because this review job forbids builds.
- Observability: Existing reader/compaction diagnostics are adequate for these local paths; no new distributed operation or critical metric requirement was identified.
- Transactions, data writes, and crash behavior: Commit/visibility and rowset publication flows are unchanged, and no new atomicity, delete-bitmap, failover, leak, or crash-order issue was found. The P1 problem is persisted metadata compatibility, not a new transaction boundary.
- FE/BE protocol: No transmitted variable or protocol field is added, so there is no FE-BE propagation gap.
- Error handling, memory safety, and nullability: Changed calls preserve
Statuspropagation; no unchecked error, new allocation ownership, reservation, nullable/const-column, or defensive-continuation issue was found. - Performance: P2 below persistently selects merge behavior and inflates compaction score for disjoint long-prefix keys. The unconditional TSO-read concern is already covered by live thread r3820374570 and was not duplicated.
- Other findings and completion: Existing live threads fence the earlier handoff, direct-load coverage, RowsetMeta-scope, horizontal future-producer, and TSO-I/O concerns. No unresolved candidate remains.
- CI: COMPILE, BE UT, NonConcurrent Regression, cloud_p0, vault_p0, External Regression, and performance are green. General P0 Regression is red, but its TeamCity detail is authentication-gated and the status exposes no failure evidence that can be attributed to this diff.
- User focus: No additional focus was supplied; the complete PR was reviewed.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TsukiokaKogane
commented
Aug 21, 2026
run p0 |
hello-stephen
commented
Aug 21, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
76041b1 to
5ee9a43CompareTsukiokaKogane
commented
Aug 21, 2026
run buildall |
hello-stephen
commented
Aug 21, 2026
TPC-H: Total hot run time: 17333 ms |
hello-stephen
commented
Aug 21, 2026
TPC-DS: Total hot run time: 83086 ms |
hello-stephen
commented
Aug 21, 2026
ClickBench: Total hot run time: 14.87 s |
hello-stephen
commented
Aug 21, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 21, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
9181de7 to
3f58ef2CompareTsukiokaKogane
commented
Aug 24, 2026
run buildall |
hello-stephen
commented
Aug 24, 2026
TPC-H: Total hot run time: 16915 ms |
hello-stephen
commented
Aug 24, 2026
TPC-DS: Total hot run time: 82219 ms |
hello-stephen
commented
Aug 24, 2026
ClickBench: Total hot run time: 14.59 s |
hello-stephen
commented
Aug 24, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: close#65901
Related PR: #65418
Problem Summary:
Table Stream queries over ROW binlog could crash or return incorrect results when a row-binlog rowset contained multiple segments. This affected both multi-segment rowsets generated by load and non-singleton overlapping rowsets produced by row-binlog LMax quick merge. Although the source table may use UNIQUE_KEYS with Merge-on-Write, the scan reads from its dedicated DUP_KEYS row-binlog tablet; the problem was caused by inconsistent iterator selection and incorrect ordering and grouping assumptions.
For a non-overlapping multi-segment rowset,
force_key_ordered_readcausedBetaRowsetReaderto build a merge iterator solely because ordered output was required. The outerVCollectIteratorcould still select its block-based union path because the rowset was non-overlapping. This mismatch made the child iterator enter by-reference mode while its parent called the block interface, triggering the!_get_data_by_refassertion and aborting the BE. Non-overlapping segments do not require a merge iterator because an ordered union already preserves key order.Row-binlog LMax quick merge introduces another multi-segment layout: it links segments from multiple rowsets into a non-singleton rowset marked
OVERLAPPING. The previousis_segments_overlapping()implementation only recognized singleton delta rowsets, so these quick-merge rowsets could incorrectly use the union path instead of merging their segments by key and TSO. The TSO column could also be skipped after zone-map pruning even though it is required as the merge sequence column.In addition, MIN_DELTA processing used
IteratorRowRef::is_sameto identify consecutive events belonging to the same user key. This flag is a merge/dedup marker and does not reliably mark same-key events that reside in one segment after quick merge. As a result, one key's change chain could be split into multiple groups and folded incorrectly.This change separates ordered-output requirements from iterator selection: non-overlapping segments use the ordered union path, while segments explicitly marked as overlapping use the merge path. It recognizes non-singleton overlapping rowsets created by quick merge, always materializes the row-binlog TSO column when it is needed for ordering, and groups MIN_DELTA events by comparing their user-key columns directly.
Before this change, affected queries could abort the BE, read quick-merge rowsets out of order, or produce incorrect MIN_DELTA results. After this change, multi-segment row-binlog rowsets are read through a consistent iterator interface and processed in key/TSO order with correct per-key change folding.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)