Copilot commented on code in PR #61946:
URL: https://github.com/apache/doris/pull/61946#discussion_r3020125156
##########
be/src/exec/operator/distinct_streaming_aggregation_operator.cpp:
##########
@@ -198,13 +194,12 @@ Status
DistinctStreamingAggLocalState::_distinct_pre_agg_with_serialized_key(
}
DCHECK_EQ(out_block->columns(), key_size);
if (_stop_emplace_flag && _distinct_row.empty()) {
- // If _stop_emplace_flag is true and _distinct_row is also empty,
it means it is in streaming mode, outputting what is input
- // swap the column directly, to solve Check failed:
d.column->use_count() == 1 (2 vs. 1)
+ // Streaming mode: move key columns directly into output block
for (int i = 0; i < key_size; ++i) {
- auto output_column = out_block->get_by_position(i).column;
- out_block->replace_by_position(i,
key_columns[i]->assume_mutable());
- in_block->replace_by_position(result_idxs[i], output_column);
+ out_block->replace_by_position(i,
std::move(key_column_ptrs[i]));
}
+ // Release VSlotRef shared column references from in_block
+ in_block->clear();
Review 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.
```suggestion
// Release VSlotRef shared column references from in_block but
preserve its schema
in_block->clear_column_data();
```
##########
be/src/exprs/vectorized_agg_fn.cpp:
##########
@@ -374,7 +385,8 @@ AggFnEvaluator::AggFnEvaluator(AggFnEvaluator& evaluator,
RuntimeState* state)
_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) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
