From ccdcf302ca8d4432c578d90f5104f09a91bf7523 Mon Sep 17 00:00:00 2001
From: jussisaurio <jussi.saurio@gmail.com>
Date: Sun, 17 Nov 2024 17:06:09 +0200
Subject: [PATCH] quick fix for #399

---
 core/translate/emitter.rs | 15 ++++++++++++++-
 core/translate/planner.rs |  4 +++-
 testing/groupby.test      |  4 ++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs
index 17c4b90fe..7104cebe6 100644
--- a/core/translate/emitter.rs
+++ b/core/translate/emitter.rs
@@ -811,7 +811,20 @@ impl Emitter for Operator {
                                 )?;
                             }
                             for (i, agg) in aggregates.iter().enumerate() {
-                                let expr = &agg.args[0]; // TODO hakhackhachkachkachk hack hack
+                                // TODO it's a hack to assume aggregate functions have exactly one argument.
+                                // Counterpoint e.g. GROUP_CONCAT(expr, separator).
+                                //
+                                // Here we are collecting scalars for the group by sorter, which will include
+                                // both the group by expressions and the aggregate arguments.
+                                // e.g. in `select u.first_name, sum(u.age) from users group by u.first_name`
+                                // the sorter will have two scalars: u.first_name and u.age.
+                                // these are then sorted by u.first_name, and for each u.first_name, we sum the u.age.
+                                // the actual aggregation is done later in GROUP_BY_SORT_AND_COMPARE below.
+                                //
+                                // This is why we take the first argument of each aggregate function currently.
+                                // It's mostly an artifact of the current architecture being a bit poor; we should recognize
+                                // which scalars are dependencies of aggregate functions and explicitly collect those.
+                                let expr = &agg.args[0];
                                 let agg_reg = start_reg + sort_keys_count + i;
                                 translate_expr(
                                     program,
diff --git a/core/translate/planner.rs b/core/translate/planner.rs
index d69e61589..3d4cd6794 100644
--- a/core/translate/planner.rs
+++ b/core/translate/planner.rs
@@ -158,7 +158,9 @@ pub fn prepare_select_plan<'a>(schema: &Schema, select: ast::Select) -> Result<P
                                     ) {
                                         aggregate_expressions.push(Aggregate {
                                             func: f,
-                                            args: vec![],
+                                            args: vec![ast::Expr::Literal(ast::Literal::Numeric(
+                                                "1".to_string(),
+                                            ))],
                                             original_expr: expr.clone(),
                                         });
                                     }
diff --git a/testing/groupby.test b/testing/groupby.test
index 385c058d7..b3d873110 100644
--- a/testing/groupby.test
+++ b/testing/groupby.test
@@ -127,3 +127,7 @@ do_execsql_test group_by_function_expression_ridiculous {
 (97|36
 (20|35
 (31|35}
+
+do_execsql_test group_by_count_star {
+  select u.first_name, count(*) from users u group by u.first_name limit 1;
+} {Aaron|41}
\ No newline at end of file