[refactor](be) Avoid rebinding storage expression column ids#64010
Closed
BiteTheDDDDt wants to merge 11 commits into
Closed
[refactor](be) Avoid rebinding storage expression column ids#64010BiteTheDDDDt wants to merge 11 commits into
BiteTheDDDDt wants to merge 11 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Storage expression pushdown previously rebound slot refs to the expanded reader schema by mutating expression column ids inside segment iterators. That violates the assumption that pushed-down expressions are immutable and can produce surprising behavior when the same expression objects are reused. This change keeps the full scan schema unchanged, adds a project schema aligned with the final projected columns, and evaluates pushed-down expressions against a temporary project block. Slot ordinal to tablet column id mapping now uses the project schema, so no expression mutation is required.
### Release note
None
### Check List (For Author)
- Test: Manual test
- ./build.sh --be
- Behavior changed: No
- Does this need documentation: No
BiteTheDDDDt
force-pushed
the
codex/avoid-expr-rebind
branch
from
710a4c5 to
2e9588c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors BE storage expression pushdown to avoid mutating (rebinding) VSlotRef/VirtualSlotRef column ids inside segment iterators. Instead, it introduces a “project schema” representing the final scan columns (pre-expansion) and evaluates pushed-down expressions against a temporary project block whose column layout matches the original slot ordinals—so expressions can remain immutable even when the reader schema is expanded for AGG/merge readers.
Changes:
- Add
project_columnstoStorageReadOptionsand propagate it fromRowsetReaderContextinBetaRowsetReader. - Introduce
SegmentIterator::_project_schemaplus helper methods to map column ids to block positions and to build a per-batch project block. - Remove per-segment expression tree rebinding logic and switch common-expr / ANN / score paths to use the project schema.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| be/src/storage/segment/segment_iterator.h | Adds project-schema helpers and stores _project_schema in SegmentIterator. |
| be/src/storage/segment/segment_iterator.cpp | Implements project schema/block building and switches expr/ANN/virtual-column flows away from mutating expressions. |
| be/src/storage/rowset/beta_rowset_reader.cpp | Passes origin_return_columns/return_columns through to storage as project_columns. |
| be/src/storage/iterators.h | Extends StorageReadOptions with project_columns (pre-expansion projected scan columns). |
Comments suppressed due to low confidence (1)
be/src/storage/segment/segment_iterator.cpp:3557
- _materialization_of_virtual_column() rebuilds a full project_block for every virtual column expression. If there can be multiple virtual columns, this adds repeated block construction and column insertion overhead per batch. Consider building the project_block once per call (or caching/reusing it) and then executing each virtual expr against the same project_block.
LOG_WARNING("Result of expr column {} is empty. cid {}, idx_in_block {}",
column_expr->root()->debug_string(), cid, idx_in_block);
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
512
to
524
| if (column->size() != selected_size) { | ||
| return Status::InternalError("project column {} has {} rows, expected {}", cid, | ||
| column->size(), selected_size); | ||
| } | ||
| } | ||
| project_block->insert({std::move(column), type, _schema->column(cid)->name()}); | ||
| } | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| void SegmentIterator::_initialize_predicate_results() { | ||
| // Initialize from _col_predicates | ||
| for (auto pred : _col_predicates) { |
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 summary:
I did not find additional distinct issues beyond the existing inline thread already raised for _build_project_block() and lazy materialization placeholders. I did not submit duplicate inline comments.
Critical checkpoint conclusions:
- Goal and tests: The PR appears intended to evaluate storage-side common/virtual expressions against the final scan projection layout while preserving expanded storage read schemas. The code generally follows that direction. I did not see new/modified tests in the PR, so coverage for non-direct AGG/UNIQUE paths with common expr pushdown, virtual columns, and ANN/score columns remains a residual risk.
- Scope and clarity: The change is focused on projection/schema mapping inside rowset and segment reading. The replacement of expression rebinding with project-block construction is understandable, though it increases reliance on correct project column coverage.
- Concurrency: The touched paths are per-reader/per-segment iterator execution paths; I did not identify new shared mutable state, lock ordering, or thread-safety issues.
- Lifecycle: No new static/global lifecycle or ownership issues were identified. The project column pointer is propagated from reader context vectors that outlive segment iterator initialization/use in the reviewed paths.
- Configuration and compatibility: No new configuration, storage format, EditLog, or wire-protocol compatibility changes were introduced.
- Parallel paths: Both direct and non-direct rowset reader paths were considered through
origin_return_columns/return_columns; I did not find an additional missing propagation path. - Error handling: New
Statusreturns are checked by callers. The existing already-known concern about non-materialized projected columns is not repeated here. - Transaction/data correctness: The change does not modify transaction visibility, delete bitmap versioning, rowset lifecycle, or persistence paths.
- Performance and observability: The project block adds extra block construction during common expr and virtual column materialization; this is localized. I did not find an obvious pathological regression, but test/benchmark coverage would be useful for wide projected schemas.
User focus: No additional user-provided review focus was specified, and I found no extra focus-specific issue.
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 23735 ms |
Contributor
TPC-DS: Total hot run time: 169276 ms |
Contributor
TPC-H: Total hot run time: 23569 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-DS: Total hot run time: 169035 ms |
Contributor
TPC-H: Total hot run time: 29593 ms |
Contributor
TPC-DS: Total hot run time: 169887 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes. I found one blocking issue in the virtual-column materialization fallback.
Critical checkpoint conclusions:
- Goal/test: The PR appears intended to evaluate storage pushdown and virtual column expressions against the final projected scan schema while avoiding per-expression rebinding. The changed flow mostly follows that goal, but the virtual expression fallback loses an exception boundary. I did not find new tests in the PR that cover this fallback/error path.
- Scope: The change is focused on segment iterator projection/schema mapping.
- Concurrency/lifecycle: No new shared mutable state, lock ordering, or special lifecycle concerns found in the changed paths.
- Config/compatibility/protocol: No new configs, storage formats, or FE-BE protocol fields found.
- Parallel paths: I checked common expr pushdown, ANN/score virtual materialization, and virtual projection materialization. The reported issue is specific to the virtual projection fallback replacing VExprContext::execute().
- Error handling: Blocking issue found: expression exceptions can now escape instead of being converted to Status in _materialization_of_virtual_column().
- Data correctness/storage invariants: No transaction, visible-version, delete-bitmap, or rowset lifecycle changes found.
- Performance/observability: No blocking performance or observability issue found; the additional projected block construction is localized to expression evaluation paths.
- Test coverage: The PR should add or update coverage for virtual column materialization through the projected schema, including an expression that can throw and should return query failure Status rather than escape.
Existing review context: I read the existing inline thread about _build_project_block() and non-materialized projected columns and did not duplicate it.
User focus: No additional user-provided review focus was specified.
HappenLee
reviewed
Jun 5, 2026
| return Status::OK(); | ||
| } | ||
|
|
||
| Status SegmentIterator::_init_project_schema() { |
Contributor
There was a problem hiding this comment.
_schema_block_id_map.assign(_schema->columns().size(), -1);
for (int i = 0; i < _schema->num_column_ids(); i++) {
auto cid = _schema->column_id(i);
_schema_block_id_map[cid] = i;
}
这个初始化的代码在_vec_init_lazy_materialization也会被调用，需要统一一下
HappenLee
reviewed
Jun 5, 2026
| ColumnPtr result_column; | ||
| RETURN_IF_ERROR(column_expr->execute(block, result_column)); | ||
| Block project_block; | ||
| RETURN_IF_ERROR(_build_project_block(block, &project_block)); |
Contributor
There was a problem hiding this comment.
这个是不是应该放到循环外面，如果有多个虚拟列表达式这个要搞好几遍？
HappenLee
reviewed
Jun 5, 2026
| } | ||
|
|
||
| Status SegmentIterator::_build_project_block(Block* block, Block* project_block) { | ||
| project_block->clear(); |
Contributor
There was a problem hiding this comment.
这个现在普通的dup key表，MOW表也会走到这里，这里应该加一个快速路径的判断，就是如果发现这个project schema和普通schema一样的话，就直接返回block吧
### What problem does this PR solve? Issue Number: None Related PR: apache#64010 Problem Summary: Address review comments for avoiding storage expression column id rebinding. The project-schema column-id to block-position map was initialized in multiple places, and project-block construction still ran on ordinary scan paths where the project schema matched the reader schema. Virtual column materialization also rebuilt the project block for each expression. This change centralizes schema block mapping initialization, reuses the original block when the project schema is identical to the reader schema, reuses the projected block during virtual column materialization, and keeps expression exceptions converted to Status at the virtual-column evaluation boundary. ### Release note None ### Check List (For Author) - Test: Unit Test - ./build.sh --be - build-support/run-clang-tidy.sh attempted; it reported pre-existing/analysis diagnostics in segment_iterator.cpp, segment_iterator.h, and be/src/util/jni-util.h unrelated to the changed logic. - Behavior changed: No - Does this need documentation: No
yiguolei
reviewed
Jun 10, 2026
Contributor
TPC-H: Total hot run time: 29531 ms |
Contributor
There was a problem hiding this comment.
Review result: request changes.
I did not open duplicate new inline threads for the already-known origin_return_columns / project_columns nullability concern. Instead, I replied to the existing threads with concrete production paths that still fail on the current PR head:
- Direct
Segment::new_iterator()callers such asbe/src/storage/task/index_builder.cpp:563-570andbe/src/storage/rowset/segcompaction.cpp:93-115constructStorageReadOptionswithoutproject_columns, thenSegmentIterator::_init_project_schema()hitsDORIS_CHECK(_opts.project_columns != nullptr). - Direct
RowsetReaderContextcallers such asbe/src/storage/schema_change/schema_change.cpp:1057-1076andbe/src/cloud/cloud_schema_change_job.cpp:258-280setreturn_columnsbut notorigin_return_columns, thenBetaRowsetReader::get_segment_iterators()hitsDORIS_CHECK(_read_context->origin_return_columns != nullptr).
Critical checkpoint conclusions:
- Goal and test proof: The PR goal, avoiding storage expression column-id rebinding by evaluating against project-schema layout, is reasonable. The current implementation is not complete because project column metadata is not propagated to all production iterator entry points. No PR tests cover these direct iterator/schema-change paths.
- Scope and clarity: The changed code is focused, but it changes a core storage read invariant now required by all segment iterator users.
- Concurrency: No new shared mutable state, lock ordering, or thread-safety issue found.
- Lifecycle: No new static/global lifecycle issue found. Pointer lifetimes are acceptable where set, but several direct paths never set the pointer.
- Configuration and compatibility: No new configuration, storage format, EditLog, or FE-BE protocol compatibility issue found.
- Parallel paths: This is the blocking area. Query
TabletReaderpaths mostly propagateorigin_return_columns, but directSegment::new_iterator()and directRowsetReaderContextpaths are not updated. - Conditional checks/error handling: The new
DORIS_CHECKassertions are valid only after all callers are updated. As written, normal schema change, cloud schema change, index build, and segment compaction paths can crash/fail before doing useful work. - Data correctness: No transaction visibility, delete bitmap versioning, or persistence change was found. The blocker is availability/correct execution of read paths.
- Memory/performance/observability: No additional blocking memory-tracking or observability issue found. Project-block overhead is localized and secondary to the propagation bug.
- Tests/results:
git diff --checkpassed for the reviewed files. GitHub macOS BE UT failed before tests due runner JDK 25 vs required JDK 17, so it does not prove this PR behavior. Linux/TeamCity checks were still pending when reviewed.
User focus: .code-review.0DYI4r/review_focus.txt contained no additional user-provided focus points.
Contributor
TPC-DS: Total hot run time: 169508 ms |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64010 Problem Summary: Review feedback pointed out that the segment iterator should work from an explicit project schema, and that the project block helper should fill caller-owned storage instead of returning a selected block pointer. Follow-up coverage showed that direct RowsetReader and Segment::new_iterator callers do not pass the TabletReader-expanded project schema fields. The reader now keeps the SegmentIterator project_columns invariant, while BetaRowsetReader maps direct callers to return_columns and Segment::new_iterator maps direct callers to schema column IDs. The project block helper remains caller-owned and the ANN source column ordinal translation is documented. ### Release note None ### Check List (For Author) - Test: Unit Test - build-support/clang-format.sh - git diff --check - ./run-be-ut.sh --run --filter=TestDeltaWriterClusterKey.vec_sequence_col - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
force-pushed
the
codex/avoid-expr-rebind
branch
from
b450ca1 to
edcdc78
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29113 ms |
Contributor
There was a problem hiding this comment.
Review summary
I reviewed the current five-file BE storage refactor around project schema plumbing, common expression evaluation, and virtual column materialization. I also read the existing inline review threads and did not find a new non-duplicate blocking issue to raise inline; the remaining suspicious virtual/output block-layout discussion is already covered by the existing thread, so I did not repeat it.
Critical checkpoint conclusions:
- Goal and tests: The PR goal is to avoid rebinding storage expression column ids by evaluating pushed-down expressions against a projected schema. The current patch wires
project_columnsthrough normal TabletReader paths, direct RowsetReaderContext paths, and Segment::new_iterator fallbacks. I did not run local tests in this review pass; the visible BE UT macOS CI failure appears to be an environment issue (JAVA version is 25, required JDK 17), not this code path. - Scope and simplicity: The change is scoped to rowset/segment iterator plumbing and does not introduce a new broad abstraction beyond the projected-schema helper path.
- Data correctness: I checked the non-direct aggregation path, direct schema-change/index/segcompaction paths, and virtual column materialization path. The current
_build_project_blockmaps projected column ids back through_schema_block_id_map, which addresses the ordinal-mismatch class already discussed in earlier comments. - Concurrency and lifecycle: No new shared mutable state or locking behavior was introduced. The
project_columnspointer is consumed during iterator initialization to build_project_schema, and the Segment::new_iterator fallback points at the schema owned by the iterator. - Error handling and memory: Status/exception boundaries are preserved in the pushed-down expression path, including the virtual
execute_columncall. I did not identify a new allocation or ownership issue. - Compatibility and persistence: No storage format, RPC, FE/BE protocol, config, or persistence compatibility change was found.
- Performance: The fast path avoids building a projected block when the projected schema matches the reader schema; otherwise the extra block is limited to expression evaluation/materialization paths.
- Observability: No additional logging or metrics seem required for this refactor.
- User focus: The focus file contained no additional user-provided review focus beyond the full PR review.
Contributor
TPC-DS: Total hot run time: 169905 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
yiguolei
reviewed
Jun 11, 2026
| @@ -253,19 +253,23 @@ Status Segment::new_iterator(SchemaSPtr schema, const StorageReadOptions& read_o | |||
| if (read_options.runtime_state != nullptr) { | |||
| _be_exec_version = read_options.runtime_state->be_exec_version(); | |||
| } | |||
| RETURN_IF_ERROR(_create_column_meta_once(read_options.stats)); | |||
| auto iterator_read_options = read_options; | |||
yiguolei
reviewed
Jun 11, 2026
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29344 ms |
Contributor
TPC-DS: Total hot run time: 168921 ms |
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 11, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64010 Problem Summary: Address review comments for avoiding storage expression column id rebinding. The project-schema column-id to block-position map was initialized in multiple places, and project-block construction still ran on ordinary scan paths where the project schema matched the reader schema. Virtual column materialization also rebuilt the project block for each expression. This change centralizes schema block mapping initialization, reuses the original block when the project schema is identical to the reader schema, reuses the projected block during virtual column materialization, and keeps expression exceptions converted to Status at the virtual-column evaluation boundary. ### Release note None ### Check List (For Author) - Test: Unit Test - ./build.sh --be - build-support/run-clang-tidy.sh attempted; it reported pre-existing/analysis diagnostics in segment_iterator.cpp, segment_iterator.h, and be/src/util/jni-util.h unrelated to the changed logic. - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 11, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64010 Problem Summary: Review feedback pointed out that the segment iterator should work from an explicit project schema, and that the project block helper should fill caller-owned storage instead of returning a selected block pointer. Follow-up coverage showed that direct RowsetReader and Segment::new_iterator callers do not pass the TabletReader-expanded project schema fields. The reader now keeps the SegmentIterator project_columns invariant, while BetaRowsetReader maps direct callers to return_columns and Segment::new_iterator maps direct callers to schema column IDs. The project block helper remains caller-owned and the ANN source column ordinal translation is documented. ### Release note None ### Check List (For Author) - Test: Unit Test - build-support/clang-format.sh - git diff --check - ./run-be-ut.sh --run --filter=TestDeltaWriterClusterKey.vec_sequence_col - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 12, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64010 Problem Summary: Address review comments for avoiding storage expression column id rebinding. The project-schema column-id to block-position map was initialized in multiple places, and project-block construction still ran on ordinary scan paths where the project schema matched the reader schema. Virtual column materialization also rebuilt the project block for each expression. This change centralizes schema block mapping initialization, reuses the original block when the project schema is identical to the reader schema, reuses the projected block during virtual column materialization, and keeps expression exceptions converted to Status at the virtual-column evaluation boundary. ### Release note None ### Check List (For Author) - Test: Unit Test - ./build.sh --be - build-support/run-clang-tidy.sh attempted; it reported pre-existing/analysis diagnostics in segment_iterator.cpp, segment_iterator.h, and be/src/util/jni-util.h unrelated to the changed logic. - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 12, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64010 Problem Summary: Review feedback pointed out that the segment iterator should work from an explicit project schema, and that the project block helper should fill caller-owned storage instead of returning a selected block pointer. Follow-up coverage showed that direct RowsetReader and Segment::new_iterator callers do not pass the TabletReader-expanded project schema fields. The reader now keeps the SegmentIterator project_columns invariant, while BetaRowsetReader maps direct callers to return_columns and Segment::new_iterator maps direct callers to schema column IDs. The project block helper remains caller-owned and the ANN source column ordinal translation is documented. ### Release note None ### Check List (For Author) - Test: Unit Test - build-support/clang-format.sh - git diff --check - ./run-be-ut.sh --run --filter=TestDeltaWriterClusterKey.vec_sequence_col - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#64010 Problem Summary: Address review comments for avoiding storage expression column id rebinding. The project-schema column-id to block-position map was initialized in multiple places, and project-block construction still ran on ordinary scan paths where the project schema matched the reader schema. Virtual column materialization also rebuilt the project block for each expression. This change centralizes schema block mapping initialization, reuses the original block when the project schema is identical to the reader schema, reuses the projected block during virtual column materialization, and keeps expression exceptions converted to Status at the virtual-column evaluation boundary. ### Release note None ### Check List (For Author) - Test: Unit Test - ./build.sh --be - build-support/run-clang-tidy.sh attempted; it reported pre-existing/analysis diagnostics in segment_iterator.cpp, segment_iterator.h, and be/src/util/jni-util.h unrelated to the changed logic. - Behavior changed: No - Does this need documentation: No
BiteTheDDDDt
added a commit
to BiteTheDDDDt/incubator-doris
that referenced
this pull request
Jun 13, 2026
Issue Number: N/A Related PR: apache#64010 Problem Summary: Review feedback pointed out that the segment iterator should work from an explicit project schema, and that the project block helper should fill caller-owned storage instead of returning a selected block pointer. Follow-up coverage showed that direct RowsetReader and Segment::new_iterator callers do not pass the TabletReader-expanded project schema fields. The reader now keeps the SegmentIterator project_columns invariant, while BetaRowsetReader maps direct callers to return_columns and Segment::new_iterator maps direct callers to schema column IDs. The project block helper remains caller-owned and the ANN source column ordinal translation is documented. None - Test: Unit Test - build-support/clang-format.sh - git diff --check - ./run-be-ut.sh --run --filter=TestDeltaWriterClusterKey.vec_sequence_col - Behavior changed: No - 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
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: N/A
Related PR: N/A
Problem Summary: Storage expression pushdown previously rebound slot refs to the expanded reader schema by mutating expression column ids inside segment iterators. That violates the assumption that pushed-down expressions are immutable and can produce surprising behavior when the same expression objects are reused. This change keeps the full scan schema unchanged, adds a project schema aligned with the final projected columns, and evaluates pushed-down expressions against a temporary project block. Slot ordinal to tablet column id mapping now uses the project schema, so no expression mutation is required.
Release note
None
Check List (For Author)