From 419ccc327e9a4d458e347b005db3636f96bb1870 Mon Sep 17 00:00:00 2001
From: jussisaurio <jussi.saurio@gmail.com>
Date: Mon, 16 Dec 2024 00:30:38 +0200
Subject: [PATCH] Dont assume GROUP BY has aggregates

---
 core/translate/emitter.rs | 18 ++++++++----------
 core/translate/plan.rs    |  2 +-
 core/translate/planner.rs | 12 +++---------
 testing/groupby.test      | 13 +++++++++++++
 4 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs
index eccbbb1d6..e5d2110dd 100644
--- a/core/translate/emitter.rs
+++ b/core/translate/emitter.rs
@@ -198,8 +198,7 @@ pub fn emit_program(
     }
 
     if let Some(ref mut group_by) = plan.group_by {
-        let aggregates = plan.aggregates.as_mut().unwrap();
-        init_group_by(&mut program, group_by, aggregates, &mut metadata)?;
+        init_group_by(&mut program, group_by, &plan.aggregates, &mut metadata)?;
     }
     init_source(&mut program, &plan.source, &mut metadata)?;
 
@@ -235,18 +234,18 @@ pub fn emit_program(
             &plan.result_columns,
             group_by,
             plan.order_by.as_ref(),
-            &plan.aggregates.as_ref().unwrap(),
+            &plan.aggregates,
             plan.limit.clone(),
             &plan.referenced_tables,
             &mut metadata,
         )?;
-    } else if let Some(ref mut aggregates) = plan.aggregates {
+    } else if !plan.aggregates.is_empty() {
         // Handle aggregation without GROUP BY
         agg_without_group_by_emit(
             &mut program,
             &plan.referenced_tables,
             &plan.result_columns,
-            aggregates,
+            &plan.aggregates,
             &mut metadata,
         )?;
         // Single row result for aggregates without GROUP BY, so ORDER BY not needed
@@ -825,14 +824,14 @@ fn inner_loop_emit(
             metadata,
             InnerLoopEmitTarget::GroupBySorter {
                 group_by,
-                aggregates: &plan.aggregates.as_ref().unwrap(),
+                aggregates: &plan.aggregates,
             },
             &plan.referenced_tables,
         );
     }
     // if we DONT have a group by, but we have aggregates, we emit without ResultRow.
     // we also do not need to sort because we are emitting a single row.
-    if plan.aggregates.is_some() {
+    if !plan.aggregates.is_empty() {
         return inner_loop_source_emit(
             program,
             &plan.result_columns,
@@ -870,7 +869,7 @@ fn inner_loop_emit(
 fn inner_loop_source_emit(
     program: &mut ProgramBuilder,
     result_columns: &Vec<ResultSetColumn>,
-    aggregates: &Option<Vec<Aggregate>>,
+    aggregates: &Vec<Aggregate>,
     metadata: &mut Metadata,
     emit_target: InnerLoopEmitTarget,
     referenced_tables: &[BTreeTableReference],
@@ -936,7 +935,6 @@ fn inner_loop_source_emit(
             Ok(())
         }
         InnerLoopEmitTarget::AggStep => {
-            let aggregates = aggregates.as_ref().unwrap();
             let agg_final_label = program.allocate_label();
             metadata.termination_label_stack.push(agg_final_label);
             let num_aggs = aggregates.len();
@@ -965,7 +963,7 @@ fn inner_loop_source_emit(
         }
         InnerLoopEmitTarget::ResultRow { limit } => {
             assert!(
-                aggregates.is_none(),
+                aggregates.is_empty(),
                 "We should not get here with aggregates"
             );
             emit_select_result(
diff --git a/core/translate/plan.rs b/core/translate/plan.rs
index 7a13d6b88..faa3a6fa0 100644
--- a/core/translate/plan.rs
+++ b/core/translate/plan.rs
@@ -39,7 +39,7 @@ pub struct Plan {
     /// order by clause
     pub order_by: Option<Vec<(ast::Expr, Direction)>>,
     /// all the aggregates collected from the result columns, order by, and (TODO) having clauses
-    pub aggregates: Option<Vec<Aggregate>>,
+    pub aggregates: Vec<Aggregate>,
     /// limit clause
     pub limit: Option<usize>,
     /// all the tables referenced in the query
diff --git a/core/translate/planner.rs b/core/translate/planner.rs
index af6add833..66195459f 100644
--- a/core/translate/planner.rs
+++ b/core/translate/planner.rs
@@ -274,7 +274,7 @@ pub fn prepare_select_plan<'a>(schema: &Schema, select: ast::Select) -> Result<P
                 where_clause: None,
                 group_by: None,
                 order_by: None,
-                aggregates: None,
+                aggregates: vec![],
                 limit: None,
                 referenced_tables,
                 available_indexes: schema.indexes.clone().into_values().flatten().collect(),
@@ -432,11 +432,7 @@ pub fn prepare_select_plan<'a>(schema: &Schema, select: ast::Select) -> Result<P
                 });
             }
 
-            plan.aggregates = if aggregate_expressions.is_empty() {
-                None
-            } else {
-                Some(aggregate_expressions)
-            };
+            plan.aggregates = aggregate_expressions;
 
             // Parse the ORDER BY clause
             if let Some(order_by) = select.order_by {
@@ -463,9 +459,7 @@ pub fn prepare_select_plan<'a>(schema: &Schema, select: ast::Select) -> Result<P
                     };
 
                     bind_column_references(&mut expr, &plan.referenced_tables)?;
-                    if let Some(aggs) = &mut plan.aggregates {
-                        resolve_aggregates(&expr, aggs);
-                    }
+                    resolve_aggregates(&expr, &mut plan.aggregates);
 
                     key.push((
                         expr,
diff --git a/testing/groupby.test b/testing/groupby.test
index 34cf802af..350405208 100644
--- a/testing/groupby.test
+++ b/testing/groupby.test
@@ -16,6 +16,19 @@ Alan|551
 Albert|369
 Alec|247}
 
+do_execsql_test group_by_without_aggs {
+  select u.first_name from users u group by u.first_name limit 10;
+} {Aaron
+Abigail
+Adam
+Adrian
+Adriana
+Adrienne
+Aimee
+Alan
+Albert
+Alec}
+
 do_execsql_test group_by_two_joined_columns {
   select u.first_name, p.name, sum(u.age) from users u join products p on u.id = p.id group by u.first_name, p.name limit 10;
 } {Aimee|jeans|24