duckdb · Mytherin · Feb 5, 2024 · Feb 4, 2024 · Feb 4, 2024
diff --git a/src/function/aggregate/sorted_aggregate_function.cpp b/src/function/aggregate/sorted_aggregate_function.cpp
@@ -714,29 +714,12 @@ void FunctionBinder::BindSortedAggregate(ClientContext &context, BoundAggregateE
 		// not a sorted aggregate: return
 		return;
 	}
+	// Remove unnecessary ORDER BY clauses and return if nothing remains
 	if (context.config.enable_optimizer) {
-		// for each ORDER BY - check if it is actually necessary
-		// expressions that are in the groups do not need to be ORDERED BY
-		// `ORDER BY` on a group has no effect, because for each aggregate, the group is unique
-		// similarly, we only need to ORDER BY each aggregate once
-		expression_set_t seen_expressions;
-		for (auto &target : groups) {
-			seen_expressions.insert(*target);
-		}
-		vector<BoundOrderByNode> new_order_nodes;
-		for (auto &order_node : expr.order_bys->orders) {
-			if (seen_expressions.find(*order_node.expression) != seen_expressions.end()) {
-				// we do not need to order by this node
-				continue;
-			}
-			seen_expressions.insert(*order_node.expression);
-			new_order_nodes.push_back(std::move(order_node));
-		}
-		if (new_order_nodes.empty()) {
+		if (expr.order_bys->Simplify(groups)) {
 			expr.order_bys.reset();
 			return;
 		}
-		expr.order_bys->orders = std::move(new_order_nodes);
 	}
 	auto &bound_function = expr.function;
 	auto &children = expr.children;

diff --git a/src/include/duckdb/planner/bound_result_modifier.hpp b/src/include/duckdb/planner/bound_result_modifier.hpp
@@ -98,6 +98,10 @@ class BoundOrderModifier : public BoundResultModifier {
 
 	void Serialize(Serializer &serializer) const;
 	static unique_ptr<BoundOrderModifier> Deserialize(Deserializer &deserializer);
+
+	//! Remove unneeded/duplicate order elements.
+	//! Returns true of orders is not empty.
+	bool Simplify(const vector<unique_ptr<Expression>> &groups);
 };
 
 enum class DistinctType : uint8_t { DISTINCT = 0, DISTINCT_ON = 1 };

diff --git a/src/optimizer/rule/ordered_aggregate_optimizer.cpp b/src/optimizer/rule/ordered_aggregate_optimizer.cpp
@@ -31,31 +31,12 @@ unique_ptr<Expression> OrderedAggregateOptimizer::Apply(LogicalOperator &op, vec
 		return nullptr;
 	}
 
-	auto &context = rewriter.context;
-	// for each ORDER BY - check if it is actually necessary
-	// expressions that are in the groups do not need to be ORDERED BY
-	// `ORDER BY` on a group has no effect, because for each aggregate, the group is unique
-	// similarly, we only need to ORDER BY each aggregate once
-	expression_set_t seen_expressions;
-	auto &groups = op.Cast<LogicalAggregate>().groups;
-	for (auto &target : groups) {
-		seen_expressions.insert(*target);
-	}
-	vector<BoundOrderByNode> new_order_nodes;
-	for (auto &order_node : aggr.order_bys->orders) {
-		if (seen_expressions.find(*order_node.expression) != seen_expressions.end()) {
-			// we do not need to order by this node
-			continue;
-		}
-		seen_expressions.insert(*order_node.expression);
-		new_order_nodes.push_back(std::move(order_node));
-	}
-	if (new_order_nodes.empty()) {
+	// Remove unnecessary ORDER BY clauses and return if nothing remains
+	if (aggr.order_bys->Simplify(op.Cast<LogicalAggregate>().groups)) {
 		aggr.order_bys.reset();
 		changes_made = true;
 		return nullptr;
 	}
-	aggr.order_bys->orders = std::move(new_order_nodes);
 
 	//	Rewrite first/last/arbitrary/any_value to use arg_xxx[_null] and create_sort_key
 	const auto &aggr_name = aggr.function.name;
@@ -70,6 +51,7 @@ unique_ptr<Expression> OrderedAggregateOptimizer::Apply(LogicalOperator &op, vec
 		return nullptr;
 	}
 
+	auto &context = rewriter.context;
 	FunctionBinder binder(context);
 	vector<unique_ptr<Expression>> sort_children;
 	for (auto &order : aggr.order_bys->orders) {

diff --git a/src/planner/bound_result_modifier.cpp b/src/planner/bound_result_modifier.cpp
@@ -1,4 +1,5 @@
 #include "duckdb/planner/bound_result_modifier.hpp"
+#include "duckdb/parser/expression_map.hpp"
 
 namespace duckdb {
 
@@ -92,6 +93,29 @@ bool BoundOrderModifier::Equals(const unique_ptr<BoundOrderModifier> &left,
 	return BoundOrderModifier::Equals(*left, *right);
 }
 
+bool BoundOrderModifier::Simplify(const vector<unique_ptr<Expression>> &groups) {
+	// for each ORDER BY - check if it is actually necessary
+	// expressions that are in the groups do not need to be ORDERED BY
+	// `ORDER BY` on a group has no effect, because for each aggregate, the group is unique
+	// similarly, we only need to ORDER BY each aggregate once
+	expression_set_t seen_expressions;
+	for (auto &target : groups) {
+		seen_expressions.insert(*target);
+	}
+	vector<BoundOrderByNode> new_order_nodes;
+	for (auto &order_node : orders) {
+		if (seen_expressions.find(*order_node.expression) != seen_expressions.end()) {
+			// we do not need to order by this node
+			continue;
+		}
+		seen_expressions.insert(*order_node.expression);
+		new_order_nodes.push_back(std::move(order_node));
+	}
+	orders.swap(new_order_nodes);
+
+	return orders.empty();
+}
+
 BoundLimitModifier::BoundLimitModifier() : BoundResultModifier(ResultModifierType::LIMIT_MODIFIER) {
 }