implement pr feedback

src/include/duckdb/planner/expression_binder/base_select_binder.hpp

@@ -21,7 +21,7 @@ class BoundSelectNode;
 struct BoundGroupInformation {
 	parsed_expression_map_t<idx_t> map;
 	case_insensitive_map_t<idx_t> alias_map;
-	unordered_map<idx_t, idx_t> implicit_collate;
+	unordered_map<idx_t, idx_t> collated_groups;
 };
 
 //! The BaseSelectBinder is the base binder of the SELECT, HAVING and QUALIFY binders. It can bind aggregates and window

src/planner/binder/query_node/bind_select_node.cpp

@@ -383,24 +383,23 @@ unique_ptr<BoundQueryNode> 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<unique_ptr<Expression>> 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

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<BoundColumnRefExpression>(expr.GetName(), node.aggregates[aggr_index]->return_type,

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