github-actions[bot] commented on code in PR #67469:
URL: https://github.com/apache/doris/pull/67469#discussion_r3923335608
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/DataSketchesHllUnionAgg.java:
##########
@@ -73,6 +90,27 @@ public void checkLegalityBeforeTypeCoercion() {
throw new AnalysisException(getName()
+ " function's argument should be of STRING/VARCHAR/VARBINARY
type, but was " + inputType);
}
+ if (arity() == 2
+ && (!getArgument(1).isConstant() ||
!getArgumentType(1).isIntegralType())) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/DataSketchesHllUnionAgg.java:
##########
@@ -73,6 +90,27 @@ public void checkLegalityBeforeTypeCoercion() {
throw new AnalysisException(getName()
+ " function's argument should be of STRING/VARCHAR/VARBINARY
type, but was " + inputType);
}
+ if (arity() == 2
+ && (!getArgument(1).isConstant() ||
!getArgumentType(1).isIntegralType())) {
+ throw new AnalysisException(getName()
+ + " requires lg_max_k to be a constant integer: " +
this.toSql());
+ }
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ if (arity() == 1) {
+ return;
+ }
+ Expression lgMaxK = getArgument(1);
+ if (!(lgMaxK instanceof IntegerLikeLiteral)) {
+ 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) {
Review 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.
##########
be/src/exprs/aggregate/aggregate_function_datasketches_hll_union_agg.h:
##########
@@ -91,12 +84,10 @@ struct AggregateFunctionHllSketchData {
"Internal error happened when update HLL sketch:
unknown exception.");
}
}
- void reset() {
- if (hll_union_data.has_value()) {
- hll_union_data->reset();
- }
- 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));
Review 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.
--
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]
