github-actions[bot] commented on code in PR #67469:
URL: https://github.com/apache/doris/pull/67469#discussion_r3921464843
##########
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) {
Review Comment:
[P1] Preserve valid constant precision expressions under DISTINCT
`checkLegalityBeforeTypeCoercion` accepts any constant integral expression,
but this post-rewrite branch narrows it to `IntegerLikeLiteral`. For
`DATASKETCHES_HLL_UNION_AGG(DISTINCT sk, CAST(8 AS BIGINT))`,
`FoldConstantRuleOnFE` intentionally skips children below DISTINCT aggregates
and numeric casts remain `Cast`, so the valid in-range value is rejected here.
Please normalize/evaluate the precision child or extract its checked integral
value without requiring this concrete node type, and add a DISTINCT cast
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/DataSketchesHllUnionAgg.java:
##########
@@ -39,27 +40,43 @@
/** datasketches_hll_union_agg agg function. */
public class DataSketchesHllUnionAgg extends NotNullableAggregateFunction
- implements UnaryExpression, ExplicitlyCastableSignature,
FunctionTrait, RollUpTrait {
+ implements ExplicitlyCastableSignature, FunctionTrait, RollUpTrait {
public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(StringType.INSTANCE),
FunctionSignature.ret(DoubleType.INSTANCE).args(VarcharType.SYSTEM_DEFAULT),
-
FunctionSignature.ret(DoubleType.INSTANCE).args(VarBinaryType.INSTANCE)
+
FunctionSignature.ret(DoubleType.INSTANCE).args(VarBinaryType.INSTANCE),
Review Comment:
[P1] Exclude the precision literal from DISTINCT grouping
Adding `lg_max_k` as a child makes the inherited `getDistinctArguments()`
return `{sk, 8}`. In a query such as `SELECT ... FROM t GROUP BY ROLLUP(g)`
with `DATASKETCHES_HLL_UNION_AGG(DISTINCT sk, 8)` and `COUNT(DISTINCT sk)`,
`AggregateUtils.distinctArgumentGroupCountUpToTwo` now sees `{sk, 8}` versus
`{sk}`. `DistinctAggStrategySelector` then rejects the query as unsupported
multi-distinct with grouping sets because this function lacks
`SupportMultiDistinct`, even though both aggregates use the same row-varying
key. Override `getDistinctArguments()` to return only the sketch argument (as
other constant-parameter aggregates do), and add a ROLLUP regression.
##########
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();
Review Comment:
[P1] Keep the one-argument cap across mixed-version partial merges
One-argument input now initializes local unions with cap 12, but this
no-configuration merge is still used by `read()` and `merge(place,rhs)`. During
a rolling upgrade, an old BE can send a one-argument partial whose first dense
sketch set `lgK=21`; the new BE then initializes at 21, bypassing the
documented default cap and potentially allocating about 2 MiB per group with
precision/memory dependent on fragment placement. Please carry the configured
cap/version in transported state or clamp/gate legacy states before merging,
and add a mixed-version partial-state test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/DataSketchesHllUnionAgg.java:
##########
@@ -39,27 +40,43 @@
/** datasketches_hll_union_agg agg function. */
public class DataSketchesHllUnionAgg extends NotNullableAggregateFunction
- implements UnaryExpression, ExplicitlyCastableSignature,
FunctionTrait, RollUpTrait {
+ implements ExplicitlyCastableSignature, FunctionTrait, RollUpTrait {
public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(StringType.INSTANCE),
FunctionSignature.ret(DoubleType.INSTANCE).args(VarcharType.SYSTEM_DEFAULT),
-
FunctionSignature.ret(DoubleType.INSTANCE).args(VarBinaryType.INSTANCE)
+
FunctionSignature.ret(DoubleType.INSTANCE).args(VarBinaryType.INSTANCE),
+
FunctionSignature.ret(DoubleType.INSTANCE).args(StringType.INSTANCE,
IntegerType.INSTANCE),
Review Comment:
[P1] Exclude the precision literal from DISTINCT grouping
Adding `lg_max_k` as a child makes the inherited `getDistinctArguments()`
return `{sk, 8}`. In a query such as `SELECT ... FROM t GROUP BY ROLLUP(g)`
with `DATASKETCHES_HLL_UNION_AGG(DISTINCT sk, 8)` and `COUNT(DISTINCT sk)`,
`AggregateUtils.distinctArgumentGroupCountUpToTwo` now sees `{sk, 8}` versus
`{sk}`. `DistinctAggStrategySelector` then rejects the query as unsupported
multi-distinct with grouping sets because this function lacks
`SupportMultiDistinct`, even though both aggregates use the same row-varying
key. Override `getDistinctArguments()` to return only the sketch argument (as
other constant-parameter aggregates do), and add a ROLLUP regression.
##########
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] Keep the one-argument cap across mixed-version partial merges
One-argument input now initializes local unions with cap 12, but this
no-configuration merge is still used by `read()` and `merge(place,rhs)`. During
a rolling upgrade, an old BE can send a one-argument partial whose first dense
sketch set `lgK=21`; the new BE then initializes at 21, bypassing the
documented default cap and potentially allocating about 2 MiB per group with
precision/memory dependent on fragment placement. Please carry the configured
cap/version in transported state or clamp/gate legacy states before merging,
and add a mixed-version partial-state test.
##########
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)) {
Review Comment:
[P1] Preserve valid constant precision expressions under DISTINCT
`checkLegalityBeforeTypeCoercion` accepts any constant integral expression,
but this post-rewrite branch narrows it to `IntegerLikeLiteral`. For
`DATASKETCHES_HLL_UNION_AGG(DISTINCT sk, CAST(8 AS BIGINT))`,
`FoldConstantRuleOnFE` intentionally skips children below DISTINCT aggregates
and numeric casts remain `Cast`, so the valid in-range value is rejected here.
Please normalize/evaluate the precision child or extract its checked integral
value without requiring this concrete node type, and add a DISTINCT cast
regression.
--
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]
