This is an automated email from the ASF dual-hosted git repository.
jacktengg pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 3d7479ac946 [fix](be) Respect NULL placement in ordered GROUP_CONCAT
(#68980)
3d7479ac946 is described below
commit 3d7479ac946a2419856e824fb316573cc7d39417
Author: TengJianPing <[email protected]>
AuthorDate: Tue Oct 13 11:39:53 2026 +0800
[fix](be) Respect NULL placement in ordered GROUP_CONCAT (#68980)
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: GROUP_CONCAT reversed NULLS FIRST and NULLS LAST when
an ORDER BY key was descending, for both ordinary and DISTINCT
aggregation in one-stage and two-stage plans. With values ('a', NULL)
and ('b', 1), DESC NULLS LAST returned a|b instead of b|a, while DESC
NULLS FIRST returned b|a instead of a|b. AggFnEvaluator constructed
nulls_direction without accounting for the sort direction, which the
comparator multiplies into its result. Construct the null comparison
direction with the same rule as the general sorter. Add BE coverage for
all four direction/NULL placement combinations, with and without
DISTINCT and serialized merges, and regression coverage for both
aggregation phases, grouping, duplicate values, multiple sort keys,
all-NULL keys, and the default separator.
### Release note
GROUP_CONCAT now respects NULLS FIRST and NULLS LAST for descending
ORDER BY keys, including DISTINCT and two-stage aggregation.
### Check List (For Author)
- Test:
- Unit Test: run-be-ut.sh --run --filter=AggFnEvaluatorTest.* -j 28; all
25 tests passed, including 16 combinations in the new test
- Regression test: run-regression-test.sh --run -d query_p0/aggregate -s
test_group_concat_null_order,agg_group_concat; both suites passed
- Manual test: verified the original two-row reproduction for ordinary
and DISTINCT aggregation with agg_phase=1 and agg_phase=2
- Validation: ASAN build.sh --be -j 16, clang-format 16, clang-tidy, and
build hygiene checks passed; regression output was generated by the test
script and independently checked
- Behavior changed: Yes, correct descending NULL placement in ordered
GROUP_CONCAT
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
---
be/src/exprs/vectorized_agg_fn.cpp | 7 +-
be/test/exec/agg/agg_fn_evaluator_test.cpp | 118 +++++++++++++++++++++
.../aggregate/test_group_concat_null_order.out | 63 +++++++++++
.../aggregate/test_group_concat_null_order.groovy | 75 +++++++++++++
4 files changed, 260 insertions(+), 3 deletions(-)
diff --git a/be/src/exprs/vectorized_agg_fn.cpp
b/be/src/exprs/vectorized_agg_fn.cpp
index 208e4d04882..19b50ebd8ee 100644
--- a/be/src/exprs/vectorized_agg_fn.cpp
+++ b/be/src/exprs/vectorized_agg_fn.cpp
@@ -104,9 +104,10 @@ Status AggFnEvaluator::create(ObjectPool* pool, const
TExpr& desc, const TSortIn
// Child arguments contains [real arguments, order by arguments], we pass
the arguments
// to the order by functions
for (int i = 0; i < sort_size; ++i) {
- agg_fn_evaluator->_sort_description.emplace_back(real_arguments_size +
i,
-
sort_info.is_asc_order[i] ? 1 : -1,
-
sort_info.nulls_first[i] ? -1 : 1);
+ const int direction = sort_info.is_asc_order[i] ? 1 : -1;
+ agg_fn_evaluator->_sort_description.emplace_back(
+ real_arguments_size + i, direction,
+ sort_info.nulls_first[i] ? -direction : direction);
}
// Pass the real arguments to get functions
diff --git a/be/test/exec/agg/agg_fn_evaluator_test.cpp
b/be/test/exec/agg/agg_fn_evaluator_test.cpp
index 940b249aa84..1e533243adb 100644
--- a/be/test/exec/agg/agg_fn_evaluator_test.cpp
+++ b/be/test/exec/agg/agg_fn_evaluator_test.cpp
@@ -15,6 +15,7 @@
// specific language governing permissions and limitations
// under the License.
+#include <gen_cpp/PlanNodes_types.h>
#include <gtest/gtest.h>
#include <string>
@@ -35,6 +36,7 @@
#include "core/data_type/data_type_number.h"
#include "core/data_type/data_type_string.h"
#include "exprs/aggregate/aggregate_function_simple_factory.h"
+#include "exprs/aggregate/aggregate_function_sort.h"
#include "runtime/runtime_state.h"
#include "testutil/column_helper.h"
#include "testutil/mock/mock_agg_fn_evaluator.h"
@@ -114,6 +116,52 @@ MutableColumnPtr
create_map_nullable_string64_key_column(bool nullable_values) {
std::move(offsets));
}
+TExpr create_group_concat_expr(const std::string& function_name, const
DataTypes& argument_types) {
+ TExpr expr;
+ TExprNode root;
+ root.node_type = TExprNodeType::AGG_EXPR;
+ root.num_children = argument_types.size();
+ root.fn.name.function_name = function_name;
+ root.fn.ret_type = argument_types[0]->to_thrift();
+ root.__set_is_nullable(false);
+ std::vector<TTypeDesc> param_types;
+ for (const auto& type : argument_types) {
+ auto param_type = type->to_thrift();
+ param_type.__set_is_nullable(type->is_nullable());
+ param_types.push_back(param_type);
+ }
+ root.agg_expr.__set_param_types(param_types);
+ expr.nodes.push_back(root);
+ for (int i = 0; i < argument_types.size(); ++i) {
+ TExprNode child;
+ child.node_type = TExprNodeType::SLOT_REF;
+ child.num_children = 0;
+ child.type = param_types[i];
+ child.__set_is_nullable(argument_types[i]->is_nullable());
+ child.slot_ref.slot_id = i;
+ child.slot_ref.tuple_id = 0;
+ expr.nodes.push_back(child);
+ }
+ return expr;
+}
+
+void add_group_concat_rows(const IAggregateFunction& function,
AggregateDataPtr result,
+ const IColumn** columns, bool merge, Arena& arena) {
+ if (!merge) {
+ function.add_batch_single_place(columns[0]->size(), result, columns,
arena);
+ return;
+ }
+ for (size_t offset : {0, 2}) {
+ AggregateFunctionGuard partial(&function);
+ for (size_t row = offset; row < offset + 2; ++row) {
+ function.add(partial.data(), columns, row, arena);
+ }
+ auto serialized = function.create_serialize_column();
+ function.serialize_without_key_to_column(partial.data(), *serialized);
+ function.deserialize_and_merge_from_column(result, *serialized, arena);
+ }
+}
+
} // namespace
TEST(AggFnEvaluatorTest, test_single) {
@@ -224,6 +272,76 @@ TEST(AggFnEvaluatorTest, test_clone) {
agg_fn->reset(place2);
}
+TEST(AggFnEvaluatorTest, group_concat_order_by_null_placement) {
+ struct TestCase {
+ bool ascending;
+ bool nulls_first;
+ std::string expected;
+ std::string distinct_expected;
+ };
+ const std::vector<TestCase> test_cases = {{.ascending = true,
+ .nulls_first = true,
+ .expected = "a|a|b|c",
+ .distinct_expected = "a|b|c"},
+ {.ascending = true,
+ .nulls_first = false,
+ .expected = "b|c|a|a",
+ .distinct_expected = "b|c|a"},
+ {.ascending = false,
+ .nulls_first = true,
+ .expected = "a|a|c|b",
+ .distinct_expected = "a|c|b"},
+ {.ascending = false,
+ .nulls_first = false,
+ .expected = "c|b|a|a",
+ .distinct_expected = "c|b|a"}};
+ const DataTypes argument_types = {std::make_shared<DataTypeString>(),
+ std::make_shared<DataTypeString>(),
+
make_nullable(std::make_shared<DataTypeInt32>())};
+ for (const auto& test_case : test_cases) {
+ for (bool distinct : {false, true}) {
+ for (bool merge : {false, true}) {
+ SCOPED_TRACE(testing::Message() << "ascending=" <<
test_case.ascending
+ << " nulls_first=" <<
test_case.nulls_first
+ << " distinct=" << distinct <<
" merge=" << merge);
+ const std::string function_name =
+ distinct ? "multi_distinct_group_concat" :
"group_concat";
+ auto expr = create_group_concat_expr(function_name,
argument_types);
+ TSortInfo sort_info;
+ sort_info.ordering_exprs.resize(1);
+ sort_info.is_asc_order = {test_case.ascending};
+ sort_info.nulls_first = {test_case.nulls_first};
+ ObjectPool pool;
+ AggFnEvaluator* evaluator = nullptr;
+ ASSERT_TRUE(AggFnEvaluator::create(&pool, expr, sort_info,
true, false, &evaluator)
+ .ok());
+ RuntimeState state;
+ state._query_options.__set_be_exec_version(
+ BeExecVersionManager::get_newest_version());
+ auto nested_function =
AggregateFunctionSimpleFactory::instance().get(
+ function_name, evaluator->_real_argument_types,
argument_types[0], false,
+ BeExecVersionManager::get_newest_version(),
{.column_names = {}});
+ ASSERT_NE(nested_function, nullptr);
+ auto function = transform_to_sort_agg_function(
+ nested_function, evaluator->_argument_types_with_sort,
+ evaluator->_sort_description, &state);
+ auto values =
ColumnHelper::create_column<DataTypeString>({"a", "b", "c", "a"});
+ auto separators =
ColumnHelper::create_column<DataTypeString>({"|", "|", "|", "|"});
+ auto keys =
ColumnHelper::create_nullable_column<DataTypeInt32>({0, 1, 2, 0},
+
{1, 0, 0, 1});
+ const IColumn* columns[] = {values.get(), separators.get(),
keys.get()};
+ Arena arena;
+ AggregateFunctionGuard result(function.get());
+ add_group_concat_rows(*function, result.data(), columns,
merge, arena);
+ auto result_column = ColumnString::create();
+ function->insert_result_into(result.data(), *result_column);
+ EXPECT_EQ(result_column->get_data_at(0).to_string(),
+ distinct ? test_case.distinct_expected :
test_case.expected);
+ }
+ }
+ }
+}
+
TEST(AggFnEvaluatorTest, check_input_column_type) {
ObjectPool pool;
auto* agg_fn = create_mock_agg_fn_evaluator(pool);
diff --git
a/regression-test/data/query_p0/aggregate/test_group_concat_null_order.out
b/regression-test/data/query_p0/aggregate/test_group_concat_null_order.out
new file mode 100644
index 00000000000..1bae0b0563e
--- /dev/null
+++ b/regression-test/data/query_p0/aggregate/test_group_concat_null_order.out
@@ -0,0 +1,63 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !repro_ordinary_1 --
+b|a a|b
+
+-- !repro_distinct_1 --
+b|a a|b
+
+-- !default_separator_1 --
+b,a b,a
+
+-- !ordinary_global_1 --
+a|d|f|h|g|b|e|i|c|c|j|l b|e|i|c|c|j|l|a|d|f|h|g a|d|f|h|g|c|c|j|l|b|e|i
c|c|j|l|b|e|i|a|d|f|h|g
+
+-- !ordinary_grouped_1 --
+1 a|b b|a a|b b|a
+2 d|f|e|c|c e|c|c|d|f d|f|c|c|e c|c|e|d|f
+3 h|g h|g h|g h|g
+4 i|j i|j j|i j|i
+5 l l l l
+
+-- !distinct_global_1 --
+a|d|f|h|g|b|e|i|c|j|l b|e|i|c|j|l|a|d|f|h|g a|d|f|h|g|c|j|l|b|e|i
c|j|l|b|e|i|a|d|f|h|g
+
+-- !distinct_grouped_1 --
+1 a|b b|a a|b b|a
+2 d|f|e|c e|c|d|f d|f|c|e c|e|d|f
+3 h|g h|g h|g h|g
+4 i|j i|j j|i j|i
+5 l l l l
+
+-- !repro_ordinary_2 --
+b|a a|b
+
+-- !repro_distinct_2 --
+b|a a|b
+
+-- !default_separator_2 --
+b,a b,a
+
+-- !ordinary_global_2 --
+a|d|f|h|g|b|e|i|c|c|j|l b|e|i|c|c|j|l|a|d|f|h|g a|d|f|h|g|c|c|j|l|b|e|i
c|c|j|l|b|e|i|a|d|f|h|g
+
+-- !ordinary_grouped_2 --
+1 a|b b|a a|b b|a
+2 d|f|e|c|c e|c|c|d|f d|f|c|c|e c|c|e|d|f
+3 h|g h|g h|g h|g
+4 i|j i|j j|i j|i
+5 l l l l
+
+-- !distinct_global_2 --
+a|d|f|h|g|b|e|i|c|j|l b|e|i|c|j|l|a|d|f|h|g a|d|f|h|g|c|j|l|b|e|i
c|j|l|b|e|i|a|d|f|h|g
+
+-- !distinct_grouped_2 --
+1 a|b b|a a|b b|a
+2 d|f|e|c e|c|d|f d|f|c|e c|e|d|f
+3 h|g h|g h|g h|g
+4 i|j i|j j|i j|i
+5 l l l l
+
+-- !plain_order_by --
+b 1
+a \N
+
diff --git
a/regression-test/suites/query_p0/aggregate/test_group_concat_null_order.groovy
b/regression-test/suites/query_p0/aggregate/test_group_concat_null_order.groovy
new file mode 100644
index 00000000000..2117ad6ca46
--- /dev/null
+++
b/regression-test/suites/query_p0/aggregate/test_group_concat_null_order.groovy
@@ -0,0 +1,75 @@
+// 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("test_group_concat_null_order", "query,p0") {
+ sql "DROP TABLE IF EXISTS test_group_concat_null_order"
+ sql """
+ CREATE TABLE test_group_concat_null_order (
+ id INT, g INT, v VARCHAR(10), k INT NULL
+ ) DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 4
+ PROPERTIES('replication_num' = '1')
+ """
+ sql """
+ INSERT INTO test_group_concat_null_order VALUES
+ (1, 1, 'a', NULL), (2, 1, 'b', 1),
+ (3, 2, 'c', 2), (4, 2, 'd', NULL), (5, 2, 'e', 1),
+ (6, 2, 'f', NULL), (7, 2, 'c', 2),
+ (8, 3, 'h', NULL), (9, 3, 'g', NULL),
+ (10, 4, 'i', 1), (11, 4, 'j', 2),
+ (12, 5, NULL, NULL), (13, 5, 'l', 2)
+ """
+
+ for (def phase : [1, 2]) {
+ sql "SET agg_phase = ${phase}"
+
+ "order_qt_repro_ordinary_${phase}" """
+ SELECT GROUP_CONCAT(v, '|' ORDER BY k DESC NULLS LAST),
+ GROUP_CONCAT(v, '|' ORDER BY k DESC NULLS FIRST)
+ FROM test_group_concat_null_order WHERE g = 1
+ """
+ "order_qt_repro_distinct_${phase}" """
+ SELECT GROUP_CONCAT(DISTINCT v, '|' ORDER BY k DESC NULLS LAST),
+ GROUP_CONCAT(DISTINCT v, '|' ORDER BY k DESC NULLS FIRST)
+ FROM test_group_concat_null_order WHERE g = 1
+ """
+ "order_qt_default_separator_${phase}" """
+ SELECT GROUP_CONCAT(v ORDER BY k DESC NULLS LAST),
+ GROUP_CONCAT(DISTINCT v ORDER BY k DESC NULLS LAST)
+ FROM test_group_concat_null_order WHERE g = 1
+ """
+
+ for (def distinct : ["", "DISTINCT"]) {
+ for (def grouped : [false, true]) {
+ "order_qt_${distinct ? 'distinct' : 'ordinary'}_${grouped ?
'grouped' : 'global'}_${phase}" """
+ SELECT ${grouped ? 'g,' : ''}
+ GROUP_CONCAT(${distinct} v, '|' ORDER BY k ASC
NULLS FIRST, id),
+ GROUP_CONCAT(${distinct} v, '|' ORDER BY k ASC
NULLS LAST, id),
+ GROUP_CONCAT(${distinct} v, '|' ORDER BY k DESC
NULLS FIRST, id),
+ GROUP_CONCAT(${distinct} v, '|' ORDER BY k DESC
NULLS LAST, id)
+ FROM test_group_concat_null_order
+ ${grouped ? 'GROUP BY g ORDER BY g' : ''}
+ """
+ }
+ }
+ }
+
+ qt_plain_order_by """
+ SELECT v, k FROM test_group_concat_null_order
+ WHERE g = 1 ORDER BY k DESC NULLS LAST
+ """
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
