github-actions[bot] commented on code in PR #66601:
URL: https://github.com/apache/doris/pull/66601#discussion_r3748124810
##########
be/src/exprs/aggregate/aggregate_function_array_agg.h:
##########
@@ -234,11 +235,13 @@ struct AggregateFunctionArrayAggData<T> {
using ElementType = StringRef;
using Self = AggregateFunctionArrayAggData<T>;
MutableColumnPtr column_data;
+ DataTypePtr column_type;
Review Comment:
**[P2] Keep invariant serializer metadata out of each group state**
`column_type` and `be_exec_version` are identical for every state and
already live on `AggregateFunctionArrayAgg` as `argument_types[0]` and
`version`. Storing a `shared_ptr` plus the integer here grows the generic
complex state from 8 to 32 bytes, so one million groups add about 24 MiB of
headers and shared-pointer refcount churn; foreach growth retains the same
overhead for every allocated position. Please pass the function-owned metadata
into `write()`/`read()` (or otherwise reference it without per-state ownership)
and keep only `column_data` in each state.
##########
be/src/exprs/aggregate/aggregate_function_array_agg.h:
##########
@@ -265,15 +268,28 @@ struct AggregateFunctionArrayAggData<T> {
}
void write(BufferWritable& buf) const {
- throw Exception(ErrorCode::NOT_IMPLEMENTED_ERROR, "array_agg not
support write");
+ const auto serialized_bytes =
+ column_type->get_uncompressed_serialized_bytes(*column_data,
be_exec_version);
+ std::string serialized_buffer(serialized_bytes, '\0');
Review Comment:
**[P2] Avoid an untracked full-state scratch allocation**
`column_data` already owns the unbounded aggregate, but this allocates the
entire native upper bound through ordinary `std::string`, outside Doris's
MemTracker, and `buf.write_binary()` then copies the logical bytes into the
tracked destination `ColumnString` while that scratch remains live. A large
foreach partial state can therefore add nearly another full payload to BE RSS
without query accounting, potentially ending in `std::bad_alloc` or process
OOM. Since this frame is new, please serialize once into padded destination
storage with logical-length backfill/shrink and rollback on failure, then add a
large-state memory test.
--
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]
