[fix](be) Fix DataSketches HLL union accuracy and add configurable lg… - #67469
[fix](be) Fix DataSketches HLL union accuracy and add configurable lg…#67469nooneuse wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
REQUEST_CHANGES for three independently verified correctness issues in the FE DISTINCT/planner path and BE aggregate-state compatibility path. The reviewed head is b62a64ce57e396e52abba0552121addc4c1c720c.
Checkpoints
- Goal and tests: the PR addresses DataSketches HLL merge accuracy and adds configurable
lg_max_k; the added tests cover empty, sparse, dense, mixed-lgK, aliases, invalid values, nullable input, and serialization, but do not cover the three cases called out inline. - Scope: the change is focused on one aggregate, its pinned DataSketches dependency, FE signatures, and tests.
- Concurrency and lifecycle: aggregate state is execution-local; reset and allocator ownership follow existing patterns, and no new shared lock or thread lifecycle was found.
- Compatibility: the compact HLL bytes remain compatible with the inspected upstream commit, but old/new runtime partial-state semantics do not preserve the documented default cap (HLL-R4-1).
- Parallel paths and nullability: ordinary, grouped, window, nullable-wrapper, storage-reader, and aggregate transport paths were traced; constants are materialized before row-wise BE access and no separate physical-column defect was found.
- FE/BE contract: the new two-argument signature and BE arity/type checks align for ordinary plans, but FE DISTINCT legality and DISTINCT-key grouping need fixes.
- Error handling and performance: malformed sketches and
[7,21]bounds fail explicitly; the unresolved legacy cap can cause roughly 2 MiB states atlgK=21instead of the documented default. - User focus:
review_focus.txtsays no additional focus; the full PR was reviewed.
No builds or tests were run by this reviewer, per the review-runner constraint; this is a static, source-and-call-graph review. Existing inline review threads/comments were empty at submission time.
There was a problem hiding this comment.
Summary
REQUEST_CHANGES for three independently verified correctness issues in the FE DISTINCT/planner path and BE aggregate-state compatibility path. The reviewed head is b62a64ce57e396e52abba0552121addc4c1c720c.
Checkpoints
- Goal and tests: the PR addresses DataSketches HLL merge accuracy and adds configurable
lg_max_k; the added tests cover empty, sparse, dense, mixed-lgK, aliases, invalid values, nullable input, and serialization, but do not cover the three cases called out inline. - Scope: the change is focused on one aggregate, its pinned DataSketches dependency, FE signatures, and tests.
- Concurrency and lifecycle: aggregate state is execution-local; reset and allocator ownership follow existing patterns, and no new shared lock or thread lifecycle was found.
- Compatibility: the compact HLL bytes remain compatible with the inspected upstream commit, but old/new runtime partial-state semantics do not preserve the documented default cap (HLL-R4-1).
- Parallel paths and nullability: ordinary, grouped, window, nullable-wrapper, storage-reader, and aggregate transport paths were traced; constants are materialized before row-wise BE access and no separate physical-column defect was found.
- FE/BE contract: the new two-argument signature and BE arity/type checks align for ordinary plans, but FE DISTINCT legality and DISTINCT-key grouping need fixes.
- Error handling and performance: malformed sketches and
[7,21]bounds fail explicitly; the unresolved legacy cap can cause roughly 2 MiB states atlgK=21instead of the documented default. - User focus:
review_focus.txtsays no additional focus; the full PR was reviewed.
No builds or tests were run by this reviewer, per the review-runner constraint; this is a static, source-and-call-graph review. Existing inline review threads/comments were empty at submission time.
There was a problem hiding this comment.
Summary
REQUEST_CHANGES for three independently verified correctness issues in the FE DISTINCT/planner path and BE aggregate-state compatibility path. The reviewed head is b62a64ce57e396e52abba0552121addc4c1c720c.
Checkpoints
- Goal and tests: the PR addresses DataSketches HLL merge accuracy and adds configurable
lg_max_k; the added tests cover empty, sparse, dense, mixed-lgK, aliases, invalid values, nullable input, and serialization, but do not cover the three cases called out inline. - Scope: the change is focused on one aggregate, its pinned DataSketches dependency, FE signatures, and tests.
- Concurrency and lifecycle: aggregate state is execution-local; reset and allocator ownership follow existing patterns, and no new shared lock or thread lifecycle was found.
- Compatibility: the compact HLL bytes remain compatible with the inspected upstream commit, but old/new runtime partial-state semantics do not preserve the documented default cap (HLL-R4-1).
- Parallel paths and nullability: ordinary, grouped, window, nullable-wrapper, storage-reader, and aggregate transport paths were traced; constants are materialized before row-wise BE access and no separate physical-column defect was found.
- FE/BE contract: the new two-argument signature and BE arity/type checks align for ordinary plans, but FE DISTINCT legality and DISTINCT-key grouping need fixes.
- Error handling and performance: malformed sketches and
[7,21]bounds fail explicitly; the unresolved legacy cap can cause roughly 2 MiB states atlgK=21instead of the documented default. - User focus:
review_focus.txtsays no additional focus; the full PR was reviewed.
No builds or tests were run by this reviewer, per the review-runner constraint; this is a static, source-and-call-graph review. Existing inline review threads/comments were empty at submission time.
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for three distinct aggregate-state gaps on exact head e629d36bc73369298de19527a1dd3f78bca2814e.
Findings:
- A valid two-argument state cannot be consumed by
_mergeor_unionafter it becomes a typed slot, including normal reads of a storedGENERICAGG_STATEcolumn. - Two-argument state values made with different caps have the same state type, and sparse state merging chooses the eventual precision/memory bound by merge order; tablet aggregation/compaction reaches this path independently of the FE binding failure.
- The generated
_stateform skips the nested aggregate's post-rewrite range check, so invalid bounds are not consistently rejected by FE and can succeed on empty/all-null inputs.
Critical checkpoint conclusions:
- Goal and algorithm: the exact upstream lazy-KxQ/
curMinrepair covers the direct emptiness, incremental-update, result-copy, estimate, and serialization paths. Homogeneous direct/exchange/spill aggregation with one common explicit cap is consistent. The blocking defects are in the automatically exposed aggregate-state paths around the new parameter. - User focus:
review_focus.txtcontains no additional user-provided focus; the full PR was reviewed. - Concurrency and parallel paths: no separate race was found. Exchange/spill paths preserve a common cap, but state-slot reconstruction and stored-state tablet aggregation expose the findings above.
- Lifecycle and resources: empty-state skipping, optional-union reset/destruction, and allocator ownership exposed no separate defect. The documented dense
lgK=21memory cost is intentional; merge-order-dependent selection of that cost is covered by the second finding. - Configuration and compatibility: aliases, constant coercion, ignored
DISTINCT, grouping/ROLLUP, the one-argument max-21 policy, and the post-upgrade-only two-argument rollout were checked. The existing one-argument rolling-upgrade concern was not duplicated. - Conditions and validation: ordinary aggregate bounds/type checks are present, but the generated
_statewrapper omits the post-rewrite bound check. - Persistence and protocol:
AggStateTypecarries argument types rather than constant values, and the serialized state carries the effective sketch rather than a separate configured-cap identity; this makes valid state binding fail and mixed-cap state aggregation order-dependent. - Tests and results: the added BE/FE/regression tests cover direct calls, aliases, ordinary invalid arguments, sparse/dense merge behavior, and the upstream regression, but not
_state/_union/_merge, typed-slot round trips, or mixed-cap stored states. Per the review-runner instructions, no builds or tests were executed here; at submission preparation time BE UT, FE UT, compile, performance, and code-review checks were still pending. - Observability and other risks: exception translation and corrupt-input reporting remain visible; no distinct logging, data-write, ABI/build-topology, or unrelated performance issue survived the final sweep.
| + " function's argument should be of STRING/VARCHAR/VARBINARY type, but was " + inputType); | ||
| } | ||
| if (arity() == 2 | ||
| && (!getArgument(1).isConstant() || !getArgumentType(1).isIntegralType())) { |
There was a problem hiding this comment.
[P1] Preserve the precision parameter when binding state consumers
A valid two-argument state becomes unusable as soon as it is materialized. For example, in SELECT datasketches_hll_union_agg_merge(s) FROM (SELECT datasketches_hll_union_agg_state(sk, 8) AS s FROM t) q, the outer s is an AggStateType slot. AggCombinerFunctionBuilder.buildMergeOrUnion() then reconstructs both nested arguments with DataTypeUtils.getMockedExpressions(), so the INT parameter is a nonconstant SlotReference; MergeCombinator/UnionCombinator delegate legality to that reconstructed function and this branch rejects it. A stored GENERIC AGG_STATE scan also automatically builds the same _union consumer in BindRelation. Only the direct AST shortcut _merge(_state(sk, 8)) retains the literal. Please preserve the required constant identity when reconstructing state consumers, or explicitly exclude this overload from combinators, and add subquery/stored-column round trips for the canonical name and aliases.
| } | ||
| hll_union_data.reset(); | ||
| void merge(const Sketch& sketch_data) { | ||
| merge(sketch_data, std::max<uint8_t>(sketch_data.get_lg_config_k(), MIN_UNION_LOG_K)); |
There was a problem hiding this comment.
[P1] Make two-argument aggregate-state caps deterministic
datasketches_hll_union_agg_state(sk, 7) and _state(sk, 21) have the same aggregate-state type because it records argument types, not the constant value. Stored-state tablet aggregation/compaction reconstructs the two-argument nested BE function from those types and reaches this no-configuration merge directly. With two sparse states, this overload initializes from whichever serialized sketch arrives first; upstream LIST/SET handling replays later coupons into that existing gadget without lowering its lgK. Consequently cap-21 then cap-7 remains 21, while the reverse remains 7, producing different serialized state and later different dense memory/precision. This is distinct from the existing one-argument rolling-upgrade thread: it is an all-new two-argument state contract. Please define a deterministic rule such as rejecting mismatched caps or merging at the minimum effective bound, and cover cap-7/cap-21 state aggregation in both orders.
| throw new AnalysisException(getName() + " requires lg_max_k to be a constant integer: " + this.toSql()); | ||
| } | ||
| long value = ((IntegerLikeLiteral) lgMaxK).getLongValue(); | ||
| if (value < MIN_LG_MAX_K || value > MAX_LG_MAX_K) { |
There was a problem hiding this comment.
[P2] Run the range check for the generated _state form
StateCombinator delegates only checkLegalityBeforeTypeCoercion() to its nested aggregate. The nested function is a field rather than an expression child, and the wrapper has no checkLegalityAfterRewrite() override, so this bound check is never reached for datasketches_hll_union_agg_state(sk, 6) or (..., 22) (including aliases). BE rejects the value only when add() sees a non-null row; an empty or all-null input can therefore succeed and produce an empty state despite the promised FE validation. Please forward a combinator-aware post-rewrite check and add _state negative tests for both bounds and aliases.
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 16758 ms |
TPC-DS: Total hot run time: 81480 ms |
ClickBench: Total hot run time: 14.71 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
3 participants
What problem does this PR solve?
Problem Summary:
DATASKETCHES_HLL_UNION_AGGmerges serialized Apache DataSketches HLL sketches. Doris previously pinneddatasketches-cpp5.2.0, which contains a regression introduced by the lazy KxQ/curMinrebuild optimization.When an HLL-mode sketch was downsampled during a union, the register array was updated while the cached estimator state remained pending rebuild. Some subsequent operations did not honor that pending state. Depending on the merge order, the union could therefore:
Doris also initialized the union limit from the first serialized sketch, including empty or sparse sketches whose configured
lgKshould not necessarily constrain the effective precision of later inputs. In a parallel aggregation, the first sketch is not deterministic, so the effective precision and memory usage could depend on input and partial-state merge order.This PR updates the
datasketches-cppsubmodule from the 5.2.0 commit (de8553ba) to upstream commit46025e9. The upstream fix rebuilds the deferred KxQ/curMinstate before operations that require it and makes union estimates and serialization independent of the affected merge order.On the Doris side, empty sketches no longer initialize the union, and deserialized intermediate states restore the union from the serialized sketch's own effective
lgK. This prevents empty inputs and the aggregate transport path from imposing an additional precision reduction.Release note
Fixed incorrect and merge-order-dependent results in
DATASKETCHES_HLL_UNION_AGGfor affected mixed-lgKHLL sketches. Added an optional constantlg_max_kargument so users can explicitly control the precision and memory upper bound of the union.Accuracy Control
The function now supports both forms:
The one-argument form has no
lg_max_kSQL argument. Internally, it initializes the union with the maximum supported limit,DEFAULT_UNION_LOG_K = 21, so it does not downsample an input solely because of an additional Doris-side default cap.The two-argument form accepts a constant integer in the inclusive range
[7, 21]:The same optional argument is supported by the aliases:
The parameter is validated by both FE and BE. Non-constant, non-integral, null, or out-of-range values are rejected.
Important Behavior and Upgrade Notes
lg_max_kis a strict upper bound, not a requested final precision. A dense input sketch with a smallerlgKcan reduce the union's effectivelgK, because a lower-precision dense sketch cannot be upsampled to recover information that is no longer present.lgKcan still decrease after consuming a lower-precision dense sketch because that lost information cannot be recovered.2^lg_max_kbytes per aggregate state, excluding object and allocator overhead. A dense state atlgK=12is approximately 4 KiB, while a dense state atlgK=21is approximately 2 MiB. The one-argument form does not always allocate 2 MiB: an existing dense input with a lowerlgKreduces the effective state size. Grouped aggregation can hold many states concurrently, so use the two-argument form when a lower memory upper bound is required.46025e9includes 129 upstream commits because no newer DataSketches C++ release contains the required fix. The dependency remains pinned to the exact reviewed commit rather than following the upstream branch.DISTINCTis ignored by the two-argument form; addinglg_max_kdoes not opt the function into multi-column DISTINCT aggregation.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)