Doris-Breakwater commented on issue #68239: URL: https://github.com/apache/doris/issues/68239#issuecomment-5747494715
## Initial assessment **Static triage: confirmed correctness defect at `84ebcf71bb52735c22034d9185c51e373caa9acd`; recommended priority: high.** The report has not demonstrated the failure in a running cluster, so the runtime manifestation is still unverified, but the loss mechanism is established by the code and is sufficient to treat this as a real bug rather than a speculative hypothesis. The issue is currently open with no labels, assignee, milestone, or linked development work. It should receive the project's bug/correctness and BE storage/compaction labels. ### Verified facts 1. The hidden commit-TSO column is defined with physical default `0` ([`ColumnDefinition.java`](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java#L734-L743)). Publish assigns the real TSO to rowset metadata ([`Rowset::make_visible`](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/be/src/storage/rowset/rowset.cpp#L87-L99)). 2. `BetaRowsetReader` passes the enclosing rowset's version and commit-TSO range to every segment. `Segment::new_column_iterator` substitutes a logical constant only when that rowset version is a singleton (`version.first == version.second`); otherwise it uses the physical column ([reader setup](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/be/src/storage/rowset/beta_rowset_reader.cpp#L98-L115), [substitution condition](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/be/src/storage/segment/segment.cpp#L946-L966)). 3. Ordered compaction calls `link_files_to()` for each input, builds one spanning-version output, and stores only `commit_tso_range(_input_rowsets)` in its metadata. It does not persist an input-rowset/segment-to-TSO mapping ([ordered output construction](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/be/src/storage/compaction/compaction.cpp#L461-L512)). 4. The admission code rejects Cloud mode and ordinary unique-key MoW tablets, but it has no exclusion for an ordinary DUP base tablet whose schema has `commit_tso_col_idx() != -1` ([admission path](https://github.com/apache/doris/blob/84ebcf71bb52735c22034d9185c51e373caa9acd/be/src/storage/compaction/compaction.cpp#L571-L688)). Ordered compaction is enabled by default; the default minimum segment size is 10 MiB. Therefore, after linking singleton rowsets with different commit TSOs into a multi-version output, the reader supplies the output version/range to each linked segment, fails the singleton check, and reads the retained physical `0`. The stated consequences follow: - projection can expose `0`; - `commit_tso > 0` can prune/reject affected rows; - `commit_tso <= target` can admit rows whose source commit TSO is newer than `target`; - disabling ZoneMap pruning cannot repair row-level values because the physical value itself is still `0`. The normal rewrite path is a valid control: each singleton input is read with its own metadata-backed constant, and those logical values are then written into the multi-version output. A single output min/max range cannot reconstruct the per-segment values after files have been linked. This also matches the discussion on #68097: the ordered-link case was independently acknowledged there as a pre-existing correctness issue and explicitly deferred to separate producer-side work. #68097's reader changes do not preserve the mapping lost here. ### What is still missing No end-to-end or BE fixture has run the exact path yet. Runtime confirmation should capture: - the exact BE commit and the two relevant configuration values; - input rowset versions and singleton commit TSOs, then output version and TSO range; - the `start to do ordered data compaction` log or compaction-profile evidence proving the link path ran; - row identity plus projected commit TSO before and after compaction, and results for both `commit_tso > 0` and a boundary between the two source TSOs. These are needed to validate the user-visible manifestation, not to establish the source-level root cause. Logs unrelated to ordered-path selection would add little value. ### Recommended next steps 1. Apply the minimal safe fix: make ordinary base-table rowsets with `commit_tso_col_idx() != -1` ineligible for ordered/link compaction, while leaving the dedicated row-binlog path distinct. Returning `false` from ordered admission lets normal rewrite compaction materialize the logical values. 2. Add a focused case to `be/test/storage/compaction/ordered_data_compaction_test.cpp`: create a TSO-enabled DUP schema, two non-overlapping singleton rowsets with physical `0` and different positive metadata TSOs, make the ordered size threshold deterministic, and verify the ordered path is rejected after the fix. 3. In the same fixture or a controlled regression, run normal rewrite compaction and assert row identities and TSO projection/predicates before and after. The existing `time_travel_p0/test_time_travel_dup` test is insufficient because it does not prove which compaction path ran. 4. If retaining the linking optimization is desired later, persist a durable source-segment-to-TSO mapping and teach every relevant reader/pruning path to consume it. This is a larger storage-format/metadata design; the overall TSO range is not enough. No code change was made as part of this triage. Breakwater-GitHub-Analysis-Slot: slot_ddd73f540137 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
