github-actions[bot] commented on code in PR #67439:
URL: https://github.com/apache/doris/pull/67439#discussion_r3912071724
##########
be/src/exprs/aggregate/aggregate_function_min_max.h:
##########
@@ -616,7 +616,7 @@ struct SingleValueDataComplexType {
}
bool change_if_less(const IColumn& column, size_t row_num, Arena& arena) {
- if (!has() || column_data->compare_at(0, row_num, column, -1) == 1) {
+ if (!has() || column_data->compare_at(0, row_num, column, 1) == 1) {
Review Comment:
`SingleValueDataComplexType` is also the state for unary complex
`min`/`max`, not just the `*_by` key. With this +1, nested `ColumnNullable`
treats NULL as greatest, so `[10,NULL]` beats `[10,5]` for `max(arr)` and loses
for `min(arr)`. That conflicts with Doris' documented ARRAY aggregate order
(NULL is smallest) and the unchanged `maxmin_array_2` expectation. Please keep
the aggregate comparator policy consistent with the documented contract (or
scope a different hint to the intended `*_by` path) and add/update plain,
merge, and window coverage.
##########
regression-test/data/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.out:
##########
@@ -395,17 +395,20 @@ true
[1, 2, 5] [11, 22, 33, 44]
-- !maxminby_array_3 --
-{"A":10, "B":1} {"x":50, "y":60}
+{"A":10, "B":5} {"x":50, "y":60}
-- !maxminby_array_4 --
-{"a":10, "b":"tt"} {"a":4, "b":"delta"}
+{"a":10, "b":"ten"} {"a":4, "b":"delta"}
+
+-- !maxminby_array_null_order --
+[11, null, null, 55] [1, 2, 3, 4]
-- !maxminby_array_5 --
1 [5, 6] [7]
2 [1, 2, 5] [1, 2]
3 [3, 1] [11, 22, 33, 44]
4 [3, 1] [10]
-5 [1, null, 3, 4] [1, 2, 3, 4]
+5 [11, null, null, 55] [1, 2, 3, 4]
Review Comment:
`max_by(arr, weight)` still uses the nullable `weight` column as an INT key.
`AggregateFunctionNullVariadicInline::add` drops any row where an argument is
NULL, so id=5's `[11, null, null, 55]` row (weight=NULL) cannot be selected;
the maximum eligible weight is 5, yielding `[1, null, 3, 4]`. Please restore
this expected value and retain the NULL-key case to verify it stays skipped.
--
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]
