Skip to content

Commit

Permalink
fix 3821
Browse files Browse the repository at this point in the history
  • Loading branch information
lnkuiper committed Sep 18, 2023
1 parent 4f50fee commit f5cf11c
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +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;
};

//! The BaseSelectBinder is the base binder of the SELECT, HAVING and QUALIFY binders. It can bind aggregates and window
Expand Down
23 changes: 20 additions & 3 deletions src/planner/binder/query_node/bind_select_node.cpp
Original file line number Diff line number Diff line change
@@ -1,25 +1,28 @@
#include "duckdb/common/limits.hpp"
#include "duckdb/common/string_util.hpp"
#include "duckdb/execution/expression_executor.hpp"
#include "duckdb/function/aggregate/distributive_functions.hpp"
#include "duckdb/function/function_binder.hpp"
#include "duckdb/main/config.hpp"
#include "duckdb/parser/expression/columnref_expression.hpp"
#include "duckdb/parser/expression/comparison_expression.hpp"
#include "duckdb/parser/expression/conjunction_expression.hpp"
#include "duckdb/parser/expression/constant_expression.hpp"
#include "duckdb/parser/expression/subquery_expression.hpp"
#include "duckdb/parser/expression/star_expression.hpp"
#include "duckdb/parser/expression/subquery_expression.hpp"
#include "duckdb/parser/query_node/select_node.hpp"
#include "duckdb/parser/tableref/joinref.hpp"
#include "duckdb/planner/binder.hpp"
#include "duckdb/planner/expression/bound_aggregate_expression.hpp"
#include "duckdb/planner/expression_binder/column_alias_binder.hpp"
#include "duckdb/planner/expression_binder/constant_binder.hpp"
#include "duckdb/planner/expression_binder/group_binder.hpp"
#include "duckdb/planner/expression_binder/having_binder.hpp"
#include "duckdb/planner/expression_binder/qualify_binder.hpp"
#include "duckdb/planner/expression_binder/order_binder.hpp"
#include "duckdb/planner/expression_binder/qualify_binder.hpp"
#include "duckdb/planner/expression_binder/select_binder.hpp"
#include "duckdb/planner/expression_binder/where_binder.hpp"
#include "duckdb/planner/query_node/bound_select_node.hpp"
#include "duckdb/parser/expression/conjunction_expression.hpp"

namespace duckdb {

Expand Down Expand Up @@ -380,6 +383,20 @@ 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();

auto first_fun = FirstFun::GetFunction(LogicalType::VARCHAR);
vector<unique_ptr<Expression>> first_children;
first_children.push_back(bound_expr->Copy());

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);
Expand Down
20 changes: 14 additions & 6 deletions src/planner/expression_binder/base_select_binder.cpp
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
#include "duckdb/planner/expression_binder/base_select_binder.hpp"

#include "duckdb/common/string_util.hpp"
#include "duckdb/parser/expression/columnref_expression.hpp"
#include "duckdb/parser/expression/operator_expression.hpp"
#include "duckdb/parser/expression/window_expression.hpp"
#include "duckdb/parser/parsed_expression_iterator.hpp"
#include "duckdb/planner/binder.hpp"
#include "duckdb/planner/expression/bound_columnref_expression.hpp"
#include "duckdb/planner/expression/bound_window_expression.hpp"
#include "duckdb/planner/expression_binder/aggregate_binder.hpp"
#include "duckdb/planner/query_node/bound_select_node.hpp"
#include "duckdb/parser/expression/operator_expression.hpp"
#include "duckdb/common/string_util.hpp"
#include "duckdb/planner/binder.hpp"

namespace duckdb {

Expand Down Expand Up @@ -138,9 +138,17 @@ BindResult BaseSelectBinder::BindGroupingFunction(OperatorExpression &op, idx_t
}

BindResult BaseSelectBinder::BindGroup(ParsedExpression &expr, idx_t depth, idx_t group_index) {
auto &group = node.groups.group_expressions[group_index];
return BindResult(make_uniq<BoundColumnRefExpression>(expr.GetName(), group->return_type,
ColumnBinding(node.group_index, group_index), depth));
auto it = info.implicit_collate.find(group_index);
if (it != info.implicit_collate.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,
ColumnBinding(node.aggregate_index, aggr_index), depth));
} else {
auto &group = node.groups.group_expressions[group_index];
return BindResult(make_uniq<BoundColumnRefExpression>(expr.GetName(), group->return_type,
ColumnBinding(node.group_index, group_index), depth));
}
}

bool BaseSelectBinder::QualifyColumnAlias(const ColumnRefExpression &colref) {
Expand Down
24 changes: 24 additions & 0 deletions test/issues/general/test_3821.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# name: test/issues/general/test_3821.test
# description: Issue 3821:Setting default_collation=NOCASE modifies values of GROUP BY queries
# group: [general]

#statement ok
#PRAGMA enable_verification

statement ok
CREATE TABLE t AS (SELECT 'hello' AS x UNION ALL SELECT 'WORLD' AS x);

query I rowsort
SELECT x FROM t GROUP BY 1;
----
WORLD
hello

statement ok
PRAGMA default_collation=NOCASE;

query I rowsort
SELECT x FROM t GROUP BY 1;
----
WORLD
hello

0 comments on commit f5cf11c

Please sign in to comment.