github-actions[bot] commented on code in PR #67469:
URL: https://github.com/apache/doris/pull/67469#discussion_r3999491701
##########
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:
[P2] Limit the manual lowering rebuild to sparse inputs
This condition also fires for a lower-lgK HLL-mode state. For a populated
lgK-21 union followed by a dense lgK-7 state, `get_result(HLL_8)` first
allocates/copies the full high-precision array (roughly 2 MiB, plus a possible
lazy estimator rebuild), and `update(current)` then scans it again to
downsample. Upstream `HllUnion::union_impl` already lowers a dense destination
when the incoming HLL has smaller lgK, so this extra copy is only needed for
LIST/SET inputs, whose coupons otherwise replay without lowering. Please gate
the manual rebuild on sparse input and add a dense high-cap/low-cap merge case;
the current seven-coupon mixed-cap test covers only the sparse branch.
##########
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);
Review Comment:
[P2] Keep invalid serialized lgK inside the corruption boundary
At upstream `46025e9`, CouponList deserialization copies `LG_K_BYTE` without
range-checking it (and CouponHashSet checks only the lower bound). A well-sized
non-empty sparse state with lgK 22 therefore gets through
`Sketch::deserialize`; on the fresh state used by `_union`/partial
deserialization, this value reaches `merge(sketch, 22)` and the union
constructor throws `std::invalid_argument` after `read()`'s CORRUPTION catch
has ended. The previous upper clamp avoided that escape. Please validate the
deserialized lgK against `MAX_LOG_K` before using it here, or extend the
conversion boundary, and add a malformed sparse-header test that verifies
CORRUPTION.
--
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]
