Issue duckdb#11419: Quantile Order By

The PERCENTILE_XXX rewrite was colliding with the sorted aggregate rewrite here. Also cleaned up the quantile negation logic to not have scary holes... fixes: duckdb#11419 fixes: duckdblabs/duckdb-internal#1712
ccfelius · Apr 6, 2024 · dd08d8c · dd08d8c
1 parent bb7b06a
commit dd08d8c
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 14 deletions.
diff --git a/src/planner/binder/expression/bind_aggregate_expression.cpp b/src/planner/binder/expression/bind_aggregate_expression.cpp
@@ -95,14 +95,17 @@ BindResult BaseSelectBinder::BindAggregate(FunctionExpression &aggr, AggregateFu
 
 	// Handle ordered-set aggregates by moving the single ORDER BY expression to the front of the children.
 	//	https://www.postgresql.org/docs/current/functions-aggregate.html#FUNCTIONS-ORDEREDSET-TABLE
-	bool ordered_set_agg = false;
+	// We also have to handle ORDER BY in the argument list, so note how many arguments we should have
+	// and only inject the ordering expression if there are too few.
+	idx_t ordered_set_agg = 0;
 	bool negate_fractions = false;
 	if (aggr.order_bys && aggr.order_bys->orders.size() == 1) {
 		const auto &func_name = aggr.function_name;
-		ordered_set_agg = (func_name == "quantile_cont" || func_name == "quantile_disc" ||
-		                   (func_name == "mode" && aggr.children.empty()));
+		if (func_name == "mode") {
+			ordered_set_agg = 1;
+		} else if (func_name == "quantile_cont" || func_name == "quantile_disc") {
+			ordered_set_agg = 2;
 
-		if (ordered_set_agg) {
 			auto &config = DBConfig::GetConfig(context);
 			const auto &order = aggr.order_bys->orders[0];
 			const auto sense =
@@ -111,10 +114,11 @@ BindResult BaseSelectBinder::BindAggregate(FunctionExpression &aggr, AggregateFu
 		}
 	}
 
-	for (auto &child : aggr.children) {
+	for (idx_t i = 0; i < aggr.children.size(); ++i) {
+		auto &child = aggr.children[i];
 		aggregate_binder.BindChild(child, 0, error);
 		// We have to negate the fractions for PERCENTILE_XXXX DESC
-		if (!error.HasError() && ordered_set_agg) {
+		if (!error.HasError() && ordered_set_agg && i == aggr.children.size() - 1) {
 			NegatePercentileFractions(context, child, negate_fractions);
 		}
 	}
@@ -181,14 +185,17 @@ BindResult BaseSelectBinder::BindAggregate(FunctionExpression &aggr, AggregateFu
 
 	if (ordered_set_agg) {
 		const bool order_sensitive = (aggr.function_name == "mode");
-		for (auto &order : aggr.order_bys->orders) {
-			auto &child = BoundExpression::GetExpression(*order.expression);
-			types.push_back(child->return_type);
-			arguments.push_back(child->return_type);
-			if (order_sensitive) {
-				children.push_back(child->Copy());
-			} else {
-				children.push_back(std::move(child));
+		// Inject missing ordering arguments
+		if (aggr.children.size() < ordered_set_agg) {
+			for (auto &order : aggr.order_bys->orders) {
+				auto &child = BoundExpression::GetExpression(*order.expression);
+				types.push_back(child->return_type);
+				arguments.push_back(child->return_type);
+				if (order_sensitive) {
+					children.push_back(child->Copy());
+				} else {
+					children.push_back(std::move(child));
+				}
 			}
 		}
 		if (!order_sensitive) {

diff --git a/test/sql/aggregate/aggregates/test_quantile_cont.test b/test/sql/aggregate/aggregates/test_quantile_cont.test
@@ -131,6 +131,16 @@ SELECT quantile_cont(r, -0.1) FROM quantile
 ----
 899910.0
 
+# ORDER BY ... DESC
+query II
+SELECT 
+    percentile_cont(0.8) WITHIN GROUP (ORDER BY x DESC),
+    quantile_cont(x, 0.8 ORDER BY x DESC),
+FROM 
+    (VALUES (2), (1)) _(x);
+----
+1.2	1.2
+
 # empty input
 query R
 SELECT quantile_cont(r, 0.1) FROM quantile WHERE 1=0

diff --git a/test/sql/aggregate/aggregates/test_quantile_disc.test b/test/sql/aggregate/aggregates/test_quantile_disc.test
@@ -83,6 +83,16 @@ FROM VALUES (11000), (3100), (2900), (2800), (2600), (2500) AS tab(col);
 ----
 2900
 
+# ORDER BY ... DESC
+query II
+SELECT 
+    percentile_disc(0.8) WITHIN GROUP (ORDER BY x DESC),
+    quantile_disc(x, 0.8 ORDER BY x DESC),
+FROM 
+    (VALUES (2), (1)) _(x);
+----
+1.2	1.2
+
 #
 # VARCHAR. Remember, this is dictionary ordering, not numeric ordering!
 #