This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new 5601d87507e [fix](be) Respect NULL placement in ordered GROUP_CONCAT
(#68980) (#69669)
5601d87507e is described below
commit 5601d87507e8c0f1bbdb5a6bd9060cdbe86091dd
Author: TengJianPing <[email protected]>
AuthorDate: Tue Oct 27 18:57:07 2026 +0800
[fix](be) Respect NULL placement in ordered GROUP_CONCAT (#68980) (#69669)
### What problem does this PR solve?
Issue Number: N/A
Related PR: #68980
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.
Resolve the branch-4.2 unit test conflict by adding only this PR's test
and helpers with their required includes, retaining all three existing
tests without importing unrelated master-only tests.
### 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 48;
ASAN build completed and all 4 tests passed, including 16 combinations
in the new test
- Validation: clang-format 16 formatting and check scripts passed for
the two modified C++ files
- Validation: both modified C++ files passed Clang Analyzer with the
toolchain resource directory explicitly supplied
- Full clang-tidy could not complete because of a pre-existing unmatched
NOLINTEND in be/src/core/types.h and the default toolchain resource path
- SQL regression files preserved from the original PR; SQL regression
not rerun for this backport
- Behavior changed: Yes, correct descending NULL placement in ordered
GROUP_CONCAT
- Does this need documentation: No
(cherry picked from commit 3d7479ac946a2419856e824fb316573cc7d39417)
### 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 | 129 +++++++++++++++++++++
.../aggregate/test_group_concat_null_order.out | 63 ++++++++++
.../aggregate/test_group_concat_null_order.groovy | 75 ++++++++++++
4 files changed, 271 insertions(+), 3 deletions(-)
diff --git a/be/src/exprs/vectorized_agg_fn.cpp
b/be/src/exprs/vectorized_agg_fn.cpp
index a093693dbc7..6a16914978c 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 03a070d6ade..86d4baa54f6 100644
--- a/be/test/exec/agg/agg_fn_evaluator_test.cpp
+++ b/be/test/exec/agg/agg_fn_evaluator_test.cpp
@@ -15,15 +15,74 @@
// specific language governing permissions and limitations
// under the License.
+#include <gen_cpp/PlanNodes_types.h>
#include <gtest/gtest.h>
+#include <string>
+#include <vector>
+
+#include "agent/be_exec_version_manager.h"
#include "common/object_pool.h"
+#include "core/column/column_string.h"
+#include "core/data_type/data_type_nullable.h"
#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"
namespace doris {
+namespace {
+
+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) {
ObjectPool pool;
@@ -138,4 +197,74 @@ 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);
+ }
+ }
+ }
+}
+
} // namespace doris
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]
