[fix](binlog) Fix BE crash when APPEND_ONLY row-binlog scan omits key columns - #66432
Merged
Conversation
… columns Any row-binlog scan used to force a key-ordered merged read with read_orderby_key_num_prefix_columns == 0, which makes the merge comparators (VMergeIteratorContext::compare and VCollectIterator's LevelIteratorComparator) fall back to comparing the first num_key_columns block positions, where num_key_columns counts the key columns of the whole tablet schema. MIN_DELTA / DETAIL scans satisfy that contract because they widen the storage projection with the full key prefix. APPEND_ONLY, however, takes the direct_mode branch and keeps the raw SQL projection, which may omit some or all key columns. Block::compare_at() only has DCHECK bounds checks, so in a release build the first heap comparison reads past the block's column array and crashes the BE with SIGSEGV inside VMergeIterator::init / std::push_heap. Fix: - olap_scanner: only MIN_DELTA / DETAIL force the key-ordered merged read. APPEND_ONLY does no key grouping, so it now reads unordered like a plain scan (ORDER BY / TopN pushdowns stay disabled for all binlog scan types as before). - VMergeIteratorContext::init and VCollectIterator::Level1Iterator::init now validate the merge compare contract (compare positions exist in the block, and the projection starts with the full ordered key prefix for the default comparison) and return a diagnosable error instead of performing an out-of-bounds or silently misordered comparison. - Add BE unit tests for the rejected projections and a regression suite covering APPEND_ONLY / DETAIL / MIN_DELTA with narrow projections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28982 ms |
Contributor
TPC-DS: Total hot run time: 166063 ms |
Contributor
ClickBench: Total hot run time: 23.93 s |
Contributor
|
/review |
Userwhite
approved these changes
Aug 4, 2026
![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. The production fix addresses the crash mechanism cleanly, but the new regression does not actually exercise the retained DETAIL/MIN_DELTA merge paths and does not follow the repository's required result/lifecycle conventions. I left two inline comments covering those gaps.
Critical checkpoint conclusions:
- Goal and proof: APPEND_ONLY no longer requests the default full-key merge on a projection that may omit keys, and both storage heap layers validate every comparator position before insertion. The code-level goal is met, but the claimed merge-mode regression proof is incomplete because FE coerces both nominal DETAIL/MIN_DELTA queries on this DUP_KEYS table back to APPEND_ONLY.
- Scope, reuse, and parallel paths: The source change is focused. APPEND_ONLY, MIN_DELTA, and DETAIL are separated at the scanner boundary; the VMerge and VCollect comparators receive equivalent contract checks. I found no missed production path after tracing FE op filtering, TSO pushdown, TabletReader, BetaRowsetReader, and BlockReader.
- Concurrency, lifecycle, and memory: No new shared mutable state or lock ordering is introduced. Validation occurs after each first block is loaded and before heap insertion; later blocks retain the same schema. The checks add no steady-state allocation beyond the error path.
- Error handling and observability: Contract violations now return non-OK Status values with projection and tablet/rowset/version context instead of allowing unchecked positional access. Status propagation through rowset/tablet reader initialization is preserved.
- Configuration, compatibility, persistence, and writes: No product configuration, storage format, symbol, transaction, persistence, or FE-BE protocol change is introduced. The APPEND_ONLY physical order change is unspecified SQL order and is documented in the release note.
- Performance: Removing the unnecessary APPEND_ONLY merge heap is beneficial; the new validation is once per initialized merge context/heap and is negligible relative to heap construction.
- Tests: The narrow APPEND_ONLY reproducer has a deterministic overlapping-rowset topology under the row-binlog compaction freshness gate, and the new VMerge unit cases pass. The remaining blockers are the false DETAIL/MIN_DELTA coverage and the missing generated golden-output/test-lifecycle compliance called out inline.
- User focus: No additional focus was provided, so the complete PR was reviewed.
Validation was static-only as required by the review runner. At the reviewed head, compile/format and nonConcurrent regression checks passed. The two added VGenericIterators tests passed in TeamCity; the aggregate BE-UT status is red for unrelated SegmentFlusher golden-byte failures, while the macOS BE-UT job failed at the linker on --exclude-libs.
Review completion: three rounds were completed. The final full-review and risk-focused agents all returned NO_NEW_VALUABLE_FINDINGS against this exact two-comment set, and every other candidate was deduplicated or dismissed with concrete code evidence.
|
|
||
| // 1. Project a single value column: 1 projected column < 2 key columns. | ||
| // This is the exact shape that used to crash the BE. | ||
| assertEquals([[30], [31], [40], [41], [50], [51]], |
Contributor
There was a problem hiding this comment.
Please follow the repository's regression-test contract for these determined results: use named qt_/order_qt_ cases and generate the matching .out file through the test runner instead of assertEquals(sql(...)). This ordinary single-table case should also hardcode the table name, and cleanup should remain at setup rather than dropping the database in finally, so failed state is preserved for debugging.
| // 5. DETAIL / MIN_DELTA keep the forced key-ordered merge but widen the | ||
| // storage projection with the full key prefix internally; a narrow SQL | ||
| // projection must still work and return the same rows for a dup table. | ||
| assertEquals([[30], [31], [40], [41], [50], [51]], |
Contributor
There was a problem hiding this comment.
These two assertions do not exercise DETAIL or MIN_DELTA. OlapScanNode.parseBinlogScanType() forces every DUP_KEYS incremental scan to APPEND_ONLY, even when the SQL requests one of these modes, so both queries re-test the APPEND_ONLY branch and cannot validate the retained key-widening/forced-merge path. Please move this coverage to a UNIQUE KEY merge-on-write table with binlog.need_historical_value=true, include update/delete changes, and keep the narrow key-omitting projection so the BE actually receives TBinlogScanType::DETAIL and MIN_DELTA.
yiguolei
requested changes
Aug 5, 2026
yiguolei
left a comment
yiguolei
left a comment
Contributor
There was a problem hiding this comment.
这个先不要这么修复了。
我们再做一个改动，把所有的列的规划都放到FE 里，这样BE 就不会出现自己加一些列了。
csun5285
approved these changes
Aug 5, 2026
yiguolei
approved these changes
Aug 6, 2026
Contributor
|
skip check_coverage |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 7, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66432 Problem Summary: Storage readers maintained FE block positions, tablet-schema column IDs, predicate IDs, and delete-predicate columns through parallel mappings. Projection, nested-column pruning, virtual expressions, and row-binlog dependencies could make those mappings diverge. Use one ordered read schema as the reader coordinate, keep expected materialization types alongside physical columns, append storage-only delete-predicate dependencies after FE slots, and preserve required row-binlog scan columns without changing the scan output contract. ### Release note Fix inconsistent storage-reader column mappings for projected and row-binlog scans. ### Check List (For Author) - Test: - ASAN BE and FE build - BE clang-format and format check - FE Checkstyle - Regression: row_binlog_p0, delete_p0, unique_seq_map_p0, and targeted schema-change/delete/sequence suites - Regression: variant_p0 code-related cases passed; one outfile case was blocked by invalid external OSS credentials - Behavior changed: Yes (reader column identity and row-binlog dependency handling are unified) - Does this need documentation: No
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 7, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66432 Problem Summary: Storage readers maintained FE block positions, tablet-schema column IDs, predicate IDs, and delete-predicate columns through parallel mappings. Projection, nested-column pruning, virtual expressions, and row-binlog dependencies could make those mappings diverge. Use one ordered read schema as the reader coordinate, keep expected materialization types alongside physical columns, append storage-only delete-predicate dependencies after FE slots, and preserve required row-binlog scan columns without changing the scan output contract. ### Release note Fix inconsistent storage-reader column mappings for projected and row-binlog scans. ### Check List (For Author) - Test: - ASAN BE and FE build - BE clang-format and format check - FE Checkstyle - Regression: row_binlog_p0, delete_p0, unique_seq_map_p0, and targeted schema-change/delete/sequence suites - Regression: variant_p0 code-related cases passed; one outfile case was blocked by invalid external OSS credentials - Behavior changed: Yes (reader column identity and row-binlog dependency handling are unified) - Does this need documentation: No
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
7 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 #66390
Related PR: #63850
Related Issue: #65418
Problem Summary:
A BE received SIGSEGV in
VMergeIteratorContext::compare()whileVMergeIterator::init()was building its merge heap during a ROW-binlog regression workload.Root cause chain:
binlog_scan_type != NONE, includingAPPEND_ONLY) forced a key-ordered merged read withread_orderby_key_num_prefix_columns == 0.read_orderby_key_columnsstays null, so both merge comparators (VMergeIteratorContext::compare()at the rowset level andVCollectIterator::LevelIteratorComparatorat the rowset-tree level) fall back to comparing the firstnum_key_columnsblock positions.Schema::num_key_columns()counts the key columns of the whole tablet schema, independent of the projection, so the fallback silently assumes the projection starts with the full ordered key prefix.MIN_DELTA/DETAILsatisfy that contract because they widen the storage projection with every key column.APPEND_ONLYtakes thedirect_modebranch (return_columns = SQL projection), which may omit some or all key columns (e.g.SELECT v1 FROM t@incr(... "incrementType" = "APPEND_ONLY")on a table with two key columns).Block::compare_at()has only DCHECK bounds checks, so in a release build the first heap comparison reads past the block's column array and kills the BE. This is reachable whenever a rowset has more than one segment (force_key_ordered_readmakesis_merge_iterator()true even for non-overlapping segments) or rowsets overlap. A projection that has enough columns but not the leading keys would instead be compared on the wrong columns — a silent misordering.Fix (three layers):
olap_scanner.cpp): onlyMIN_DELTA/DETAIL— the modes that actually group by key and reconstruct BEFORE/AFTER rows, and whose projection is widened with the full key prefix — force the key-ordered merged read.APPEND_ONLYdoes no key grouping (it is a plain op-filter + TSO-range stream), so it now reads unordered like a plain scan, which also removes an unnecessary merge-heap cost from the highest-throughput mode. ORDER BY / TopN pushdown params stay disabled for all binlog scan types, as before.VMergeIteratorContext::init(): validate the compare contract once the first block is loaded — explicit compare columns must point inside the block; the default key-prefix comparison requires the projection to start with exactly the schema's key column ids in order; the sequence tie-break position must be in range. Violations return anInternalErrorcarrying tablet/rowset/version/projection details instead of an out-of-bounds read.VCollectIterator::Level1Iterator::init(): validate every child's first block against the same contract before anything is pushed into the merge heap.Release note
Fixed a BE crash (SIGSEGV in the storage merge comparator) when an APPEND_ONLY row-binlog incremental scan projected fewer columns than the table's key columns. APPEND_ONLY binlog scans no longer force a key-ordered storage read; their row order (without ORDER BY) may differ from before.
Check List (For Author)
Test
Behavior changed:
InternalError(contract violations).Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code