github-actions[bot] commented on code in PR #65664: URL: https://github.com/apache/doris/pull/65664#discussion_r3619094302
########## regression-test/suites/nereids_function_p0/agg_function/agg_distinct_function.groovy: ########## @@ -0,0 +1,207 @@ +// 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("agg_distinct_function") { + sql "drop table if exists agg_distinct_function" + sql """ + create table agg_distinct_function ( + k int, + a int, + b int, + s varchar(10), + w int + ) engine=olap + duplicate key(k) + distributed by hash(k) buckets 1 + properties ("replication_num" = "1") + """ + + // The duplicate rows make DISTINCT observable. Every query is run both as a + // scalar aggregation and as a grouped aggregation. + sql """ + insert into agg_distinct_function values + (1, 1, 10, 'a', 1), (1, 1, 10, 'a', 1), + (1, 2, 20, 'b', 2), (1, 3, 30, 'c', 3), + (2, 1, 10, 'a', 1), (2, 4, 40, 'd', 4), + (2, 4, 40, 'd', 4), (2, 5, 50, 'e', 5) + """ + + qt_array_agg "select array_sort(array_agg(distinct a)) from agg_distinct_function" + order_qt_array_agg_group "select k, array_sort(array_agg(distinct a)) from agg_distinct_function group by k order by k" + qt_avg "select avg(distinct a) from agg_distinct_function" + order_qt_avg_group "select k, avg(distinct a) from agg_distinct_function group by k order by k" + qt_avg_weighted "select avg_weighted(distinct a, w) from agg_distinct_function" + order_qt_avg_weighted_group "select k, avg_weighted(distinct a, w) from agg_distinct_function group by k order by k" + qt_collect_list "select array_sort(collect_list(distinct a)) from agg_distinct_function" + order_qt_collect_list_group "select k, array_sort(collect_list(distinct a)) from agg_distinct_function group by k order by k" + qt_collect_set "select array_sort(collect_set(distinct a)) from agg_distinct_function" + order_qt_collect_set_group "select k, array_sort(collect_set(distinct a)) from agg_distinct_function group by k order by k" + qt_corr_welford "select corr_welford(distinct a, b) from agg_distinct_function" + order_qt_corr_welford_group "select k, corr_welford(distinct a, b) from agg_distinct_function group by k order by k" + qt_corr "select corr(distinct a, b) from agg_distinct_function" + order_qt_corr_group "select k, corr(distinct a, b) from agg_distinct_function group by k order by k" + qt_covar "select covar(distinct a, b) from agg_distinct_function" + order_qt_covar_group "select k, covar(distinct a, b) from agg_distinct_function group by k order by k" + qt_covar_samp "select covar_samp(distinct a, b) from agg_distinct_function" + order_qt_covar_samp_group "select k, covar_samp(distinct a, b) from agg_distinct_function group by k order by k" + qt_group_bit_and "select group_bit_and(distinct a) from agg_distinct_function" + order_qt_group_bit_and_group "select k, group_bit_and(distinct a) from agg_distinct_function group by k order by k" + qt_group_bit_or "select group_bit_or(distinct a) from agg_distinct_function" + order_qt_group_bit_or_group "select k, group_bit_or(distinct a) from agg_distinct_function group by k order by k" + qt_group_bit_xor "select group_bit_xor(distinct a) from agg_distinct_function" + order_qt_group_bit_xor_group "select k, group_bit_xor(distinct a) from agg_distinct_function group by k order by k" + qt_group_bitmap_xor "select bitmap_to_string(group_bitmap_xor(distinct bitmap_hash(a))) from agg_distinct_function" Review Comment: [P1] Do not record raw XOR as DISTINCT behavior `GroupBitmapXor(boolean distinct, ...)` discards this modifier by passing `false` to its superclass, while BE combines rows with bitmap `^=`. In this fixture each group has three distinct `a` values, but the new `.out` contains only two IDs: the duplicated singleton was XORed twice and canceled, which is exactly the non-DISTINCT result. This query therefore neither exercises the new DISTINCT/CTE path nor has semantics matching its SQL, and the generated output locks that behavior in. Please preserve or explicitly reject DISTINCT for this non-idempotent aggregate before listing it as supported coverage, and assert the three-value result when it is supported. ########## regression-test/suites/nereids_rules_p0/agg_strategy/distinct_agg_strategy_selector.groovy: ########## @@ -62,4 +62,43 @@ suite("distinct_agg_strategy_selector") { // test order_qt_count_distinct_group "select count(distinct a_1,b_5), count(distinct b_5,a_1) from t1000;" order_qt_count_distinct_group_with_gby "select count(distinct a_1,b_5), count(distinct b_5,a_1) from t1000 group by c_10;" + + sql """ Review Comment: [P1] Run this setup with the multi-statement helper This block contains a `SET`, two `DROP`s, a `CREATE TABLE`, and an `INSERT`, but `sql` forwards the whole string to one JDBC `PreparedStatement`. The standard P0 URL does not enable `allowMultiQueries`; `multi_sql` is the helper that splits on semicolons, and this same suite already uses it above. As written, setup fails before the view/session-guard path is reached. Please use `multi_sql` (or separate `sql` calls), then keep the final guarded SELECT as an expected-result assertion. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeRepeat.java: ########## @@ -247,6 +247,9 @@ private static Set<Expression> collectNeedToSlotArgsOfGroupingScalarFuncAndAggFu ImmutableSet.Builder<Expression> argumentsOfAggregateFunctionBuilder = ImmutableSet.builder(); for (AggregateFunction function : aggregateFunctions) { for (Expression arg : function.getArguments()) { + if (arg.isConstant()) { Review Comment: [P2] Keep row-observable constants below Repeat Consider the reduced non-DISTINCT plan for `SUM(CAST(SLEEP(1) AS INT)) ... GROUP BY ROLLUP(k)`. Previously this argument became a slot in a `Project` below `Repeat`, so `SLEEP` ran for the N source rows. Now `isConstant()` skips it here (and later in `NormalizeAggregate`), leaving it inside the aggregate above `Repeat`: ```text Aggregate(sum(CAST(SLEEP(1) AS INT))) Repeat([[k], []]) // emits 2N rows Scan(t) // N rows ``` `Sleep` inherits deterministic=true, but BE deliberately materializes its constant and loops over every input row, so this change makes two grouping sets perform 2N sleeps (and more sets multiply it further). Unlike the existing ordinary-DISTINCT thread, this is a non-DISTINCT failure across a row-multiplying operator. Please restrict the exemption to literals/a proven defer-safe predicate or otherwise keep row-observable constants below Repeat. -- 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]
