[opt](exec) Align operator projection with scanner (without copy when do project) - #66085
[opt](exec) Align operator projection with scanner (without copy when do project)#66085Mryange wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary — REQUEST_CHANGES
I found five distinct issues in the generic operator projection rewrite: supported nullable widening can now fail, runtime slot columns can be published under an incompatible retained DataType, projection allocations no longer inform the workload-group reservation gate, pass-through slot projections clone while discarding reusable output capacity, and an empty intermediate stage can silently drop nonempty rows in release builds.
Critical checkpoints:
- Goal and proof: the intended no-copy alignment is not achieved for slot refs, and the PR includes no targeted ownership/reuse/reservation test or benchmark.
- Correctness and error handling: two P1 type/nullability contract regressions and one P2 release-mode invariant regression are detailed inline.
- Lifecycle and concurrency: there is no new shared-state or lock-order issue, but the COW owner lifecycle/output-block reuse is incorrect, and the pipeline memory-pressure scheduler is under-informed.
- Compatibility and parallel paths: there are no new FE-BE fields, storage-format, configuration, persistence, or data-write changes. The scanner analogue does not establish that the generic operator contracts are safe, and malformed/compatibility thrift input still requires runtime rejection.
- Performance and memory: slot-ref projections allocate and deep-copy on each reused block, while the new allocations are absent from the next-round reservation estimate.
- Tests: no tests are added and the PR checklist is entirely unchecked. Formatting/title/static repository checks are green; BE UT was still pending at review time. Per the review-run contract, I did not run builds or tests locally.
- Observability/security: no security-sensitive behavior is changed; the new generic
Nullable mismatchexception is also less diagnosable than the removed contextual failures. - User focus: no additional focus was provided, so the whole PR was reviewed.
The _keep_origin field removal itself has no remaining callers and is safe, but do_projections() is not ready to merge until the inline issues are addressed with focused correctness, ownership/reuse, and reservation coverage. Two complete review rounds converged, with all three independent Round 2 reviewers returning NO_NEW_VALUABLE_FINDINGS for this frozen five-comment set.
|
|
||
| size_t bytes_usage = 0; | ||
| ColumnsWithTypeAndName new_columns; | ||
| std::vector<int> result_column_ids; |
There was a problem hiding this comment.
[P1] Keep projection allocations in the next-round reservation estimate
This rewrite removes every update to local_state->_estimate_memory_usage. OperatorX::get_reserve_mem_size() reads that value, and PipelineTask consumes/resets it before the next get_block_after_projects() to decide whether to pause/revoke/spill. Moving a result into output_block after expression evaluation does not remove the allocation: a large string/complex intermediate or final projection will now reserve only the minimum and can allocate through the workload-group pressure gate. Please keep an observed or sound pre-execution estimate for newly allocated projection data (without counting genuinely shared pass-through storage) and cover a repeated large variable-width projection with reservation enabled.
| input_block.shuffle_columns(result_column_ids); | ||
| } | ||
|
|
||
| DCHECK_EQ(rows, input_block.rows()); |
There was a problem hiding this comment.
[P2] Keep a release-mode rejection for an empty intermediate stage
init() still accepts an empty element in intermediate_projections_list; resize(0) followed by shuffle_columns({}) creates a zero-column block whose rows() is zero. These replacement checks disappear in release builds, so final expressions execute with count zero and a nonempty input can return Status::OK() with no rows, whereas the deleted check returned InternalError. Please restore the empty-stage/runtime row-count invariant (the per-expression size checks can rely on execute_column()) and cover the release behavior with a malformed-plan/unit test.
| insert_column_datas(mutable_columns[i], column_ptr, rows); | ||
| DCHECK_EQ(mutable_columns.size(), local_state->_projections.size()) << debug_string(); | ||
|
|
||
| for (int i = 0; i < mutable_columns.size(); ++i) { |
There was a problem hiding this comment.
[P2] Avoid cloning pass-through slots while discarding the reusable buffer
For a final slot ref, origin_block, the shallow input_block, and column_ptr all own the same column when IColumn::mutate() runs, so shallow_mutate() takes its deep-clone path. Meanwhile mutable_columns[i] is the retained-capacity output column cleared by PipelineTask, and this assignment destroys it. A slot-only projection therefore performs a fresh allocation plus full-column copy on every block. Please publish the immutable result without requiring mutable ownership, or release the source owners before a real transfer, and add a reused-block slot projection benchmark/test.
| ColumnPtr column_ptr; | ||
| RETURN_IF_ERROR(local_state->_projections[i]->execute(&input_block, column_ptr)); | ||
| column_ptr = column_ptr->convert_to_full_column_if_const(); | ||
| if (mutable_columns[i]->is_nullable() != column_ptr->is_nullable()) { |
There was a problem hiding this comment.
[P1] Preserve the supported nullable widening here
VExpr::check_expr_output_type() explicitly accepts T -> Nullable(T), and VExpr::execute_column() also allows a declared nullable expression to return a physically non-nullable column when the evaluated rows contain no nulls. Such a plan passes prepare/expression execution but now throws here; the deleted branch attached an all-zero null map. Please preserve this one-way mismatch with make_nullable(..., false) (or equivalent) before transferring ownership, while still rejecting Nullable(T) -> T, and add a focused positive-row test.
| throw Exception(ErrorCode::INTERNAL_ERROR, "Nullable mismatch"); | ||
| } | ||
| DCHECK(mutable_block.rows() == rows); | ||
| mutable_columns[i] = IColumn::mutate(std::move(column_ptr)); |
There was a problem hiding this comment.
[P1] Validate the result against the destination type before replacing its pointer
This block keeps the DataType created from _output_row_descriptor, but VSlotRef::execute_type(block) intentionally validates a returned slot column against the runtime input type, which can differ from the expression's declared/output type. With equal nullability (for example declared/output INT but runtime BIGINT), the check above passes and this installs ColumnInt64 under DataTypeInt32. The old typed insert_range_from failed loudly through release-enabled assert_cast; please validate against the destination type or materialize into the typed destination before replacing it.
|
/review |
There was a problem hiding this comment.
Automated follow-up review — REQUEST_CHANGES
The current head fixes the pass-through COW clone reported in discussion r3655092239: the generic operator and scanner paths now publish immutable ColumnPtr results directly, and the reviewed ownership/free-block lifecycle is safe. No new inline finding survived validation and deduplication, so I am not repeating existing comments.
Four previously filed blockers remain present on this head:
- [P1] Supported non-null
Tresults still throw when the destination isNullable(T)(r3655092246). - [P1] A runtime SlotRef column can still be published under an incompatible retained destination
DataType(r3655092252). - [P1] Computed/intermediate projection allocations still no longer inform the next-round reservation gate (r3655092228).
- [P2] An empty intermediate stage can still silently drop nonempty rows in release builds (r3655092233).
Critical checkpoints:
- Goal and proof: immutable publication now achieves the intended no-forced-clone handoff, but the remaining correctness, reservation, and release-invariant defects mean the PR is not ready to merge. No focused test or benchmark was added.
- Scope and parallel paths: the three-file change is focused, both generic and scanner publication paths were reviewed, and
_keep_originwas already dead. The scanner lifecycle change itself is safe; its analogous nullable/intermediate behavior predates this PR. - Concurrency and lifecycle: projection contexts and blocks are task/scanner-local, scanner results cross the existing locked task/context handoff, and COW-aware clearing preserves shared output data. No new lock-order, deadlock, static-lifetime, or cross-thread mutation issue was found.
- Error handling and compatibility: expression
Statusvalues remain checked, but the two existing type-contract failures and release-only empty-stage behavior remain. There is no new protocol, storage-format, configuration, rolling-upgrade, persistence, transaction, data-write, or FE-BE variable change. - Performance and memory: the old shared-SlotRef deep copy is resolved, while the existing reservation-accounting blocker remains for allocating projections. No separate changed-line scanner allocation defect was substantiated.
- Tests and observability: existing tests exercise ordinary projections but do not prove immutable identity, nullable widening, destination runtime-type validation, malformed empty stages, reservation-enabled variable-width repetition, or scanner free-block reuse. Per the runner contract, no local build or test was run.
- User focus: no additional focus was provided, so the whole PR was reviewed.
Two current-head review rounds, including full operator/scanner coverage and a separate risk-focused pass, converged on this frozen zero-inline set with every candidate either deduplicated or dismissed with concrete evidence.
|
/review • The remaining comments describe hypothetical or malformed-plan cases rather than regressions from this PR:
The current head resolves the actual COW clone by publishing immutable projection columns directly in both operator and |
|
run buildall |
TPC-H: Total hot run time: 28910 ms |
TPC-DS: Total hot run time: 171315 ms |
ClickBench: Total hot run time: 24.25 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
Projection results were converted to mutable columns before being published to the output block. For shared columns such as SlotRef results,
IColumn::mutate()triggered a COW clone and copied the entire column.This change publishes immutable
ColumnPtrresults directly in both operator and scanner projection paths, while preserving the output schema and scanner origin-block lifecycle.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)