feiniaofeiafei commented on code in PR #65245:
URL: https://github.com/apache/doris/pull/65245#discussion_r3534640049
##########
be/src/exprs/aggregate/aggregate_function_array_agg.cpp:
##########
@@ -25,9 +27,19 @@
namespace doris {
template <PrimitiveType T>
-AggregateFunctionPtr do_create_agg_function_collect(const DataTypes&
argument_types,
+AggregateFunctionPtr do_create_agg_function_collect(bool distinct, const
DataTypes& argument_types,
const bool
result_is_nullable,
const
AggregateFunctionAttr& attr) {
+ if (distinct) {
+ if constexpr (T == INVALID_TYPE) {
+ throw Exception(ErrorCode::INTERNAL_ERROR,
+ "unexpected type for array_agg distinct, please
check the input");
+ } else {
+ return creator_without_type::create<
Review Comment:
**P1 — `array_agg(DISTINCT nullable_col)` drops NULL.** The normal nullable
`array_agg` path uses `create_ignore_nullable` and lets
`AggregateFunctionArrayAggData` preserve the null map. This new set-based path
uses the regular nullable adapter, which skips null input rows before the inner
aggregate sees them, and `AggregateFunctionCollectSetData` has no state for a
distinct NULL. As a result, the multi-distinct plan returns an array without
NULL, while the existing single-distinct path should retain one NULL. Please
preserve one distinct NULL explicitly and add a regression case with nullable
input.
##########
regression-test/suites/nereids_p0/multi_distinct/multi_distinct_collect_list.groovy:
##########
@@ -0,0 +1,105 @@
+// 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.
+
+suite('multi_distinct_collect_list') {
+ sql "drop table if exists multi_distinct_cl;"
+ sql """
+ CREATE TABLE multi_distinct_cl (
+ g int,
+ a varchar(16),
+ b varchar(16)
+ ) ENGINE = OLAP
+ DUPLICATE KEY(g) COMMENT 'OLAP'
+ DISTRIBUTED BY HASH(g) BUCKETS 2
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ );
+ """
+
+ // g=1: a in (x,x,y) b in (p,p,q); g=2: a in (m,m,n) b in (r,r,s) --
duplicates per group make dedup observable.
+ sql """
+ insert into multi_distinct_cl values
+ (1, 'x', 'p'), (1, 'x', 'p'), (1, 'y', 'q'),
+ (2, 'm', 'r'), (2, 'm', 'r'), (2, 'n', 's');
+ """
+
+ // (a) Multiple distinct aggregates in one GROUP BY now plan (previously
threw "can't support multi distinct"); explain proves the multi_distinct_*
rewrite fired.
+ explain {
+ sql "select g, collect_list(distinct a), collect_list(distinct b) from
multi_distinct_cl group by g"
+ contains "multi_distinct_collect_list"
+ }
+ explain {
+ sql "select g, array_agg(distinct a), array_agg(distinct b) from
multi_distinct_cl group by g"
+ contains "multi_distinct_array_agg"
+ }
+ // Mixed collect_list + array_agg, both DISTINCT, still on the
multi-distinct path.
+ explain {
+ sql "select g, collect_list(distinct a), array_agg(distinct b) from
multi_distinct_cl group by g"
+ contains "multi_distinct_collect_list"
+ }
+
+ // (b) DISTINCT dedups. Two distinct args (a AND b) exercise the
multi-distinct path (getDistinctArguments().size() > 1); array_sort() makes
element order deterministic.
+ def cl = sql """
+ select g,
+ array_sort(collect_list(distinct a)) la,
+ array_sort(collect_list(distinct b)) lb
+ from multi_distinct_cl
+ group by g
+ order by g
+ """
+ assertEquals(2, cl.size())
Review Comment:
**P2 — This result check does not follow the regression-test convention.**
Deterministic expected results should use `qt_`/`order_qt_` and a generated
`.out` file instead of manual `assertEquals` checks. Please convert these
result assertions accordingly. The test data is also entirely non-null, so it
misses the `array_agg(DISTINCT nullable_col)` semantic regression above; please
add nullable input and the supported/unsupported type boundary cases.
##########
be/src/exprs/aggregate/aggregate_function_array_agg.cpp:
##########
@@ -25,9 +27,19 @@
namespace doris {
template <PrimitiveType T>
-AggregateFunctionPtr do_create_agg_function_collect(const DataTypes&
argument_types,
+AggregateFunctionPtr do_create_agg_function_collect(bool distinct, const
DataTypes& argument_types,
const bool
result_is_nullable,
const
AggregateFunctionAttr& attr) {
+ if (distinct) {
+ if constexpr (T == INVALID_TYPE) {
Review Comment:
**P2 — FE and BE advertise different type support.** `MultiDistinctArrayAgg`
declares an `AnyDataType` argument, so FE can accept types that
`dispatch_switch_all` does not dispatch, but this branch turns them into an
`INTERNAL_ERROR` at execution time. Please either implement the distinct state
for every FE-advertised type or reject unsupported types during FE analysis
with a user-facing error. An accepted query must not fail later with an
internal error.
##########
be/src/exprs/aggregate/aggregate_function_array_agg.cpp:
##########
@@ -44,23 +56,25 @@ AggregateFunctionPtr
create_aggregate_function_array_agg(const std::string& name
const DataTypePtr&
result_type,
const bool
result_is_nullable,
const
AggregateFunctionAttr& attr) {
+ bool distinct = name == "multi_distinct_array_agg";
AggregateFunctionPtr agg_fn;
auto call = [&](const auto& type) -> bool {
using DispatcType = std::decay_t<decltype(type)>;
- agg_fn =
do_create_agg_function_collect<DispatcType::PType>(argument_types,
+ agg_fn = do_create_agg_function_collect<DispatcType::PType>(distinct,
argument_types,
result_is_nullable, attr);
return true;
};
if (!dispatch_switch_all(argument_types[0]->get_primitive_type(), call)) {
// We do not care what the real type is.
- agg_fn = do_create_agg_function_collect<INVALID_TYPE>(argument_types,
result_is_nullable,
- attr);
+ agg_fn = do_create_agg_function_collect<INVALID_TYPE>(distinct,
argument_types,
+
result_is_nullable, attr);
}
return agg_fn;
}
void register_aggregate_function_array_agg(AggregateFunctionSimpleFactory&
factory) {
factory.register_function_both("array_agg",
create_aggregate_function_array_agg);
+ factory.register_function_both("multi_distinct_array_agg",
create_aggregate_function_array_agg);
Review Comment:
**P3 — The registered function is reported as `collect_set` by BE.** This
implementation instantiates
`AggregateFunctionCollect<AggregateFunctionCollectSetData<...>>`, whose
`get_name()` returns `collect_set`. Therefore `multi_distinct_array_agg` (and
similarly `multi_distinct_collect_list`) is mislabeled in profiles and
error/debug output. Please preserve the registered function identity, for
example by passing/storing the logical function name or using a dedicated
implementation whose `get_name()` returns the correct name.
--
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]
