From 52e58e1a74f5247d59a97dec3033ca4e37fdaf9b Mon Sep 17 00:00:00 2001 From: Laurens Kuiper Date: Tue, 19 Sep 2023 10:20:54 +0200 Subject: [PATCH] implement pr feedback --- .../expression_binder/base_select_binder.hpp | 2 +- .../binder/query_node/bind_select_node.cpp | 19 +++++++++---------- .../expression_binder/base_select_binder.cpp | 4 ++-- test/issues/general/test_3821.test | 16 ++++++++++++---- 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/include/duckdb/planner/expression_binder/base_select_binder.hpp b/src/include/duckdb/planner/expression_binder/base_select_binder.hpp index d025f1f5abd8..c05e1d8d99f1 100644 --- a/src/include/duckdb/planner/expression_binder/base_select_binder.hpp +++ b/src/include/duckdb/planner/expression_binder/base_select_binder.hpp @@ -21,7 +21,7 @@ class BoundSelectNode; struct BoundGroupInformation { parsed_expression_map_t map; case_insensitive_map_t alias_map; - unordered_map implicit_collate; + unordered_map collated_groups; }; //! The BaseSelectBinder is the base binder of the SELECT, HAVING and QUALIFY binders. It can bind aggregates and window diff --git a/src/planner/binder/query_node/bind_select_node.cpp b/src/planner/binder/query_node/bind_select_node.cpp index 0952af07314d..ba5759030a99 100644 --- a/src/planner/binder/query_node/bind_select_node.cpp +++ b/src/planner/binder/query_node/bind_select_node.cpp @@ -383,24 +383,23 @@ unique_ptr Binder::BindSelectNode(SelectNode &statement, unique_ auto bound_expr = group_binder.Bind(group_expressions[i], &group_type); D_ASSERT(bound_expr->return_type.id() != LogicalTypeId::INVALID); - if (group_type.id() == LogicalTypeId::VARCHAR && !DBConfig::GetConfig(context).options.collation.empty()) { - // TODO If there is a collation on a group x, we should group by the collated expr, - // but also push a first(x) aggregate) - info.implicit_collate[i] = result->aggregates.size(); + // push a potential collation, if necessary + auto collated_expr = ExpressionBinder::PushCollation(context, bound_expr->Copy(), + StringType::GetCollation(group_type), true); + if (!collated_expr->Equals(*bound_expr)) { + // If there is a collation on a group x, we should group by the collated expr, + // but also push a first(x) aggregate in case x is selected (uncollated) + info.collated_groups[i] = result->aggregates.size(); auto first_fun = FirstFun::GetFunction(LogicalType::VARCHAR); vector> first_children; - first_children.push_back(bound_expr->Copy()); + first_children.push_back(std::move(bound_expr)); FunctionBinder function_binder(context); auto function = function_binder.BindAggregateFunction(first_fun, std::move(first_children)); result->aggregates.push_back(std::move(function)); } - - // push a potential collation, if necessary - bound_expr = ExpressionBinder::PushCollation(context, std::move(bound_expr), - StringType::GetCollation(group_type), true); - result->groups.group_expressions.push_back(std::move(bound_expr)); + result->groups.group_expressions.push_back(std::move(collated_expr)); // in the unbound expression we DO bind the table names of any ColumnRefs // we do this to make sure that "table.a" and "a" are treated the same diff --git a/src/planner/expression_binder/base_select_binder.cpp b/src/planner/expression_binder/base_select_binder.cpp index 1810fa6c2f44..542bfa52625d 100644 --- a/src/planner/expression_binder/base_select_binder.cpp +++ b/src/planner/expression_binder/base_select_binder.cpp @@ -138,8 +138,8 @@ BindResult BaseSelectBinder::BindGroupingFunction(OperatorExpression &op, idx_t } BindResult BaseSelectBinder::BindGroup(ParsedExpression &expr, idx_t depth, idx_t group_index) { - auto it = info.implicit_collate.find(group_index); - if (it != info.implicit_collate.end()) { + auto it = info.collated_groups.find(group_index); + if (it != info.collated_groups.end()) { // This is an implicitly collated group, so we need to refer to the first() aggregate const auto &aggr_index = it->second; return BindResult(make_uniq(expr.GetName(), node.aggregates[aggr_index]->return_type, diff --git a/test/issues/general/test_3821.test b/test/issues/general/test_3821.test index d535bb49c5d4..baddbc45826f 100644 --- a/test/issues/general/test_3821.test +++ b/test/issues/general/test_3821.test @@ -1,17 +1,25 @@ # name: test/issues/general/test_3821.test -# description: Issue 3821:Setting default_collation=NOCASE modifies values of GROUP BY queries +# description: Issue 3821: Setting default_collation=NOCASE modifies values of GROUP BY queries # group: [general] -#statement ok -#PRAGMA enable_verification +statement ok +PRAGMA enable_verification statement ok -CREATE TABLE t AS (SELECT 'hello' AS x UNION ALL SELECT 'WORLD' AS x); +CREATE TABLE t AS (SELECT 'hello' AS x UNION ALL SELECT 'WORLD' AS x UNION ALL SELECT 'WoRlD' AS x); +# 3 groups because no collation query I rowsort SELECT x FROM t GROUP BY 1; ---- WORLD +WoRlD +hello + +query I rowsort +SELECT x COLLATE NOCASE FROM t GROUP BY 1; +---- +WORLD hello statement ok