[refine](expr) Migrate old VExpr execute interface to new execute_column (Part 2)#61946
Closed
Mryange wants to merge 1 commit into
Closed
[refine](expr) Migrate old VExpr execute interface to new execute_column (Part 2)#61946Mryange wants to merge 1 commit into
Mryange wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 26668 ms |
TPC-DS: Total hot run time: 170044 ms |
Contributor
Author
|
run nonConcurrent |
Contributor
Author
|
run p0 |
Contributor
Author
|
run beut |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 finding:
be/src/exec/operator/distinct_streaming_aggregation_operator.cpp: the newin_block->clear()calls in the streaming pass-through path drop the reusable child block schema.StatefulOperatorX::get_block()reuseslocal_state._child_blockacross iterations and only callsclear_column_data(num_materialized_slots())before the next childget_block_after_projects(). The previous swap-back logic preserved the block layout; this refactor can hand a structurally empty block to the child on the next iteration.
Critical checkpoint conclusions:
- Goal of the task: migrate callers to the new
VExprContext::execute(..., ColumnPtr&)/ColumnWithTypeAndNameAPIs and reduce block mutation. Conclusion: mostly achieved, but the distinct streaming agg change is not safe yet. No test was added for this path. - Small / clear / focused: yes, the PR is narrowly focused on API migration.
- Concurrency: I did not find new thread, lock, or atomic-ordering issues in the touched code.
- Lifecycle / static initialization: no static-init issue found. Lifecycle management is the main risk area in this PR; hash join and set probe now keep holders for raw pointers correctly, but distinct streaming agg breaks the reusable
Blocklifecycle described above. - Configuration changes: none.
- Compatibility / rolling upgrade / storage format: none.
- Parallel code paths: the migrated hash join / scanner / agg paths are generally consistent with the new API; the distinct streaming agg streaming branch is the outlier.
- Special conditional checks: the
_stop_emplace_flag && _distinct_row.empty()branch now relies onclear(), but that branch still needs to preserve the input block structure for the next reuse. - Test coverage: no regression or unit test was added for this lifetime-sensitive refactor path.
- Observability: no new observability requirement identified.
- Transaction / persistence: not applicable.
- Data writes / modifications: not applicable.
- FE-BE variable passing: not applicable.
- Performance: the API migration should reduce unnecessary block mutation overall; I did not find a separate performance bug beyond the reusable-block regression above.
- Other issues: none confirmed beyond the finding above.
| out_block->replace_by_position(i, std::move(key_column_ptrs[i])); | ||
| } | ||
| // Release VSlotRef shared column references from in_block | ||
| in_block->clear(); |
Contributor
There was a problem hiding this comment.
push() here is operating on the reusable _child_block owned by StatefulOperatorX. Replacing the old swap-back logic with in_block->clear() changes more than reference counts: it drops the block schema entirely. On the next iteration, StatefulOperatorX::get_block() only does clear_column_data(num_materialized_slots()) before reusing the same block, so this path can hand a structurally empty block back to the child.
The previous code kept the block layout intact by swapping the original output columns back into in_block; this branch still needs to preserve that reusable-block contract (same issue for the other new in_block->clear() below).`
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the BE-side refactor to migrate expression evaluation from the legacy VExprContext::execute(Block*, int*) API (mutating Block + returning a column index) to the newer VExprContext::execute(const Block*, ColumnPtr&) / execute(const Block*, ColumnWithTypeAndName&) APIs that return columns directly and avoid Block mutation/lifetime coupling.
Changes:
- Refactor aggregation evaluators and multiple operators to hold
ColumnPtrresults (and raw-pointer views) instead of trackingresult_column_idindices inBlock. - Update hash-join probe and set-probe code paths to operate on
std::vector<ColumnPtr>and add holder vectors to keep raw pointers valid. - Update scanner intermediate projections to use the
ColumnWithTypeAndNameexecute overload and rebuild the intermediateBlockdirectly from returned columns.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| be/src/exprs/vectorized_agg_fn.h | Switch agg argument evaluation to const-Block execute API; add input-column holders + reset hook. |
| be/src/exprs/vectorized_agg_fn.cpp | Use ColumnPtr holders and pass raw pointers to agg functions; clear holders after each call. |
| be/src/exec/scan/scanner.cpp | Replace execute + shuffle_columns with execute(ColumnWithTypeAndName) and rebuild the intermediate block. |
| be/src/exec/operator/set_probe_sink_operator.h | Replace probe “inserted id” tracking with ColumnPtr holder storage. |
| be/src/exec/operator/set_probe_sink_operator.cpp | Extract probe columns via new execute API; keep ColumnPtr holders to back raw pointers. |
| be/src/exec/operator/hashjoin_probe_operator.h | Update helper signatures to accept std::vector<ColumnPtr> instead of column ids. |
| be/src/exec/operator/hashjoin_probe_operator.cpp | Evaluate probe expressions into ColumnPtr results and hold them to keep key pointers valid. |
| be/src/exec/operator/distinct_streaming_aggregation_operator.cpp | Replace result_idxs with Columns holders; streamline streaming path using move + input block clearing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
375
to
+389
| AggFnEvaluator::AggFnEvaluator(AggFnEvaluator& evaluator, RuntimeState* state) | ||
| : _fn(evaluator._fn), | ||
| _is_merge(evaluator._is_merge), | ||
| _without_key(evaluator._without_key), | ||
| _is_window_function(evaluator._is_window_function), | ||
| _argument_types_with_sort(evaluator._argument_types_with_sort), | ||
| _real_argument_types(evaluator._real_argument_types), | ||
| _intermediate_slot_desc(evaluator._intermediate_slot_desc), | ||
| _output_slot_desc(evaluator._output_slot_desc), | ||
| _sort_description(evaluator._sort_description), | ||
| _data_type(evaluator._data_type), | ||
| _function(evaluator._function), | ||
| _expr_name(evaluator._expr_name), | ||
| _agg_columns(evaluator._agg_columns) { | ||
| _agg_input_columns(evaluator._agg_input_columns), | ||
| _agg_raw_input_columns(evaluator._agg_raw_input_columns) { |
There was a problem hiding this comment.
AggFnEvaluator clone copy-ctor is copying _agg_input_columns (ColumnPtr holders) and _agg_raw_input_columns from the source evaluator. These vectors are per-execution temporaries; copying them can unintentionally keep previous Block columns alive (extra ref-counts) and undermines the goal of decoupling column lifetimes. Consider initializing these vectors empty in the clone (or clearing them in the copy-ctor) so each evaluator only holds columns produced by its own _calc_argument_columns() call.
Comment on lines
+201
to
+202
| // Release VSlotRef shared column references from in_block | ||
| in_block->clear(); |
There was a problem hiding this comment.
In the streaming/mem_reuse fast path, in_block->clear() drops the entire Block structure (schema), which disables upstream block memory reuse and can force repeated column re-allocation/re-insertion on subsequent pulls. If the intent is only to release shared column references (without mutating the moved columns now owned by out_block), consider replacing the referenced columns in in_block with fresh empty columns (e.g., clone_empty() equivalents) or swapping with an empty block that preserves structure, instead of clearing the container.
Suggested change
| // Release VSlotRef shared column references from in_block | |
| in_block->clear(); | |
| // Release VSlotRef shared column references from in_block but preserve its schema | |
| in_block->clear_column_data(); |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29130 ms |
TPC-DS: Total hot run time: 174237 ms |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29073 ms |
TPC-DS: Total hot run time: 174977 ms |
Contributor
Author
|
run cloud_p0 |
Mryange
force-pushed
the
expr-api-ref-part-2
branch
from
294434a to
61cbca2
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29330 ms |
TPC-DS: Total hot run time: 180853 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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?
Continue the migration from old
VExprContext::execute(Block*, int*)API to the newVExprContext::execute(const Block*, ColumnPtr&)API, following up on #61912 (Part 1).The old API inserts the result column into the Block and returns a column index, which mutates the Block and causes unnecessary column lifetime coupling. The new API returns the result column directly without modifying the Block.
Changes
_calc_argument_columns()populates holders, and_reset_input_columns()clears them after each aggregation call to avoid inflating VSlotRef shared column reference counts._do_evaluate/_extract_join_column/_need_probe_null_mapto work withvector<ColumnPtr>instead ofvector<int>. All column references stored in_key_columns_holderto keep raw pointers alive.result_idxswithColumns key_column_ptrs. Simplify the streaming swap path tostd::move+in_block->clear()._probe_column_holdersto hold ColumnPtrs. Remove dead_probe_column_inserted_idfield.execute + shuffle_columnswithexecute(ColumnWithTypeAndName)overload + direct Block cRelease note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)