[refactor](be) Unify storage reader column ordinals - #66472
Open
csun5285 wants to merge 5 commits into
Open
Conversation
csun5285
requested review from
924060929,
airborne12,
deardeng,
eldenmoon,
englefly,
gavinchou,
liaoxin01,
luwei16,
morrySnow,
starocean999 and
yiguolei
as code owners
August 5, 2026 06:26
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
csun5285
force-pushed
the
refactor/reader-return-columns
branch
2 times, most recently
from
August 7, 2026 12:27
6a3b439 to
768a009
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29131 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 158453 ms |
Contributor
ClickBench: Total hot run time: 23.85 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 9, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66472 Problem Summary: The reader-schema refactor renamed storage block and schema APIs, but several BE tests still used the old interfaces. The FE translator test also left row-binlog index replicas behind the partition version, and one DATETIMEV2 fixture omitted its scale. Update these test fixtures so the refactored reader code is compiled and exercised correctly. ### Release note None ### Check List (For Author) - Test: Unit Test and build - ASAN BE and FE build - PhysicalPlanTranslatorTest: 13 tests passed - Reader-related BE UT: 71 tests passed - clang-format and clang-tidy passed - Behavior changed: No - Does this need documentation: No
csun5285
force-pushed
the
refactor/reader-return-columns
branch
from
August 9, 2026 14:20
768a009 to
aff0dbe
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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.
Requesting changes for two P1 correctness/compatibility blockers in the new FE-authored ReadSchema contract.
Critical checkpoints:
- Goal and tests: The ordinal unification is coherent on most current-FE paths, but the added tests primarily assert descriptor shape; they do not prove lazy scan outcomes or mixed-version execution.
- Scope: Reviewed the complete 124-file authoritative diff and the upstream/downstream FE scan, BE reader, merge, index, schema-change, compaction, cloud, binlog, snapshot, rowid, and test paths.
- Concurrency: No new race was substantiated; shared read-schema mutation is copy-before-publish and occurs before rowset-reader context capture.
- Lifecycle: Delete-predicate suffix construction, reader initialization, segment state sizing, and visible-prefix output boundaries are coherent.
- Configuration: The lazy bug is reachable under the existing TopN lazy-materialization threshold; no separate configuration/default regression was found.
- Compatibility: Blocking issue inline: an older FE can send the same accepted execution version while omitting dependencies that this BE now assumes are present.
- Parallel paths: Query/direct readers, vertical readers, schema change, compaction, cloud, inverted/ANN, and rowid paths were traced; the distinct missed parallel path is lazy scan translation.
- Conditions: Key/value predicate ordinal binding, TSO predicate creation, condition-cache fencing, and historical delete conditions were checked; no additional issue survived.
- Test coverage/results: No local build or tests were run, as required by the review environment. Visible CI has successful BE UT (macOS), CheckStyle, Clang Formatter, license, secrets, and dependency checks; other displayed jobs are skipped, and this automated review status is pending until submission.
- Observability: The ReadSchema profile output is useful and no logging/profile correctness issue was found.
- Persistence/transactions: The runtime-only schema object does not alter persisted metadata; rowset versions, delete history, snapshot fences, and transaction-facing row images were reviewed.
- Data writes: Writer, memtable, compaction, and rowid-conversion edits are mechanical ordinal/schema migrations with no substantiated write-path defect.
- FE/BE variables:
be_exec_version=11does not encode the new tuple-completeness capability, which is the compatibility blocker noted inline. - Performance: No clear performance regression was found in the ordinal maps, column state, or reader construction changes.
- Other issues: Blocking issue inline: lazy translation can remove keys and snapshot/binlog helper slots before BE constructs ReadSchema.
User focus: review_focus.txt contained no additional focus beyond the full PR review.
Review completion: The full-scope and risk-focused passes converged in Round 2 with no unresolved candidates.
| .collect(Collectors.toSet()); | ||
|
|
||
| olapScanNode.updateRequiredSlots(context, scanIds); | ||
| preserveMergeSequenceSlots(olapScanNode, scanIds); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the full storage dependency set for lazy scans
This second pruning pass bypasses the normal calls to preserveStorageSemanticSlots() and preserveExtraStorageKeySlots(). For the new test query, k1/k2 are lazy, so the scan descriptor keeps v1/sequence/rowid but drops the key prefix; BE then builds a ReadSchema with zero key columns and can merge distinct MOR keys as equal. Lazy snapshot/binlog scans similarly lose commit-TSO or TSO/op/before slots and fail initialization. Apply the same two preservation steps used by updateScanSlotsMaterialization before removing tuple slots.
| return Status::InternalError("failed to build storage scanner, no materialized slot!"); | ||
| } | ||
|
|
||
| // The FE physical scan tuple is the read-path schema. It already includes |
Contributor
There was a problem hiding this comment.
[P1] Keep a fallback for plans from older FEs
This assumes every accepted FE plan already carries the new hidden dependencies, but the PR removes the BE-side expansion that older FEs rely on and adds no thrift/version handshake. The immediately prior FE emits the same be_exec_version=11, so its ordinary plans can reach this BE without sequence/map, commit-TSO, or row-binlog helper slots; the BE then silently chooses the wrong MOR winner or fails _init_tso_predicates/init_sequence_map. Retain legacy dependency expansion (with ordinal remapping), or add an explicit plan capability marker before treating the tuple as authoritative.
csun5285
changed the title
Contributor
|
run buildall |
yiguolei
reviewed
Aug 10, 2026
| .ann_topn_runtime {}, | ||
| .condition_cache_digest = parent->get_condition_cache_digest(), | ||
| .binlog_scan_type = params.binlog_scan_type, | ||
| .start_tso = std::nullopt, |
Contributor
Author
There was a problem hiding this comment.
不需要往下层传递了，这一层就构建出 tso 的pred 了
yiguolei
reviewed
Aug 10, 2026
| // Scanner will be executed in a different thread, so we need to clone the context. | ||
| VExprContextSPtr context; | ||
| RETURN_IF_ERROR(expr_it->second->clone(_state, context)); | ||
| _virtual_column_exprs[ordinal] = std::move(context); |
Contributor
There was a problem hiding this comment.
如果这个_virtual_column_exprs 的key 是columnid，这里直接从0 开始，感觉跟之前的tablet column的id 重合了
yiguolei
reviewed
Aug 10, 2026
| olap_scan_local_state->get_topn_filter_source_node_ids(_state, true); | ||
| if (!_tablet_reader_params.topn_filter_source_node_ids.empty()) { | ||
| _tablet_reader_params.topn_filter_target_node_id = | ||
| olap_scan_local_state->parent()->node_id(); |
yiguolei
reviewed
Aug 10, 2026
| RETURN_IF_ERROR(pair.second->clone(_state, context)); | ||
| _slot_id_to_virtual_column_expr[pair.first] = context; | ||
| for (uint32_t ordinal = 0; auto* slot : _output_tuple_desc->slots()) { | ||
| if (slot->get_virtual_column_expr()) { |
Contributor
There was a problem hiding this comment.
把这个方法移动到tuple descriptor 里
yiguolei
reviewed
Aug 10, 2026
| _tablet_reader_params.tso_predicate_column_id = static_cast<ColumnId>(tso_index); | ||
|
|
||
| // The scan-node digest does not contain the per-range TSO bounds. | ||
| _tablet_reader_params.condition_cache_digest = 0; |
Contributor
Author
There was a problem hiding this comment.
在这个where 条件下 condition_cache 失效
yiguolei
reviewed
Aug 10, 2026
|
|
||
| protected: | ||
| const TabletSchema& _schema; | ||
| ReadSchemaSPtr _schema; |
csun5285
force-pushed
the
refactor/reader-return-columns
branch
from
August 10, 2026 14:42
aff0dbe to
990e3f8
Compare
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66472 Problem Summary: The reader-schema refactor renamed storage block and schema APIs, but several BE tests still used the old interfaces. The FE translator test also left row-binlog index replicas behind the partition version, and one DATETIMEV2 fixture omitted its scale. Update these test fixtures so the refactored reader code is compiled and exercised correctly. ### Release note None ### Check List (For Author) - Test: Unit Test and build - ASAN BE and FE build - PhysicalPlanTranslatorTest: 13 tests passed - Reader-related BE UT: 71 tests passed - clang-format and clang-tidy passed - Behavior changed: No - Does this need documentation: No
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66472 Problem Summary: Reader schemas can require storage-only columns that are absent from the SQL output, including historical columns used by delete predicates, row-binlog semantic columns, and merge sequence columns. Losing those dependencies can bind predicates to the wrong schema or materialize a row selected without the correct sequence semantics. Preserve and validate those dependencies, disable TopN lazy materialization for sequence-map tables, add focused coverage, and update storage tests to use the explicit storage-block API. ### Release note Fix hidden storage-column handling for reader schemas, row-binlog scans, historical delete predicates, and TopN lazy materialization. ### Check List (For Author) - Test: - Unit Test: PhysicalPlanTranslatorTest, DeleteHandler tests, ValidateStageTest, VariantRowStoreTest - Regression test: test_alter_table_column_with_delete_drop_column_dup_key - Build: BE and FE - Behavior changed: Yes. Hidden storage dependencies are retained and sequence-map TopN lazy materialization is disabled. - Does this need documentation: No
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66472 Problem Summary: Reader predicates and historical delete columns were still wired through transitional TabletReader-only helpers after read-schema ordinals were unified. Normalize LIKE predicates once in the scanner, preserve per-clone LIKE execution state, remove obsolete FunctionFilter factories, and append dropped delete-predicate columns in every TabletReader and schema-change path so predicate ordinals remain aligned with ReadSchema. ### Release note None ### Check List (For Author) - Test: - Unit Test: PhysicalPlanTranslatorTest, DeleteHandler tests, ValidateStageTest, VariantRowStoreTest - Regression test: test_alter_table_column_with_delete_drop_column_dup_key - Build: BE and FE - Behavior changed: No. This completes the internal reader schema and predicate ownership refactoring. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: the full review of all 141 changed paths converged with no new distinct blocking finding, so this review adds zero inline comments. The existing P1 about old-FE/new-BE rolling-upgrade compatibility remains tracked in its current thread and is intentionally not duplicated here; the previously reported lazy-scan slot-preservation omission is fixed at this head.
Critical checkpoint conclusions:
- Goal and proof: The change replaces sparse tablet-CID read layouts with an ordered
ReadSchemawhose visible prefix exactly matches caller Blocks, while FE preserves the hidden key, sequence/sequence-map, snapshot, row-binlog, and before-image dependencies that storage needs. The production path and the added BE/FE/result-bearing regression tests are consistent with that goal. - Focus and minimality: The diff is broad because the coordinate and Block-construction contract crosses the scanner, rowset, iterator, compaction, schema-change, index, and test layers. The semantic changes are concentrated in schema ownership, slot preservation, read ordinals, and historical-delete resolution; no unrelated behavioral addition survived review.
- Concurrency: Read schemas are completed during initialization and then consumed through owning/shared references; per-scanner expressions, predicates, and LIKE search scratch are cloned rather than concurrently mutated. No new thread entry, lock ordering, atomic, race, or deadlock concern was introduced.
- Lifecycle and static initialization:
ReadSchemaSPtrownership extends through lazy segment, rowset, union, and merge iterators, while direct-reader local schemas outlive their iterators. Historical suffix columns live in the owning schema, and no reference cycle, premature release, or cross-TU static-initialization dependency was found. - Configuration: No production configuration item or dynamic-update contract was added. FE tests save and restore the global binlog/stream flags they modify.
- Compatibility: No storage format, persisted metadata, or new thrift field is introduced. The existing rolling-upgrade concern—an older FE does not send the newly authoritative hidden dependency set to this BE—is already covered by the live P1 thread and remains the compatibility conclusion for this checkpoint.
- Parallel paths: Ordinary and lazy OLAP scans, snapshot wrappers, APPEND_ONLY/MIN_DELTA/DETAIL row binlog, simple sequence and sequence maps, raw binlog TVF, local/cloud schema change, horizontal/vertical compaction, segcompaction, checksum, index builder, historical-row fetch, ANN/inverted-index/virtual-slot paths, and empty/statistics iterators were traced. No stale production coordinate path was found.
- Conditional logic: Key-merge versus direct/union mode, visible-prefix versus delete-only suffix columns, sequence-map eligibility, single-version TSO replacement, predicate/common-expression/output role selection after zonemap or inverted-index elimination, nullable conversions, zero-row handling, and EOF restoration preserve their documented invariants.
- Test coverage: Added ReadSchema, descriptor, DeleteHandler, merge/binlog, iterator-role, ANN, translator, and materialization tests cover the changed contracts. Ordered regressions exercise value-only row-binlog projections, hidden sequence-map dependencies, lazy-materialization rejection for sequence maps, and dropped/re-added delete columns across schema changes. No concrete uncovered PR-introduced failure remained.
- Test results: The changed expected outputs match the reviewed queries and use deterministic ordering. Per the automated review instructions, tests and builds were not run in this review, so no execution result is claimed.
- Observability: Read-schema/profile labels and iterator-phase counters remain coherent, while merge-contract and initialization errors retain tablet, rowset, version, and schema-width context. No new distributed or silent critical path required an additional metric.
- Transactions and persistence: The change does not modify EditLog, transaction state, publish ordering, rowset commit protocols, version visibility, or persisted schema encoding. Historical delete predicates continue to bind to their creating rowset schema.
- Data writes: Writer-side edits select explicit physical storage Blocks; partial update, row-binlog writing, compaction, and schema-change output retain their existing atomic rowset/writer lifecycles and visible Block widths. No new crash window or write-order issue was found.
- FE/BE variables: Existing tuple slots carry order, UID, type, and nullability into one read-ordinal coordinate system used by predicates, access paths, virtual/ANN expressions, merge comparators, and output projection. No new transmitted variable was added; the old-FE compatibility caveat is the already-tracked exception above.
- Performance: Exact Block prefixes avoid materializing historical delete-only suffixes, deferred output reads remain lazy, adaptive sizing counts visible columns, and semantic slots are retained only when required. Conservative optimization disables were reviewed and no correctness-affecting or obvious hot-path regression survived.
- Other safety checks: Status propagation, exception conversion, nullable/constant-column boundaries, merge-contract assertions, and shared-schema mutation timing were reviewed. One row-binlog TSO suspicion was reconstructed against the merge base and dismissed because its complete enabling path predates this PR.
User focus: no additional review focus was provided; the whole PR was reviewed.
Contributor
TPC-H: Total hot run time: 29450 ms |
Contributor
TPC-DS: Total hot run time: 158113 ms |
Contributor
ClickBench: Total hot run time: 23.83 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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. Fix inconsistent storage-reader column mappings for projected and row-binlog scans. - 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
### What problem does this PR solve? Issue Number: None Related PR: apache#66472 Problem Summary: The reader-schema refactor renamed storage block and schema APIs, but several BE tests still used the old interfaces. The FE translator test also left row-binlog index replicas behind the partition version, and one DATETIMEV2 fixture omitted its scale. Update these test fixtures so the refactored reader code is compiled and exercised correctly. ### Release note None ### Check List (For Author) - Test: Unit Test and build - ASAN BE and FE build - PhysicalPlanTranslatorTest: 13 tests passed - Reader-related BE UT: 71 tests passed - clang-format and clang-tidy passed - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66472 Problem Summary: Reader schemas can require storage-only columns that are absent from the SQL output, including historical columns used by delete predicates, row-binlog semantic columns, and merge sequence columns. Losing those dependencies can bind predicates to the wrong schema or materialize a row selected without the correct sequence semantics. Preserve and validate those dependencies, disable TopN lazy materialization for sequence-map tables, add focused coverage, and update storage tests to use the explicit storage-block API. ### Release note Fix hidden storage-column handling for reader schemas, row-binlog scans, historical delete predicates, and TopN lazy materialization. ### Check List (For Author) - Test: - Unit Test: PhysicalPlanTranslatorTest, DeleteHandler tests, ValidateStageTest, VariantRowStoreTest - Regression test: test_alter_table_column_with_delete_drop_column_dup_key - Build: BE and FE - Behavior changed: Yes. Hidden storage dependencies are retained and sequence-map TopN lazy materialization is disabled. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66472 Problem Summary: Reader predicates and historical delete columns were still wired through transitional TabletReader-only helpers after read-schema ordinals were unified. Normalize LIKE predicates once in the scanner, preserve per-clone LIKE execution state, remove obsolete FunctionFilter factories, and append dropped delete-predicate columns in every TabletReader and schema-change path so predicate ordinals remain aligned with ReadSchema. ### Release note None ### Check List (For Author) - Test: - Unit Test: PhysicalPlanTranslatorTest, DeleteHandler tests, ValidateStageTest, VariantRowStoreTest - Regression test: test_alter_table_column_with_delete_drop_column_dup_key - Build: BE and FE - Behavior changed: No. This completes the internal reader schema and predicate ownership refactoring. - Does this need documentation: No
Issue Number: None Related PR: apache#65396 Problem Summary: ReadSchema keeps full physical TabletColumn metadata while its read type may contain a pruned complex-column shape. Aggregate readers previously created reader_replace from the physical type, so pruned Struct blocks could fail type validation. Use the ReadSchema type when constructing reader aggregate functions while preserving the physical-type overload for storage callers. Add the overlapping-segment and aggregate-reader regression coverage from apache#65396. Also initialize the scan-normalization test tuple descriptor required by read-schema ordinal binding. Fix queries that aggregate pruned complex columns across rowsets. - Test: - ASAN BE build - Regression test: fault_injection_p0/test_topn_pruned_struct_overlapping_segments - Regression test: schema_change_p0/test_schema_change_unique_mow (3 consecutive runs) - Behavior changed: Yes. Aggregate readers now bind functions to the materialized read type. - Does this need documentation: No
csun5285
force-pushed
the
refactor/reader-return-columns
branch
from
August 11, 2026 05:00
2383f16 to
4952478
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29454 ms |
Contributor
TPC-DS: Total hot run time: 158086 ms |
Contributor
ClickBench: Total hot run time: 24.03 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
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)