github-actions[bot] commented on code in PR #67469:
URL: https://github.com/apache/doris/pull/67469#discussion_r3921458378
##########
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 {
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:
##########
@@ -24,10 +24,11 @@
import org.apache.doris.nereids.trees.expressions.functions.Function;
import org.apache.doris.nereids.trees.expressions.functions.FunctionTrait;
import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral;
-import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
+import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DataType;
import org.apache.doris.nereids.types.DoubleType;
+import org.apache.doris.nereids.types.IntegerType;
import org.apache.doris.nereids.types.StringType;
import org.apache.doris.nereids.types.VarBinaryType;
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:
##########
@@ -53,11 +53,9 @@ class ColumnDecimal;
/// datasketches_hll_union_agg
template <PrimitiveType T>
struct AggregateFunctionHllSketchData {
- /** We set the default LgK to 12,
- * as this value is used as a performance baseline in the relevant
documentation.
- * (https://datasketches.apache.org/docs/HLL/HllPerformance.html)
- */
- static constexpr uint8_t DEFAULT_LOG_K = 12;
+ static constexpr uint8_t EMPTY_STATE_LOG_K = 12;
+ static constexpr uint8_t MIN_UNION_LOG_K = 7;
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.
--
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]
