github-actions[bot] commented on code in PR #65847:
URL: https://github.com/apache/doris/pull/65847#discussion_r3627382299
##########
be/src/util/tdigest.h:
##########
@@ -357,6 +357,61 @@ class TDigest {
return quantileProcessed(q);
}
+ void quantiles(const double* quantile_levels, const size_t* permutation,
size_t size,
+ double* result) {
+ if (size == 0) {
+ return;
+ }
+ if (haveUnprocessed() || isDirty()) {
+ process();
+ }
+
+ if (_processed.empty()) {
+ std::fill(result, result + size, NAN);
+ return;
+ }
+
+ if (_processed.size() == 1) {
+ std::fill(result, result + size, static_cast<double>(mean(0)));
+ return;
+ }
+
+ const auto n = _processed.size();
+ size_t cumulative_index = 0;
+ for (size_t result_index = 0; result_index < size; ++result_index) {
+ const size_t level_index = permutation[result_index];
+ const auto q = static_cast<Value>(quantile_levels[level_index]);
+ DCHECK_GE(q, 0);
+ DCHECK_LE(q, 1);
+
+ const auto index = q * _processed_weight;
+ if (index <= weight(0) / 2.0) {
+ DCHECK_GT(weight(0), 0);
+ result[level_index] =
+ static_cast<double>(_min + 2.0 * index / weight(0) *
(mean(0) - _min));
Review Comment:
**[P2] Preserve the scalar result precision in the left tail.**
`quantileProcessed()` returns `Value` (`float`) and explicitly narrows this
same interpolation, but `2.0` promotes the new expression to `double` and it is
stored without that rounding. For example, with `_min=0`, first-centroid
mean/weight `1/3`, total weight `6`, and `q=float(0.1)`, the batched path
yields `0.40000001589457196` while the scalar path yields
`0.40000000596046448`. That makes `percentile_approx_array` disagree with
equivalent scalar calls for low quantiles on multiweight first centroids.
Please cast the interpolation to `Value` before converting to `double` (or
share the scalar helper), and add a batch-vs-single test for this shape.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxArray.java:
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees.expressions.functions.agg;
+
+import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import
org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
+import org.apache.doris.nereids.trees.expressions.literal.ArrayLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
+import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
+import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
+import org.apache.doris.nereids.types.ArrayType;
+import org.apache.doris.nereids.types.DoubleType;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * AggregateFunction 'percentile_approx_array'.
+ */
+public class PercentileApproxArray extends NotNullableAggregateFunction
+ implements ExplicitlyCastableSignature {
+
+ public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
+ FunctionSignature.ret(ArrayType.of(DoubleType.INSTANCE))
+ .args(DoubleType.INSTANCE,
ArrayType.of(DoubleType.INSTANCE)),
+ FunctionSignature.ret(ArrayType.of(DoubleType.INSTANCE))
+ .args(DoubleType.INSTANCE,
ArrayType.of(DoubleType.INSTANCE), DoubleType.INSTANCE)
+ );
+
+ public PercentileApproxArray(Expression arg0, Expression arg1) {
+ this(false, arg0, arg1);
+ }
+
+ public PercentileApproxArray(boolean distinct, Expression arg0, Expression
arg1) {
+ super("percentile_approx_array", distinct, arg0, arg1);
+ }
+
+ public PercentileApproxArray(Expression arg0, Expression arg1, Expression
arg2) {
+ this(false, arg0, arg1, arg2);
+ }
+
+ public PercentileApproxArray(boolean distinct, Expression arg0, Expression
arg1,
+ Expression arg2) {
+ super("percentile_approx_array", distinct, arg0, arg1, arg2);
+ }
+
+ /** constructor for withChildren and reuse signature */
+ private PercentileApproxArray(AggregateFunctionParams functionParams) {
+ super(functionParams);
+ }
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (!getArgument(1).isConstant()) {
Review Comment:
**[P2] Preserve `_merge`/`_union` after an aggregate-state boundary.** A
direct `percentile_approx_array_merge(percentile_approx_array_state(...))`
keeps the literal children, but once that state is projected through a subquery
or read from a stored `AGG_STATE` column, `buildMergeOrUnion` reconstructs the
nested arguments as type-only `SlotReference`s.
`MergeCombinator`/`UnionCombinator` then delegate legality back here, so the
mocked quantile array (and optional compression) is necessarily rejected as
nonconstant even though the serialized state already carries those values.
Please avoid rerunning original value-expression legality for merge/union
states (or preserve the constants), and add a test that merges this function's
state after a subquery or stored-column boundary.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinAggregateFunctions.java:
##########
@@ -190,6 +191,7 @@ private BuiltinAggregateFunctions() {
agg(Percentile.class, "percentile", "percentile_cont"),
agg(PercentileReservoir.class, "percentile_reservoir"),
agg(PercentileApprox.class, "percentile_approx"),
+ agg(PercentileApproxArray.class, "percentile_approx_array"),
Review Comment:
**[P2] Keep the new percentile function out of `_foreach`.** Registering
this base name also lets `FunctionRegistry` synthesize
`percentile_approx_array_foreach`, but
`AggCombinerFunctionBuilder.buildForEach` replaces every nested argument with a
`SlotReference`. The nested function therefore can never satisfy its
requirement that the quantile array be constant, so every `_foreach` call is
exposed as supported and then rejected during legality checking. The parallel
`percentile`, `percentile_array`, `percentile_approx`, and
`percentile_approx_weighted` functions are all in
`ForEachCombinator.UNSUPPORTED_AGGREGATE_FUNCTION`; please add this new name
there (and extend the rejection test), unless constant-preserving foreach
support is implemented.
--
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]
