github-actions[bot] commented on code in PR #64624:
URL: https://github.com/apache/doris/pull/64624#discussion_r3671170638
##########
regression-test/suites/query_p0/aggregate/support_type/percentile/percentile.groovy:
##########
@@ -62,6 +62,45 @@ suite("percentile") {
qt_percentile_largeint """select percentile(col_largeint, 0.5) from
d_table;"""
qt_percentile_float """select percentile(col_float, 0.5) from d_table;"""
qt_percentile_double """select percentile(col_double, 0.5) from d_table;"""
+ qt_percentile_distinct_const_arg """select percentile(distinct col_double,
cast('0.5' as double)) from d_table;"""
+ sql """set debug_skip_fold_constant=true;"""
+ qt_percentile_const_expr_materialized """select percentile(col_double,
coalesce(cast(null as double), cast(0.5 as double))) from d_table;"""
+ test {
+ // NormalizeRepeat materializes the constant expression as a Slot in
old FE plans.
+ sql """select percentile(col_double, 0.1 + 0.1) from d_table group by
grouping sets ((k1), ());"""
+ }
+ sql """set enable_aggregate_function_null_v2=false;"""
+ qt_percentile_const_nullable_null_v1 """select percentile(col_double,
coalesce(cast(null as double), cast(null as double))) from d_table;"""
+ sql """set enable_aggregate_function_null_v2=true;"""
+ qt_percentile_const_nullable_null_v2 """select percentile(col_double,
coalesce(cast(null as double), cast(null as double))) from d_table;"""
+ sql """set enable_aggregate_function_null_v2=false;"""
+ sql """set debug_skip_fold_constant=false;"""
+
+ sql """drop table if exists percentile_nullable_t;"""
+ sql """
+ create table percentile_nullable_t (
+ id int,
+ v double null
+ ) duplicate key(id)
+ distributed by hash(id) buckets 1
+ properties("replication_num" = "1");
+ """
+ sql """
+ insert into percentile_nullable_t values
+ (1, null),
+ (2, 1.0),
+ (3, 2.0);
+ """
+ sql """set enable_aggregate_function_null_v2=false;"""
+ qt_percentile_nullable_input_v1 """select percentile(v, 0.5) from
percentile_nullable_t;"""
+ order_qt_percentile_window_nullable_v1 """select id, percentile(v, 0.5)
over(order by id rows between 1 preceding and current row) from
percentile_nullable_t order by id;"""
+ sql """set debug_skip_fold_constant=true;"""
+ order_qt_percentile_window_const_expr """select id, percentile(v,
coalesce(cast(null as double), cast(0.5 as double))) over(order by id rows
between 1 preceding and current row) from percentile_nullable_t order by id;"""
+ sql """set debug_skip_fold_constant=false;"""
+ sql """set enable_aggregate_function_null_v2=true;"""
+ qt_percentile_nullable_input_v2 """select percentile(v, 0.5) from
percentile_nullable_t;"""
+ order_qt_percentile_window_nullable_v2 """select id, percentile(v, 0.5)
over(order by id rows between 1 preceding and current row) from
percentile_nullable_t order by id;"""
+ sql """set enable_aggregate_function_null_v2=false;"""
Review Comment:
[P2] This leaves the suite on the legacy null-v1 implementation after the
paired v1/v2 checks. The session-variable default is `true`, so all of the
pre-existing NaN percentile cases below now silently stop exercising the
default null-v2 wrapper. Please restore `true` (or the captured prior value)
here so the added comparison does not change the remaining suite's coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggCombinerFunctionBuilder.java:
##########
@@ -138,6 +139,11 @@ public Pair<BoundFunction, AggregateFunction> build(String
name, List<?> argumen
if (!arguments.isEmpty() && arguments.get(0) instanceof Boolean &&
(Boolean) arguments.get(0)) {
arguments = arguments.subList(1, arguments.size());
}
+ // AIAgg's two-argument constructor injects the default resource
into the nested function.
+ // Include it in the state arguments because BE always consumes
[resource, input, task].
+ if (nestedFunction instanceof AIAgg && arguments.size() == 2) {
+ arguments = nestedFunction.getArguments();
Review Comment:
[P1] This makes the default-resource `ai_agg_state(input, task)` form bind,
but it still cannot execute. The BE scalar `_state` path constructs
`FunctionAggState` without ever calling `set_query_context()` on the nested
aggregate; serialization creates `AggregateFunctionAIAggData` with a null
context, and its first `add()` dereferences `_ctx` while loading AI resources.
Please propagate the current query context into the nested aggregate for
FunctionAggState (including nullable wrappers) and add a runtime serialization
test for this newly reachable form.
##########
be/src/exprs/aggregate/aggregate_function_null.h:
##########
@@ -606,11 +633,23 @@ class AggregateFunctionNullVariadicInline final
void add(AggregateDataPtr __restrict place, const IColumn** columns,
ssize_t row_num,
Arena& arena) const override {
+ if (this->check_always_const_col_has_null(columns)) {
Review Comment:
[P2] This adds an extra full argument scan to every row handled by a
variadic nullable aggregate, even when the nested function has no
semantic-constant arguments (the common case for functions such as
`covar_pop`). The inherited keyed, selected, single-place, range, and streaming
helpers all dispatch through this `add()`, so the redundant scan is on the core
aggregation hot path. Please fold the constant-NULL check into the existing
argument loop, or precompute/retain only nullable semantic-constant indexes and
bypass it entirely when that set is empty.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/DistinctAggregateRewriterTest.java:
##########
@@ -245,6 +248,31 @@ void testMultiDistinct() {
connectContext.getSessionVariable().setAggPhase(0);
}
+ @Test
+ void testDistinctAggregatePreservesNonLiteralConstantArguments() {
+ assertOptimizeSuccess(
+ "select percentile(distinct a, cast('0.5' as double)) from
test.distinct_agg_split_t");
+ assertOptimizeSuccess(
+ "select percentile_approx(distinct a, cast('0.5' as double),
cast('2048' as double)) "
+ + "from test.distinct_agg_split_t");
+ assertOptimizeSuccess(
+ "select percentile_array(distinct a, array(cast('0.5' as
double), cast('0.8' as double))) "
+ + "from test.distinct_agg_split_t");
+ assertOptimizeSuccess(
+ "select percentile_reservoir(distinct a, cast('0.5' as
double)) "
+ + "from test.distinct_agg_split_t");
+ }
+
+ @Test
+ void testAIAggDistinctIgnoresResourceAndTaskArguments() {
+ SlotReference input = new SlotReference("input", StringType.INSTANCE);
+ AIAgg aiAgg = new AIAgg(new StringLiteral("resource"), input, new
StringLiteral("task"))
+ .withDistinctAndChildren(true, ImmutableList.of(
+ new StringLiteral("resource"), input, new
StringLiteral("task")));
+
+ Assertions.assertEquals(ImmutableList.of(input),
aiAgg.getDistinctArguments());
Review Comment:
[P1] This assertion cannot pass with the production implementation in this
patch. `AIAgg` does not override `getDistinctArguments()`, so the inherited
method returns all three arguments for a DISTINCT call (`resource`, `input`,
and `task`), not just `input`. This also makes the resource/task values
participate as DISTINCT keys. Please implement the AIAgg-specific
distinct-argument contract (while separately preserving the literals through
normalization) so this test and the planner agree.
--
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]
