Repository: duckdb/duckdb (language: python)

Issue:
Cannot use overriding column alias for aggregate expression in HAVING clause
### What happens?

DuckDB doesn't let me use aliases in the HAVING clause that I define in the SELECT clause and that override an existing column. (More precisely, it gives higher precedence to the original columns over  aliases in the SELECT when resolving column names in HAVING expressions, and then throws because the original column is not being aggregated over.)

DuckDB does let me use aliases in the HAVING clause that I define in the SELECT clause as long as they do not override an existing column. 

Prior art:
- MySQL lets me use all aliases in the HAVING clause, see https://www.db-fiddle.com/f/6CMxm3gtr2h7PWLXkJkax3/0
- SQLite allows aliases in the HAVING clause but in case of overrides gives precedence to original columns (and if those original columns are not GROUPED over, it'll filter the groups by checking the filter condition on a randomly selected row of each group, which seems insane to me)
- PostgreSQL and SQLServer don't let me use any alias in the HAVING clause.

This may as well be intentional, just opening in case there is interest in adopting MySQL behavior here, which I find more convenient, since duckdb is already deviating from PostgreSQL.

Personally, I find that:

- even in non-grouped by select statements, the aliases should take precedence over the original columns, see https://github.com/duckdb/duckdb/issues/13991
- even if not, typing the HAVING instead of the ubiquitous WHERE raises enough alerts in my brain that I'm now talking about groups, so I see even less danger of accidentally referring to the alias when the original column was meant. Also, MySQL here offers a precedent, the lack of which was given in #13991 as a reason not to make a general switch to alias precedence. 
 

### To Reproduce



```sql
SELECT b, sum(a) AS a
FROM (VALUES (1, 0), (1, 1)) t(a, b)
GROUP BY b
HAVING a > 0
```
```
Error: column t.a must appear in the GROUP BY clause or be used in an aggregate function
```

while
```sql
SELECT b, sum(a) AS c
FROM (VALUES (1, 0), (1, 1)) t(a, b)
GROUP BY b
HAVING c > 0
```
```
┌───────┬────────┐
│   b   │   c    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      1 │
│     0 │      1 │
└───────┴────────┘
```

### OS:

Linux

### DuckDB Version:

'0.10.1-dev717'

### DuckDB Client:

Python

### Full Name:

Soeren Wolfers

### Affiliation:

G-Research

### Have you tried this on the latest [nightly build](https://duckdb.org/docs/installation/?version=main)?

I have tested with a nightly build

### Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?

- [X] Yes, I have
Cannot use overriding column alias for aggregate expression in HAVING clause
### What happens?

DuckDB doesn't let me use aliases in the HAVING clause that I define in the SELECT clause and that override an existing column. (More precisely, it gives higher precedence to the original columns over  aliases in the SELECT when resolving column names in HAVING expressions, and then throws because the original column is not being aggregated over.)

DuckDB does let me use aliases in the HAVING clause that I define in the SELECT clause as long as they do not override an existing column. 

Prior art:
- MySQL lets me use all aliases in the HAVING clause, see https://www.db-fiddle.com/f/6CMxm3gtr2h7PWLXkJkax3/0
- SQLite allows aliases in the HAVING clause but in case of overrides gives precedence to original columns (and if those original columns are not GROUPED over, it'll filter the groups by checking the filter condition on a randomly selected row of each group, which seems insane to me)
- PostgreSQL and SQLServer don't let me use any alias in the HAVING clause.

This may as well be intentional, just opening in case there is interest in adopting MySQL behavior here, which I find more convenient, since duckdb is already deviating from PostgreSQL.

Personally, I find that:

- even in non-grouped by select statements, the aliases should take precedence over the original columns, see https://github.com/duckdb/duckdb/issues/13991
- even if not, typing the HAVING instead of the ubiquitous WHERE raises enough alerts in my brain that I'm now talking about groups, so I see even less danger of accidentally referring to the alias when the original column was meant. Also, MySQL here offers a precedent, the lack of which was given in #13991 as a reason not to make a general switch to alias precedence. 
 

### To Reproduce



```sql
SELECT b, sum(a) AS a
FROM (VALUES (1, 0), (1, 1)) t(a, b)
GROUP BY b
HAVING a > 0
```
```
Error: column t.a must appear in the GROUP BY clause or be used in an aggregate function
```

while
```sql
SELECT b, sum(a) AS c
FROM (VALUES (1, 0), (1, 1)) t(a, b)
GROUP BY b
HAVING c > 0
```
```
┌───────┬────────┐
│   b   │   c    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      1 │
│     0 │      1 │
└───────┴────────┘
```

### OS:

Linux

### DuckDB Version:

'0.10.1-dev717'

### DuckDB Client:

Python

### Full Name:

Soeren Wolfers

### Affiliation:

G-Research

### Have you tried this on the latest [nightly build](https://duckdb.org/docs/installation/?version=main)?

I have tested with a nightly build

### Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?

- [X] Yes, I have


Discussion / hints:
This is likely a standard behaviour. I checked it on teradata and it behaves the same.

That being said, it is unfortunate. If I wanted to filter the original column, I would do it in the `WHERE` clause, not in the `HAVING` clause. There I want to filter the result of the aggregation so the alias should be used.

It gets even worse if you use `GROUP BY ALL` - it will add the original column to the group list without warning, leading to a non-intuitive behaviour and ugly bugs. 

Consider this:
```
SELECT a, sum(b) AS b
  FROM (VALUES (1, -1), (1, 1)) t(a, b)
  GROUP BY ALL
  ;

┌───────┬────────┐
│   a   │   b    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      0 │
└───────┴────────┘
```

But putting there `HAVING` we get this without any warning:

```
SELECT a, sum(b) AS b
  FROM (VALUES (1, -1), (1, 1)) t(a, b)
  GROUP BY ALL
  HAVING b <> 0;
┌───────┬────────┐
│   a   │   b    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      1 │
│     1 │     -1 │
└───────┴────────┘
```

Instead of having no rows in the output, we see the two original rows.

Of course changing it is a breaking change but it would go in line with the DuckDB's pledge of having friendly SQL.

Thank you for your good work.
It's not standard behavior; the four major dialects all behave differently than duckdb and they behave differently in different ways (both of which are friendlier that duckdb's solution, IMO). 

I do agree with you that the behavior with GROUP BY ALL is an even clearer argument that the current behavior should be changed.
This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.
@szarnyasg This is still an issue on 1.0.0. I cannot remove the stale label myself. Is it okay to tag you in each of these (i have O(10) affected tickets) or is there an alternative solution? (For example, if the closing after 30 days happens non-automatically I guess I can just leave a comment under each ticket for the manual closer to read)
Thanks for the report!

I've looked into this - and I think the issue here is a bit more complex than just which order columns are resolved in. MySQL prefers the original columns over the aliases as well. Below are some queries that return the same result in all systems I've tested (DuckDB, MySQL, SQLite, Postgres) showing the original columns are filtered instead of the aliases:

```sql
create table t1(a int);
insert into t1 values (42), (84);
select a+1 as a from t1 group by a having a=42;
+------+
| a    |
+------+
|   43 |
+------+

create table t2(a int);
insert into t2 values (42), (84), (42);
select a as b, sum(a) as a from t2 group by b having a=42;
+------+------+
| b    | a    |
+------+------+
|   42 |   84 |
+------+------+
```

It seems to me that what MySQL/SQLite are doing is instead: filter on the original columns first, if filtering on the original column is not possible because it is part of an aggregate then we filter on an alias.

I think that's fair, and actually seems like a good solution to the issue here since the alternative is throwing an error. I would argue the `GROUP BY ALL` should adopt the same binding rule or throw an error in that scenario.

> @szarnyasg This is still an issue on 1.0.0. I cannot remove the stale label myself. Is it okay to tag you in each of these (i have O(10) affected tickets) or is there an alternative solution? (For example, if the closing after 30 days happens non-automatically I guess I can just leave a comment under each ticket for the manual closer to read)

The stalebot automatically removes the label if there's any activity in the issue.



As usual, you have a keener eye for these intricacies. lf the discussion in #13991 ever results in a way to explicitly refer to an alias, that can adopted here too, but until then your solution sounds great. 

> The stalebot automatically removes the label if there's any activity in the issue.

Good bot.
This is likely a standard behaviour. I checked it on teradata and it behaves the same.

That being said, it is unfortunate. If I wanted to filter the original column, I would do it in the `WHERE` clause, not in the `HAVING` clause. There I want to filter the result of the aggregation so the alias should be used.

It gets even worse if you use `GROUP BY ALL` - it will add the original column to the group list without warning, leading to a non-intuitive behaviour and ugly bugs. 

Consider this:
```
SELECT a, sum(b) AS b
  FROM (VALUES (1, -1), (1, 1)) t(a, b)
  GROUP BY ALL
  ;

┌───────┬────────┐
│   a   │   b    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      0 │
└───────┴────────┘
```

But putting there `HAVING` we get this without any warning:

```
SELECT a, sum(b) AS b
  FROM (VALUES (1, -1), (1, 1)) t(a, b)
  GROUP BY ALL
  HAVING b <> 0;
┌───────┬────────┐
│   a   │   b    │
│ int32 │ int128 │
├───────┼────────┤
│     1 │      1 │
│     1 │     -1 │
└───────┴────────┘
```

Instead of having no rows in the output, we see the two original rows.

Of course changing it is a breaking change but it would go in line with the DuckDB's pledge of having friendly SQL.

Thank you for your good work.
It's not standard behavior; the four major dialects all behave differently than duckdb and they behave differently in different ways (both of which are friendlier that duckdb's solution, IMO). 

I do agree with you that the behavior with GROUP BY ALL is an even clearer argument that the current behavior should be changed.
This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.
@szarnyasg This is still an issue on 1.0.0. I cannot remove the stale label myself. Is it okay to tag you in each of these (i have O(10) affected tickets) or is there an alternative solution? (For example, if the closing after 30 days happens non-automatically I guess I can just leave a comment under each ticket for the manual closer to read)
Thanks for the report!

I've looked into this - and I think the issue here is a bit more complex than just which order columns are resolved in. MySQL prefers the original columns over the aliases as well. Below are some queries that return the same result in all systems I've tested (DuckDB, MySQL, SQLite, Postgres) showing the original columns are filtered instead of the aliases:

```sql
create table t1(a int);
insert into t1 values (42), (84);
select a+1 as a from t1 group by a having a=42;
+------+
| a    |
+------+
|   43 |
+------+

create table t2(a int);
insert into t2 values (42), (84), (42);
select a as b, sum(a) as a from t2 group by b having a=42;
+------+------+
| b    | a    |
+------+------+
|   42 |   84 |
+------+------+
```

It seems to me that what MySQL/SQLite are doing is instead: filter on the original columns first, if filtering on the original column is not possible because it is part of an aggregate then we filter on an alias.

I think that's fair, and actually seems like a good solution to the issue here since the alternative is throwing an error. I would argue the `GROUP BY ALL` should adopt the same binding rule or throw an error in that scenario.

> @szarnyasg This is still an issue on 1.0.0. I cannot remove the stale label myself. Is it okay to tag you in each of these (i have O(10) affected tickets) or is there an alternative solution? (For example, if the closing after 30 days happens non-automatically I guess I can just leave a comment under each ticket for the manual closer to read)

The stalebot automatically removes the label if there's any activity in the issue.



As usual, you have a keener eye for these intricacies. lf the discussion in #13991 ever results in a way to explicitly refer to an alias, that can adopted here too, but until then your solution sounds great. 

> The stalebot automatically removes the label if there's any activity in the issue.

Good bot.

Fix patch:
diff --git a/.github/config/out_of_tree_extensions.cmake b/.github/config/out_of_tree_extensions.cmake
index 6e1af53494c9..dda2e8bd37db 100644
--- a/.github/config/out_of_tree_extensions.cmake
+++ b/.github/config/out_of_tree_extensions.cmake
@@ -104,6 +104,7 @@ duckdb_extension_load(spatial
     GIT_TAG 4107eb788f933c9e268b52f6f927a6b36b9ea440
     INCLUDE_DIR spatial/include
     TEST_DIR test/sql
+    APPLY_PATCHES
     )
 
 ################# SQLITE_SCANNER
diff --git a/src/include/duckdb/planner/expression_binder.hpp b/src/include/duckdb/planner/expression_binder.hpp
index 4cc7bb0139ee..407b1eea03f9 100644
--- a/src/include/duckdb/planner/expression_binder.hpp
+++ b/src/include/duckdb/planner/expression_binder.hpp
@@ -107,7 +107,7 @@ class ExpressionBinder {
 	//! Returns a qualified column reference from a column reference with column_names.size() > 2
 	unique_ptr<ParsedExpression> QualifyColumnNameWithManyDots(ColumnRefExpression &col_ref, ErrorData &error);
 	//! Returns a qualified column reference from a column reference
-	unique_ptr<ParsedExpression> QualifyColumnName(ColumnRefExpression &col_ref, ErrorData &error);
+	virtual unique_ptr<ParsedExpression> QualifyColumnName(ColumnRefExpression &col_ref, ErrorData &error);
 	//! Enables special-handling of lambda parameters by tracking them in the lambda_params vector
 	void QualifyColumnNamesInLambda(FunctionExpression &function, vector<unordered_set<string>> &lambda_params);
 	//! Recursively qualifies the column references in the (children) of the expression. Passes on the
@@ -116,6 +116,7 @@ class ExpressionBinder {
 	                        const bool within_function_expression = false);
 	//! Entry point for qualifying the column references of the expression
 	static void QualifyColumnNames(Binder &binder, unique_ptr<ParsedExpression> &expr);
+	static void QualifyColumnNames(ExpressionBinder &binder, unique_ptr<ParsedExpression> &expr);
 
 	static bool PushCollation(ClientContext &context, unique_ptr<Expression> &source, const LogicalType &sql_type);
 	static void TestCollation(ClientContext &context, const string &collation);
diff --git a/src/include/duckdb/planner/expression_binder/having_binder.hpp b/src/include/duckdb/planner/expression_binder/having_binder.hpp
index 4cb65dab041e..b111cc368fa4 100644
--- a/src/include/duckdb/planner/expression_binder/having_binder.hpp
+++ b/src/include/duckdb/planner/expression_binder/having_binder.hpp
@@ -25,6 +25,8 @@ class HavingBinder : public BaseSelectBinder {
 	BindResult BindWindow(WindowExpression &expr, idx_t depth) override;
 	BindResult BindColumnRef(unique_ptr<ParsedExpression> &expr_ptr, idx_t depth, bool root_expression) override;
 
+	unique_ptr<ParsedExpression> QualifyColumnName(ColumnRefExpression &col_ref, ErrorData &error) override;
+
 private:
 	ColumnAliasBinder column_alias_binder;
 	AggregateHandling aggregate_handling;
diff --git a/src/planner/bind_context.cpp b/src/planner/bind_context.cpp
index be68c9e61dab..bca3c448d94b 100644
--- a/src/planner/bind_context.cpp
+++ b/src/planner/bind_context.cpp
@@ -534,8 +534,8 @@ void BindContext::AddBaseTable(idx_t index, const string &alias, const vector<st
 void BindContext::AddBaseTable(idx_t index, const string &alias, const vector<string> &names,
                                const vector<LogicalType> &types, vector<column_t> &bound_column_ids,
                                const string &table_name) {
-	AddBinding(
-	    make_uniq<TableBinding>(alias.empty() ? table_name : alias, types, names, bound_column_ids, nullptr, index));
+	AddBinding(make_uniq<TableBinding>(alias.empty() ? table_name : alias, types, names, bound_column_ids, nullptr,
+	                                   index, true));
 }
 
 void BindContext::AddTableFunction(idx_t index, const string &alias, const vector<string> &names,
diff --git a/src/planner/binder/expression/bind_columnref_expression.cpp b/src/planner/binder/expression/bind_columnref_expression.cpp
index dcd34e70cd83..473bd2139c25 100644
--- a/src/planner/binder/expression/bind_columnref_expression.cpp
+++ b/src/planner/binder/expression/bind_columnref_expression.cpp
@@ -218,6 +218,11 @@ void ExpressionBinder::QualifyColumnNames(Binder &binder, unique_ptr<ParsedExpre
 	where_binder.QualifyColumnNames(expr, lambda_params);
 }
 
+void ExpressionBinder::QualifyColumnNames(ExpressionBinder &expression_binder, unique_ptr<ParsedExpression> &expr) {
+	vector<unordered_set<string>> lambda_params;
+	expression_binder.QualifyColumnNames(expr, lambda_params);
+}
+
 unique_ptr<ParsedExpression> ExpressionBinder::CreateStructExtract(unique_ptr<ParsedExpression> base,
                                                                    const string &field_name) {
 
diff --git a/src/planner/binder/query_node/bind_select_node.cpp b/src/planner/binder/query_node/bind_select_node.cpp
index f1bad4c335da..27901d363928 100644
--- a/src/planner/binder/query_node/bind_select_node.cpp
+++ b/src/planner/binder/query_node/bind_select_node.cpp
@@ -514,7 +514,7 @@ unique_ptr<BoundQueryNode> Binder::BindSelectNode(SelectNode &statement, unique_
 	// bind the HAVING clause, if any
 	if (statement.having) {
 		HavingBinder having_binder(*this, context, *result, info, statement.aggregate_handling);
-		ExpressionBinder::QualifyColumnNames(*this, statement.having);
+		ExpressionBinder::QualifyColumnNames(having_binder, statement.having);
 		result->having = having_binder.Bind(statement.having);
 	}
 
diff --git a/src/planner/expression_binder/having_binder.cpp b/src/planner/expression_binder/having_binder.cpp
index 27b0124bcb93..6f0c96a06517 100644
--- a/src/planner/expression_binder/having_binder.cpp
+++ b/src/planner/expression_binder/having_binder.cpp
@@ -22,6 +22,22 @@ BindResult HavingBinder::BindLambdaReference(LambdaRefExpression &expr, idx_t de
 	return (*lambda_bindings)[expr.lambda_idx].Bind(lambda_ref, depth);
 }
 
+unique_ptr<ParsedExpression> HavingBinder::QualifyColumnName(ColumnRefExpression &colref, ErrorData &error) {
+	auto qualified_colref = ExpressionBinder::QualifyColumnName(colref, error);
+	if (!qualified_colref) {
+		return nullptr;
+	}
+
+	auto group_index = TryBindGroup(*qualified_colref);
+	if (group_index != DConstants::INVALID_INDEX) {
+		return qualified_colref;
+	}
+	if (column_alias_binder.QualifyColumnAlias(colref)) {
+		return nullptr;
+	}
+	return qualified_colref;
+}
+
 BindResult HavingBinder::BindColumnRef(unique_ptr<ParsedExpression> &expr_ptr, idx_t depth, bool root_expression) {
 
 	// Keep the original column name to return a meaningful error message.


Test patch:
diff --git a/test/sql/aggregate/having/having_alias.test b/test/sql/aggregate/having/having_alias.test
new file mode 100644
index 000000000000..0b5f3fbedd37
--- /dev/null
+++ b/test/sql/aggregate/having/having_alias.test
@@ -0,0 +1,39 @@
+# name: test/sql/aggregate/having/having_alias.test
+# description: Test aliases in the HAVING clause
+# group: [having]
+
+statement ok
+PRAGMA enable_verification
+
+query II
+SELECT b, sum(a) AS a
+FROM (VALUES (1, 0), (1, 1)) t(a, b)
+GROUP BY b
+HAVING a > 0
+ORDER BY ALL
+----
+0	1
+1	1
+
+# if a reference is both a group and an alias, we prefer to bind to the group
+statement ok
+create table t1(a int);
+
+statement ok
+insert into t1 values (42), (84);
+
+query I
+select a+1 as a from t1 group by a having a=42;
+----
+43
+
+statement ok
+create table t2(a int);
+
+statement ok
+insert into t2 values (42), (84), (42);
+
+query II
+select a as b, sum(a) as a from t2 group by b having a=42;
+----
+42	84
diff --git a/test/sql/binder/test_having_alias.test b/test/sql/binder/test_having_alias.test
index bfb0f2e06aa5..573ae895c1b8 100644
--- a/test/sql/binder/test_having_alias.test
+++ b/test/sql/binder/test_having_alias.test
@@ -46,15 +46,20 @@ SELECT COUNT(i) AS j FROM integers HAVING j=5;
 ----
 5
 
-# These don't work since i in HAVING refers the column instead of the alias
-# (SQLite says: 'Error: a GROUP BY clause is required before HAVING')
-statement error
+
+query I
 SELECT COUNT(i) AS i FROM integers HAVING i=5;
 ----
+5
 
-statement error
+query I
+SELECT COUNT(i) AS i FROM integers GROUP BY i HAVING i=5;
+----
+
+query I
 SELECT COUNT(i) AS i FROM integers HAVING i=5 ORDER BY i;
 ----
+5
 
 # use the same alias multiple times
 query I