morrySnow commented on code in PR #65245:
URL: https://github.com/apache/doris/pull/65245#discussion_r3528220181
##########
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,
Review Comment:
**๐ Null elements are silently dropped in the multi-distinct path**
The distinct path creates
`AggregateFunctionCollect<AggregateFunctionCollectSetData<T, false>, false>`
via `creator_without_type::create`. This factory wraps the inner function in
`AggregateFunctionNullUnaryInline` (see `aggregate_function_null.h:331-342`),
which **skips null rows** and never calls the inner `add()`. Additionally,
`AggregateFunctionCollectSetData` uses `flat_hash_set<ElementType>` with no
null map โ it cannot represent null elements.
In contrast, the non-distinct array_agg path uses
`AggregateFunctionArrayAggData` which explicitly tracks nulls via a null_map.
**Concrete failure:**
```sql
SELECT array_agg(distinct col) FROM (
SELECT CAST(NULL AS INT) AS col UNION ALL SELECT 1 AS col
) t
```
Returns `[1]` instead of `[null, 1]`.
The behavior change is silent โ it only triggers when multiple distinct
aggregates cause the multi-distinct rewrite to fire.
**Suggested fix:** The multi_distinct_array_agg path needs a data structure
that combines hash-set dedup with null tracking, similar to what
`AggregateFunctionArrayAggData` does.
##########
be/src/exprs/aggregate/aggregate_function_array_agg.cpp:
##########
@@ -17,6 +17,8 @@
#include "exprs/aggregate/aggregate_function_array_agg.h"
+#include "common/exception.h"
Review Comment:
**๐งน Unused include**
`#include "common/status.h"` is added but `Status` is never used in this
file. Only `Exception` (from `"common/exception.h"`) is thrown in the new
distinct path. Consider removing this include to follow include-what-you-use
hygiene.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectList.java:
##########
@@ -102,4 +102,15 @@ public List<FunctionSignature> getSignatures() {
public Expression resultForEmptyInput() {
return new ArrayLiteral(new ArrayList<>(), this.getDataType());
}
+
+ @Override
+ public AggregateFunction convertToMultiDistinct() {
+ Preconditions.checkArgument(distinct,
Review Comment:
**โ ๏ธ Limit argument silently dropped**
When `CollectList` is called with 2 children (the `collect_list(expr,
limit)` variant), the limit argument `children.get(1)` is silently discarded.
Only `children.get(0)` is passed to `MultiDistinctCollectList`. The limit
parameter has no effect in the multi-distinct path.
Consider either:
1. Documenting that the limit is intentionally dropped in the multi-distinct
path, or
2. Simplifying to always use `child(0)` since the `children.size() == 1` vs
else branch both result in the same effective behavior.
--
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]
