nooneuse commented on code in PR #67469:
URL: https://github.com/apache/doris/pull/67469#discussion_r4002313218
##########
be/src/exprs/aggregate/aggregate_function_datasketches_hll_union_agg.h:
##########
@@ -91,12 +84,32 @@ struct AggregateFunctionHllSketchData {
"Internal error happened when update HLL sketch:
unknown exception.");
}
}
- void reset() {
- if (hll_union_data.has_value()) {
- hll_union_data->reset();
+ void merge(const Sketch& sketch_data) {
+ if (sketch_data.is_empty()) {
+ return;
}
- hll_union_data.reset();
+ const auto lg_max_k = std::max<uint8_t>(sketch_data.get_lg_config_k(),
MIN_UNION_LOG_K);
+ if (hll_union_data.has_value() && lg_max_k <
hll_union_data->get_lg_config_k()) {
Review Comment:
The redundant destination copy is real. For dense input, DataSketches
already handles downsampling, so the manual rebuild adds unnecessary work
without changing the result.
However, the current DataSketches API does not expose the current
representation mode. I would prefer not to make Doris depend on
serialized-header inspection or serialization-size inference to distinguish
sparse and dense sketches.
I will propose a public mode-query API upstream, such as get_current_mode()
or a narrower is_sparse() accessor. After integrating that API, I will restrict
the manual rebuild in Doris to sparse inputs and verify the allocation and
performance improvement. This keeps representation knowledge in DataSketches
and avoids a Doris-specific workaround.
--
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]
