Repository: duckdb/duckdb (language: python)

Issue:
string_agg() with "order by" can't join by "_"
### What happens?

`string_agg(DISTINCT kind order by kind, '_') `
for this code, it could not by joined "_",
for example, if kind is ['A', 'B', 'C'], result would not be A_B_C, but A,B,C.

`string_agg(DISTINCT kind, '_') `
if delete "order by kind", it works well, and result is A_B_C

### To Reproduce

```py
import duckdb
import pandas as pd

data = {
    "kind": ["A", "B", "A", "C", "B", "D", "A"],
    "year": [2020, 2020, 2021, 2021, 2021, 2020, 2022],
    "score": [10, 15, 20, 10, 25, 30, 18]
}

df = pd.DataFrame(data)

con = duckdb.connect()
con.execute("CREATE TABLE my_table AS SELECT * FROM df")

result = con.execute("""
    SELECT
        year,
        string_agg(DISTINCT kind order by kind, '_') AS kinds,
        MAX(score) AS max_score
    FROM my_table
    GROUP BY year
""").fetchdf()

print(result)

con.close()
```

### OS:

x64

### DuckDB Version:

1.0.0

### DuckDB Client:

Python

### Full Name:

Cong Ma

### Affiliation:

individual

### What is the latest build you tested with? If possible, we recommend testing with the latest nightly build.

I have tested with a stable release

### Did you include all relevant data sets for reproducing the issue?

Not applicable - the reproduction does not require a data set

### Did you include all code required to reproduce the issue?

- [X] Yes, I have

### Did you include all relevant configuration (e.g., CPU architecture, Python version, Linux distribution) to reproduce the issue?

- [X] Yes, I have


Discussion / hints:
@fengchutadie thanks! I could reproduce it with SQL-only code:

```sql
CREATE TABLE my_table AS
    SELECT kind, year, score
    FROM (VALUES
        ('A', 2020, 10),
        ('B', 2020, 15),
        ('A', 2021, 20),
        ('C', 2021, 10),
        ('B', 2021, 25),
        ('D', 2020, 30),
        ('A', 2022, 18),
    ) t(kind, year, score);

SELECT
    year,
    string_agg(DISTINCT kind, '_') AS kinds,
    MAX(score) AS max_score
FROM my_table
GROUP BY year;

SELECT
    year,
    string_agg(DISTINCT kind order by kind, '_') AS kinds,
    MAX(score) AS max_score
FROM my_table
GROUP BY year;
```

Thanks for the report!

This is not a bug - `ORDER BY` needs to come after **all** of the arguments to a function. In your example you are executing `string_agg(distinct kind)` with an `ORDER` modifier that orders on two expressions: `kind, '_'`. `string_agg` with a single parameter uses `,` as the delimiter.

The correct syntax for what you intent here is `string_agg(DISTINCT kind, '_' ORDER BY kind)`.

That being said - perhaps we can throw an exception here when ordering by a string literal as I agree this is confusing.

Fix patch:
diff --git a/src/include/duckdb/main/config.hpp b/src/include/duckdb/main/config.hpp
index d762587f9c41..802fb0d105b3 100644
--- a/src/include/duckdb/main/config.hpp
+++ b/src/include/duckdb/main/config.hpp
@@ -265,6 +265,8 @@ struct DBConfigOptions {
 	bool debug_skip_checkpoint_on_commit = false;
 	//! Use IEE754-compliant floating point operations (returning NAN instead of errors/NULL)
 	bool ieee_floating_point_ops = true;
+	//! Allow ordering by non-integer literals - ordering by such literals has no effect
+	bool order_by_non_integer_literal = false;
 
 	bool operator==(const DBConfigOptions &other) const;
 };
diff --git a/src/include/duckdb/main/settings.hpp b/src/include/duckdb/main/settings.hpp
index 1feafd655661..7f66d7d25b5d 100644
--- a/src/include/duckdb/main/settings.hpp
+++ b/src/include/duckdb/main/settings.hpp
@@ -604,6 +604,16 @@ struct OldImplicitCasting {
 	static Value GetSetting(const ClientContext &context);
 };
 
+struct OrderByNonIntegerLiteral {
+	static constexpr const char *Name = "order_by_non_integer_literal";
+	static constexpr const char *Description =
+	    "Allow ordering by non-integer literals - ordering by such literals has no effect";
+	static constexpr const LogicalTypeId InputType = LogicalTypeId::BOOLEAN;
+	static void SetGlobal(DatabaseInstance *db, DBConfig &config, const Value &parameter);
+	static void ResetGlobal(DatabaseInstance *db, DBConfig &config);
+	static Value GetSetting(const ClientContext &context);
+};
+
 struct PartitionedWriteFlushThreshold {
 	static constexpr const char *Name = "partitioned_write_flush_threshold";
 	static constexpr const char *Description =
diff --git a/src/include/duckdb/planner/expression_binder/order_binder.hpp b/src/include/duckdb/planner/expression_binder/order_binder.hpp
index 4fa297891827..df8365cc7001 100644
--- a/src/include/duckdb/planner/expression_binder/order_binder.hpp
+++ b/src/include/duckdb/planner/expression_binder/order_binder.hpp
@@ -22,8 +22,8 @@ struct SelectBindState;
 //! The ORDER binder is responsible for binding an expression within the ORDER BY clause of a SQL statement
 class OrderBinder {
 public:
-	OrderBinder(vector<Binder *> binders, SelectBindState &bind_state);
-	OrderBinder(vector<Binder *> binders, SelectNode &node, SelectBindState &bind_state);
+	OrderBinder(vector<reference<Binder>> binders, SelectBindState &bind_state);
+	OrderBinder(vector<reference<Binder>> binders, SelectNode &node, SelectBindState &bind_state);
 
 public:
 	unique_ptr<Expression> Bind(unique_ptr<ParsedExpression> expr);
@@ -31,7 +31,7 @@ class OrderBinder {
 	bool HasExtraList() const {
 		return extra_list;
 	}
-	const vector<Binder *> &GetBinders() const {
+	const vector<reference<Binder>> &GetBinders() const {
 		return binders;
 	}
 
@@ -43,7 +43,7 @@ class OrderBinder {
 	optional_idx TryGetProjectionReference(ParsedExpression &expr) const;
 
 private:
-	vector<Binder *> binders;
+	vector<reference<Binder>> binders;
 	optional_ptr<vector<unique_ptr<ParsedExpression>>> extra_list;
 	SelectBindState &bind_state;
 };
diff --git a/src/main/config.cpp b/src/main/config.cpp
index 9a1988f834e7..3ca639166f3a 100644
--- a/src/main/config.cpp
+++ b/src/main/config.cpp
@@ -113,6 +113,7 @@ static const ConfigurationOption internal_options[] = {
     DUCKDB_GLOBAL(OldImplicitCasting),
     DUCKDB_GLOBAL_ALIAS("memory_limit", MaximumMemorySetting),
     DUCKDB_GLOBAL_ALIAS("null_order", DefaultNullOrderSetting),
+    DUCKDB_GLOBAL(OrderByNonIntegerLiteral),
     DUCKDB_LOCAL(OrderedAggregateThreshold),
     DUCKDB_GLOBAL(PasswordSetting),
     DUCKDB_LOCAL(PerfectHashThresholdSetting),
diff --git a/src/main/settings/settings.cpp b/src/main/settings/settings.cpp
index a1bb26e06e82..cd906d2032d4 100644
--- a/src/main/settings/settings.cpp
+++ b/src/main/settings/settings.cpp
@@ -1345,6 +1345,22 @@ Value OldImplicitCasting::GetSetting(const ClientContext &context) {
 	return Value::BOOLEAN(config.options.old_implicit_casting);
 }
 
+//===--------------------------------------------------------------------===//
+// Old Implicit Casting
+//===--------------------------------------------------------------------===//
+void OrderByNonIntegerLiteral::SetGlobal(DatabaseInstance *db, DBConfig &config, const Value &input) {
+	config.options.order_by_non_integer_literal = input.GetValue<bool>();
+}
+
+void OrderByNonIntegerLiteral::ResetGlobal(DatabaseInstance *db, DBConfig &config) {
+	config.options.order_by_non_integer_literal = DBConfig().options.order_by_non_integer_literal;
+}
+
+Value OrderByNonIntegerLiteral::GetSetting(const ClientContext &context) {
+	auto &config = DBConfig::GetConfig(context);
+	return Value::BOOLEAN(config.options.order_by_non_integer_literal);
+}
+
 //===--------------------------------------------------------------------===//
 // Partitioned Write Flush Threshold
 //===--------------------------------------------------------------------===//
diff --git a/src/planner/binder/expression/bind_aggregate_expression.cpp b/src/planner/binder/expression/bind_aggregate_expression.cpp
index 13f24ce0e4be..d61d18d255ec 100644
--- a/src/planner/binder/expression/bind_aggregate_expression.cpp
+++ b/src/planner/binder/expression/bind_aggregate_expression.cpp
@@ -1,21 +1,22 @@
 #include "duckdb/catalog/catalog_entry/aggregate_function_catalog_entry.hpp"
-#include "duckdb/common/pair.hpp"
 #include "duckdb/common/operator/cast_operators.hpp"
+#include "duckdb/common/pair.hpp"
+#include "duckdb/execution/expression_executor.hpp"
+#include "duckdb/function/function_binder.hpp"
+#include "duckdb/function/scalar/generic_functions.hpp"
+#include "duckdb/main/config.hpp"
+#include "duckdb/parser/expression/constant_expression.hpp"
 #include "duckdb/parser/expression/function_expression.hpp"
+#include "duckdb/planner/binder.hpp"
 #include "duckdb/planner/expression/bound_aggregate_expression.hpp"
 #include "duckdb/planner/expression/bound_cast_expression.hpp"
 #include "duckdb/planner/expression/bound_columnref_expression.hpp"
 #include "duckdb/planner/expression/bound_constant_expression.hpp"
 #include "duckdb/planner/expression/bound_function_expression.hpp"
-#include "duckdb/planner/expression_iterator.hpp"
 #include "duckdb/planner/expression_binder/aggregate_binder.hpp"
 #include "duckdb/planner/expression_binder/base_select_binder.hpp"
+#include "duckdb/planner/expression_iterator.hpp"
 #include "duckdb/planner/query_node/bound_select_node.hpp"
-#include "duckdb/execution/expression_executor.hpp"
-#include "duckdb/function/scalar/generic_functions.hpp"
-#include "duckdb/main/config.hpp"
-#include "duckdb/function/function_binder.hpp"
-#include "duckdb/planner/binder.hpp"
 
 namespace duckdb {
 
@@ -140,6 +141,19 @@ BindResult BaseSelectBinder::BindAggregate(FunctionExpression &aggr, AggregateFu
 	// Bind the ORDER BYs, if any
 	if (aggr.order_bys && !aggr.order_bys->orders.empty()) {
 		for (auto &order : aggr.order_bys->orders) {
+			if (order.expression->type == ExpressionType::VALUE_CONSTANT) {
+				auto &const_expr = order.expression->Cast<ConstantExpression>();
+				if (!const_expr.value.type().IsIntegral()) {
+					auto &config = DBConfig::GetConfig(context);
+					if (!config.options.order_by_non_integer_literal) {
+						throw BinderException(
+						    *order.expression,
+						    "ORDER BY non-integer literal has no effect.\n* SET order_by_non_integer_literal=true to "
+						    "allow this behavior.\n\nPerhaps you misplaced ORDER BY; ORDER BY must appear "
+						    "after all regular arguments of the aggregate.");
+					}
+				}
+			}
 			aggregate_binder.BindChild(order.expression, 0, error);
 		}
 	}
diff --git a/src/planner/binder/query_node/bind_select_node.cpp b/src/planner/binder/query_node/bind_select_node.cpp
index 85ea733339ff..f303b4646af0 100644
--- a/src/planner/binder/query_node/bind_select_node.cpp
+++ b/src/planner/binder/query_node/bind_select_node.cpp
@@ -201,7 +201,7 @@ void Binder::PrepareModifiers(OrderBinder &order_binder, QueryNode &statement, B
 #endif
 			for (auto &order_node : order.orders) {
 				vector<unique_ptr<ParsedExpression>> order_list;
-				order_binders[0]->ExpandStarExpression(std::move(order_node.expression), order_list);
+				order_binders[0].get().ExpandStarExpression(std::move(order_node.expression), order_list);
 
 				auto type = config.ResolveOrder(order_node.type);
 				auto null_order = config.ResolveNullOrder(type, order_node.null_order);
@@ -453,7 +453,7 @@ unique_ptr<BoundQueryNode> Binder::BindSelectNode(SelectNode &statement, unique_
 	}
 
 	// now bind all the result modifiers; including DISTINCT and ORDER BY targets
-	OrderBinder order_binder({this}, statement, bind_state);
+	OrderBinder order_binder({*this}, statement, bind_state);
 	PrepareModifiers(order_binder, statement, *result);
 
 	vector<unique_ptr<ParsedExpression>> unbound_groups;
diff --git a/src/planner/binder/query_node/bind_setop_node.cpp b/src/planner/binder/query_node/bind_setop_node.cpp
index 316e9c9edf27..66ffede4d1e1 100644
--- a/src/planner/binder/query_node/bind_setop_node.cpp
+++ b/src/planner/binder/query_node/bind_setop_node.cpp
@@ -247,7 +247,7 @@ unique_ptr<BoundQueryNode> Binder::BindNode(SetOperationNode &statement) {
 			GatherAliases(*result, bind_state, reorder_idx);
 		}
 		// now we perform the actual resolution of the ORDER BY/DISTINCT expressions
-		OrderBinder order_binder({result->left_binder.get(), result->right_binder.get()}, bind_state);
+		OrderBinder order_binder({*result->left_binder, *result->right_binder}, bind_state);
 		PrepareModifiers(order_binder, statement, *result);
 	}
 
diff --git a/src/planner/expression_binder/order_binder.cpp b/src/planner/expression_binder/order_binder.cpp
index bbc39fef805d..c3e4fb6bf8e8 100644
--- a/src/planner/expression_binder/order_binder.cpp
+++ b/src/planner/expression_binder/order_binder.cpp
@@ -12,14 +12,15 @@
 #include "duckdb/planner/expression/bound_parameter_expression.hpp"
 #include "duckdb/planner/expression_binder.hpp"
 #include "duckdb/planner/expression_binder/select_bind_state.hpp"
+#include "duckdb/main/config.hpp"
 #include "duckdb/common/pair.hpp"
 
 namespace duckdb {
 
-OrderBinder::OrderBinder(vector<Binder *> binders, SelectBindState &bind_state)
+OrderBinder::OrderBinder(vector<reference<Binder>> binders, SelectBindState &bind_state)
     : binders(std::move(binders)), extra_list(nullptr), bind_state(bind_state) {
 }
-OrderBinder::OrderBinder(vector<Binder *> binders, SelectNode &node, SelectBindState &bind_state)
+OrderBinder::OrderBinder(vector<reference<Binder>> binders, SelectNode &node, SelectBindState &bind_state)
     : binders(std::move(binders)), bind_state(bind_state) {
 	this->extra_list = &node.select_list;
 }
@@ -55,9 +56,14 @@ optional_idx OrderBinder::TryGetProjectionReference(ParsedExpression &expr) cons
 		auto &constant = expr.Cast<ConstantExpression>();
 		// ORDER BY a constant
 		if (!constant.value.type().IsIntegral()) {
-			// non-integral expression, we just leave the constant here.
+			// non-integral expression
 			// ORDER BY <constant> has no effect
-			// CONTROVERSIAL: maybe we should throw an error
+			// this is disabled by default (matching Postgres) - but we can control this with a setting
+			auto &config = DBConfig::GetConfig(binders[0].get().context);
+			if (!config.options.order_by_non_integer_literal) {
+				throw BinderException(expr, "ORDER BY non-integer literal has no effect.\n* SET "
+				                            "order_by_non_integer_literal=true to allow this behavior.");
+			}
 			break;
 		}
 		// INTEGER constant: we use the integer as an index into the select list (e.g. ORDER BY 1)
@@ -143,7 +149,7 @@ unique_ptr<Expression> OrderBinder::Bind(unique_ptr<ParsedExpression> expr) {
 	// general case
 	// first bind the table names of this entry
 	for (auto &binder : binders) {
-		ExpressionBinder::QualifyColumnNames(*binder, expr);
+		ExpressionBinder::QualifyColumnNames(binder.get(), expr);
 	}
 	// first check if the ORDER BY clause already points to an entry in the projection list
 	auto entry = bind_state.projection_map.find(*expr);


Test patch:
diff --git a/test/issues/rigger/assertion_scale.test b/test/issues/rigger/assertion_scale.test
index 7ad3899a6783..58f601d48837 100644
--- a/test/issues/rigger/assertion_scale.test
+++ b/test/issues/rigger/assertion_scale.test
@@ -14,6 +14,14 @@ CREATE TABLE t0(c0 DOUBLE, c1 DOUBLE);;
 statement ok
 INSERT INTO t0 VALUES(0.0,-1570504255.0);
 
+statement error
+CREATE VIEW v0(c0, c1) AS SELECT 888938716, '' FROM t0 GROUP BY t0.rowid, t0.rowid, t0.c1 ORDER BY 0.9507538105182436 OFFSET 686929302;
+----
+ORDER BY non-integer literal has no effect
+
+statement ok
+SET order_by_non_integer_literal=true
+
 statement ok
 CREATE VIEW v0(c0, c1) AS SELECT 888938716, '' FROM t0 GROUP BY t0.rowid, t0.rowid, t0.c1 ORDER BY 0.9507538105182436 OFFSET 686929302;
 
diff --git a/test/issues/rigger/test_544.test b/test/issues/rigger/test_544.test
index 812452132d5d..9fdb5da3d941 100644
--- a/test/issues/rigger/test_544.test
+++ b/test/issues/rigger/test_544.test
@@ -15,8 +15,15 @@ INSERT INTO t0(c0) VALUES (0);
 statement ok
 CREATE VIEW v0(c0) AS SELECT 1 FROM t0;
 
+statement error
+SELECT * FROM v0 ORDER BY 'a';
+----
+ORDER BY non-integer literal has no effect
+
+statement ok
+SET order_by_non_integer_literal=true
+
 query I
 SELECT * FROM v0 ORDER BY 'a';
 ----
 1
-
diff --git a/test/sql/aggregate/distinct/grouped/string_agg.test b/test/sql/aggregate/distinct/grouped/string_agg.test
index 19e4e1ee1104..aabfd6c14f92 100644
--- a/test/sql/aggregate/distinct/grouped/string_agg.test
+++ b/test/sql/aggregate/distinct/grouped/string_agg.test
@@ -53,3 +53,25 @@ ORDER BY 1
 2	3	/,-,+	3
 3	1	/	1
 4	3	NULL	3
+
+statement error
+SELECT g, STRING_AGG(DISTINCT y ORDER BY y, '_' ) FILTER (WHERE g < 4)
+FROM strings
+GROUP BY g
+ORDER BY 1
+----
+ORDER BY non-integer literal has no effect
+
+statement ok
+SET order_by_non_integer_literal=true
+
+query II
+SELECT g, STRING_AGG(DISTINCT y ORDER BY y, '_' ) FILTER (WHERE g < 4)
+FROM strings
+GROUP BY g
+ORDER BY 1
+----
+1	-,/
+2	+,-,/
+3	/
+4	NULL
diff --git a/test/sql/copy/parquet/batched_write/batch_memory_usage.test b/test/sql/copy/parquet/batched_write/batch_memory_usage.test
index 76ffbc2392c7..393bf7ba7f9a 100644
--- a/test/sql/copy/parquet/batched_write/batch_memory_usage.test
+++ b/test/sql/copy/parquet/batched_write/batch_memory_usage.test
@@ -11,7 +11,7 @@ statement ok
 COPY (SELECT uuid()::VARCHAR as varchar, uuid() AS uuid FROM range(10000000) t(i)) TO '__TEST_DIR__/random_uuids.parquet'
 
 statement ok
-SET memory_limit='500MB'
+SET memory_limit='750MB'
 
 statement ok
 COPY '__TEST_DIR__/random_uuids.parquet' TO '__TEST_DIR__/random_uuids_copy.parquet';
diff --git a/test/sql/order/test_order_by.test b/test/sql/order/test_order_by.test
index ab3cd1694bee..c64b0eba009c 100644
--- a/test/sql/order/test_order_by.test
+++ b/test/sql/order/test_order_by.test
@@ -177,3 +177,19 @@ SELECT a-10 AS k FROM test UNION SELECT a-11 AS l FROM test ORDER BY a-11;
 1
 2
 3
+
+# order by non-integer literal
+statement error
+SELECT a FROM test ORDER BY 'hello world', a
+----
+ORDER BY non-integer literal has no effect
+
+statement ok
+SET order_by_non_integer_literal=true
+
+query I
+SELECT a FROM test ORDER BY 'hello world', a
+----
+11
+12
+13
diff --git a/test/sql/order/test_order_by_exceptions.test b/test/sql/order/test_order_by_exceptions.test
index 45b5efa6ad71..a85272f2d045 100644
--- a/test/sql/order/test_order_by_exceptions.test
+++ b/test/sql/order/test_order_by_exceptions.test
@@ -17,8 +17,16 @@ SELECT a FROM test ORDER BY 2
 ----
 <REGEX>:Binder Error:.*term out of range.*
 
-# ORDER BY constant works, but does nothing
+# ORDER BY constant returns an error
 # CONTROVERSIAL: works in SQLite but not in Postgres
+statement error
+SELECT a FROM test ORDER BY 'hello', a
+----
+ORDER BY non-integer literal has no effect
+
+statement ok
+SET order_by_non_integer_literal=true
+
 query I
 SELECT a FROM test ORDER BY 'hello', a
 ----
diff --git a/test/sql/subquery/scalar/array_order_subquery.test b/test/sql/subquery/scalar/array_order_subquery.test
index 759a93589e48..6e1f36f7e086 100644
--- a/test/sql/subquery/scalar/array_order_subquery.test
+++ b/test/sql/subquery/scalar/array_order_subquery.test
@@ -92,9 +92,11 @@ SELECT ARRAY
 ----
 ORDER term out of range
 
-statement ok
+statement error
 SELECT ARRAY
   (SELECT 1 UNION ALL
    SELECT 2 UNION ALL
    SELECT 3
    ORDER by 'hello world') AS new_array;
+----
+ORDER BY non-integer literal has no effect