[fix](storage) align pruned complex column types with sub-column iterators on merge/agg read paths#65396
Open
csun5285 wants to merge 1 commit into
Open
[fix](storage) align pruned complex column types with sub-column iterators on merge/agg read paths#65396csun5285 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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: requesting changes for one regression-test oracle issue.
Critical checkpoint conclusions:
- Goal/correctness: the C++ change threads pruned complex-column data types through the query scan schema, merge iterator, and aggregate reader paths; I did not find an additional functional bug in those inspected paths.
- Scope: the implementation is focused on query-pruned type propagation plus one regression test.
- Concurrency/lifecycle: no new thread, lock, static initialization, or ownership lifecycle risk was introduced by the changed code.
- Compatibility/persistence/config: no new persisted format, FE-BE protocol field, or config item was added; the pruned-column type map is query-local and
shawdow_copy_without_columnsclears it. - Parallel paths: TopN merge-reader, aggregate reader, ordinary materialization/rowid fetch, and compaction/vertical-reader paths were checked. Compaction/vertical paths do not use query-pruned maps.
- Tests: the regression targets the right failure family, but its layout self-check does not prove the rowset has more than one segment, which is required to select
VMergeIterator. - Validation: static review only. This checkout is shallow/missing the base SHA,
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc, so I did not build or run regression tests. - User focus: no additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite reported no valuable findings. tests-session-config proposed TSC-001, accepted as MAIN-001 and submitted inline. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
4 times, most recently
from
6141226 to
7cad469
Compare
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
4 times, most recently
from
d69fffb to
ac507fb
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
from
ac507fb to
fb7a8df
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no new distinct inline comments in this pass. The existing line-100 thread remains the only actionable issue I found, and I did not duplicate it.
Critical checkpoint conclusions:
- Goal/correctness: the PR aligns FE-pruned complex-column types with the merge block and aggregate-reader function paths. I checked the scan setup,
TabletReader,BetaRowsetReader,VMergeIteratorContext,BlockReader::_init_agg_state, rowid-fetch materialization, and compaction/schema-change call sites; I found no additional correctness issue. - Scope: the implementation is focused on pruned type propagation plus one regression test.
- Concurrency/lifecycle: no new thread, lock, static initialization, or storage lifetime risk was introduced; the pruned type map is query-local and read through the tablet schema held by the reader path.
- Compatibility/config/persistence: no new persisted format, FE-BE protocol field, or config item was added.
- Parallel paths: query TopN merge and AGG reader paths use the pruned block/type shape; load, schema-change, and vertical compaction callers continue to pass full storage types.
- Tests: the regression covers default materialization, forced inline TopN, and AGG merge reads. The remaining test oracle gap is already covered by existing thread discussion_r3544580724: line 100 still checks only
segments_overlap_pband should also assertnum_segments > 1. - Validation: static review only. This checkout is shallow and lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc, so I did not build or run regression tests. I refreshed the live PR head (fb7a8dfb66da7c750db6101d2dd6c2bb38331e90); it differs from the local checkout only by comment wording in two C++ files. - User focus: no additional user-provided review focus was present.
Subagent conclusions: both required subagents reported only the duplicate of existing thread discussion_r3544580724. The main merge recorded that as MAIN-DUP-001 with no new proposed inline comments. Convergence round 2, after the live-head refresh, ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
TPC-H: Total hot run time: 29975 ms |
Contributor
TPC-DS: Total hot run time: 181241 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
from
fb7a8df to
26bb98e
Compare
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one remaining test-coverage issue. The storage/query-reader changes themselves look consistent after tracing the pruned complex type path from FE access paths through TabletSchema, SegmentIterator pruning, BetaRowsetReader, VMergeIteratorContext block creation, and AGG_KEYS reader aggregation.
Critical checkpoint conclusions:
- Goal/test proof: the code appears to implement the intended pruned complex-column merge/agg read fix, but the new regression does not reliably force one of the target paths because a separate fuzzy session gate is not pinned.
- Scope/focus: the implementation is focused on the block type mismatch and aggregate-function argument typing; no unrelated behavior or persistence format change found.
- Concurrency/lifecycle: no new shared-state race found. The pruned-type map pointer is backed by the rowset reader's StorageReadOptions TabletSchemaSPtr during iterator lifetime.
- Compatibility/persistence: no persisted schema or mixed-version serialization change found. Aggregate-state handling still routes through the existing BE exec-version compatibility path.
- Parallel paths: load, schema-change, vertical compaction, segcompaction, query BlockReader, and VMerge paths were checked. The remaining issue is test path selection, not source logic.
- Tests: BE UT covers pruned VMerge block shape; regression covers default TopN, inline TopN, and AGG_KEYS reads, but the inline TopN portion needs the segment-limit gate pinned. The existing line-100 thread already covers the separate missing num_segments > 1 assertion, so I did not duplicate it.
Validation was static only per the review prompt; I did not run builds or tests.
| // 2. Force the inline topn scan path (no VMaterializeNode). This is the exact | ||
| // path that used to fail with | ||
| // "[E-3110] Method insert_many_dict_data is not supported for TINYINT". | ||
| sql "SET topn_lazy_materialization_threshold = -1" |
Contributor
There was a problem hiding this comment.
This still does not fully force the repaired inline TopN/VMergeIterator path. The test disables TopN lazy materialization, but the storage ordered-key read is gated separately by enable_segment_limit_pushdown: OlapScanner only sets read_orderby_key/read_orderby_key_limit when that flag participates in can_push_down_segment_limit, and the variable is fuzzy/randomized in regression runs. If it is false, this DUP_KEYS scan keeps need_ordered_result=false, so BetaRowsetReader::is_merge_iterator() will not select VMergeIterator even if the rowset is overlapping and multi-segment, and these value checks can pass through the normal scan path. Please pin this section with SET enable_segment_limit_pushdown = true (or a SET_VAR hint) before the inline TopN assertions so the regression always exercises the path this fix targets.
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
from
26bb98e to
d6e68bf
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed. I found two regression-test issues that should be fixed before this lands: the deterministic SQL value checks should use qt/order_qt baselines with a generated .out file, and the suite should not drop the tables at the end after already dropping them before setup.
Implementation checkpoints: I did not find a correctness issue in the pruned complex-column type propagation into VMergeIterator, and the TabletSchema lifetime is held through the reader options on the production path. The aggregate-function type change also looks aligned: BlockReader now uses the actual stored/pruned block type, while load, vertical compaction, and schema-change paths use full-schema blocks and full-schema types. I did not identify new concurrency, storage lock-order, persistence, compatibility, or observability concerns in the changed implementation.
Test/duplicate context: existing thread r3544580724 still covers the missing num_segments > 1 proof, so I did not duplicate it. Existing thread r3567770409 appears resolved by SET enable_segment_limit_pushdown = true. No additional user focus was provided. Review was static only per the review-only runner instructions; I did not run builds or tests.
Contributor
TPC-H: Total hot run time: 29883 ms |
Contributor
TPC-DS: Total hot run time: 180666 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
…ators on merge/agg read paths
When a query prunes nested sub-columns of a complex column (EXPLAIN shows
"pruned type"), the scan's struct sub-column iterators are filtered down to
the pruned subset (set_access_paths + remove_pruned_sub_iterators), while the
FE-pruned data type is recorded only in TabletSchema::_pruned_columns_data_type
and honored only by TabletSchema::create_block*. Read paths that materialize
column shapes from other sources still used the FULL storage type, so the
positional pairing in StructFileColumnIterator::next_batch worked on two
different ordinal spaces:
1. VMergeIteratorContext::block_reset built per-segment merge blocks from the
storage Schema (full type). A topn ordered read over a rowset with multiple
OVERLAPPING segments then decoded pruned children into the wrong dst
columns:
- "[E-3110] Method insert_many_dict_data is not supported for TINYINT"
(dict-encoded text child decoded into a tinyint child)
- "... is not supported for INT" (single-child pruning variant)
- BE SIGSEGV when the shifted child types happen to match and the loop
runs past the pruned iterator vector (same family as DORIS-26595)
This is the root cause of DORIS-26889: the CI's s3 insert naturally
produces overlapping multi-segment rowsets, while single-segment local
loads never take the merge path. On master the default plan usually
shields this path via TopN lazy materialization (apache#63736 + apache#64242), but
e.g. raising topn_opt_limit_threshold, disabling
topn_lazy_materialization_threshold, or light_schema_change=false tables
expose it; branch-4.1 hits it by default.
2. BlockReader::_init_agg_state built the reader_replace argument type from
the raw TabletColumn (full type) while the stored block uses the pruned
type. On AGG tables any pruned read that goes through the aggregate merge
path (>= 2 data rowsets, no special session vars needed) failed with
"Aggregate function reader_replace argument 0 type check failed:
ColumnStruct has 2 elements, but DataTypeStruct has 3 elements".
Fix: make the shape-materializing read paths use the same FE-pruned type as
the sub-column iterators, so dst blocks and iterators stay in one ordinal
space by construction.
- VMergeIteratorContext::init reads the FE-pruned complex types from
StorageReadOptions::tablet_schema (TabletSchema::pruned_columns_data_type(),
already set on the query read path), and block_reset then builds each merge
block with the pruned type for pruned columns and the full storage type for
the rest, so the block's struct children align with the pruned sub-column
iterators. No new plumbing through the merge iterator or the storage Schema.
- TabletColumn::get_aggregate_function now takes the column's actual data type
explicitly. BlockReader::_init_agg_state passes the stored block's column
type (already the pruned type for pruned complex columns), while the
load/compaction/schema-change paths pass the full column type.
The new regression case constructs an OVERLAPPING multi-segment rowset
deterministically (MemTable.need_flush debug point + NULL keys in every
batch, segment count kept below the segcompaction threshold), self-checks the
layout via tablet meta, and covers all variants: the dict-error and
SIGSEGV topn shapes on the inline path, the AGG default-session shape across
two rowsets, and the VMaterializeNode default path as a guard. All variants
fail before this fix and pass after it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
from
d6e68bf to
3c43f4c
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Static review completed for the current patch. I did not find a new blocking issue to raise.
Checkpoint conclusions:
- Goal/test coverage: The change targets pruned nested complex-column reads where VMergeIterator and AGG reader aggregation previously built full-type blocks/functions while the segment sub-iterators had been pruned. The C++ fix aligns VMergeIterator block construction and AGG reader aggregate-function argument types with the actual pruned block type. Coverage now includes a focused BE unit test plus a regression test for inline TopN/VMergeIterator and AGG merge reads.
- Scope/minimality: The implementation is limited to aggregate-function type plumbing, merge iterator block materialization, and targeted tests.
- Concurrency/lifecycle: No new locks, threads, or persistent lifecycle states were introduced. The pruned-type map is held on the scanner's copied TabletSchema and is carried through reader options for the iterator lifetime.
- Compatibility/persistence: No storage-format, FE-BE protocol, or edit-log compatibility change is introduced. Non-query load, schema-change, and compaction paths continue to use full tablet-column types.
- Parallel paths: I checked query merge/union/direct behavior, AGG reader aggregation, MemTable load aggregation, schema-change MultiBlockMerger, vertical compaction, and aggregate-state UNION handling; no missed equivalent path was substantiated.
- Tests: The current regression addresses the existing review-thread concerns: it asserts OVERLAPPING plus num_segments > 1, pins enable_segment_limit_pushdown for the inline TopN path, uses qt_* output backed by the generated .out file, and avoids final table drops. No additional user focus was provided.
Validation limits: This was a static review only. Per the review prompt I did not run builds or tests; the checkout is also shallow and lacks .worktree_initialized/thirdparty/installed.
Contributor
TPC-H: Total hot run time: 29440 ms |
Contributor
TPC-DS: Total hot run time: 181615 ms |
Contributor
ClickBench: Total hot run time: 24.88 s |
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
3 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.
VMergeIteratorContext::block_reset built per-segment merge blocks from the storage Schema (full type). A topn ordered read over a rowset with multiple OVERLAPPING segments then decoded pruned children into the wrong dst columns
BlockReader::_init_agg_state built the reader_replace argument type from the raw TabletColumn (full type) while the stored block uses the pruned type. On AGG tables any pruned read that goes through the aggregate merge path (>= 2 data rowsets, no special session vars needed) failed with "Aggregate function reader_replace argument 0 type check failed: ColumnStruct has 2 elements, but DataTypeStruct has 3 elements".
What problem does this PR solve?
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)