Aggregation without group by produces incorrect results for scalars #1198

ihorandrianov · 2025-03-27T19:57:44Z

Closes #954
Before:

After:

SQLite:

jussisaurio · 2025-03-29T09:23:21Z

Hi, thanks! this fix needs to be a bit more general in my opinion - things like this still give incorrect results:

# notice id is not in the group by clause, but sqlite still returns the first id per group, whereas limbo returns NULL
select id, first_name, sum(age) from users group by first_name limit 1;
42|Aaron|2271

ihorandrianov · 2025-03-31T08:11:38Z

Ready for second review try. Also fixed order by elimination when group by is present like:

select id, first_name, sum(age) from users group by first_name order by id limit 5;

jussisaurio · 2025-04-01T06:49:05Z

Hi @ihorandrianov can you please add at least TCL tests (*.test) that demonstrate the fix works?

jussisaurio · 2025-04-01T06:49:55Z

core/translate/emitter.rs

@@ -63,6 +63,8 @@ pub struct TranslateCtx<'a> {
    pub label_main_loop_end: Option<BranchOffset>,
    // First register of the aggregation results
    pub reg_agg_start: Option<usize>,
+    // Register to track if we set non aggregate cols to first encountered row in non group by agg statement


If I had no context at all and didn't know what this PR was about, I would have no idea what this means.

How about:

// In non-group-by statements with aggregations (e.g. SELECT foo, bar, sum(baz) FROM t), we want to emit the non-aggregate columns (foo and bar) only once. This register is a flag that tracks whether we have already done that.

jussisaurio · 2025-04-01T06:59:56Z

core/translate/group_by.rs


    let sort_cursor = program.alloc_cursor_id(None, CursorType::Sorter);

    let reg_abort_flag = program.alloc_register();
    let reg_group_exprs_cmp = program.alloc_registers(group_by.exprs.len());
-    let reg_group_exprs_acc = program.alloc_registers(group_by.exprs.len());
+    let reg_group_exprs_acc = program.alloc_registers(non_aggregate_count);


this should be renamed reg_non_aggregate_exprs_acc

jussisaurio · 2025-04-01T07:00:59Z

core/translate/group_by.rs

-            .iter()
-            .map(|agg| agg.args.len())
-            .sum::<usize>();
+    let sorter_column_count = non_aggregate_count + agg_args_count;


The comment above should now say "all non-aggregate columns and all arguments of agg functions are in the sorter"

jussisaurio · 2025-04-01T07:01:55Z

core/translate/group_by.rs

@@ -296,7 +311,7 @@ pub fn emit_group_by<'a>(
    });

    // Read the group by columns for a finished group
-    for i in 0..group_by.exprs.len() {
+    for i in 0..non_aggregate_count {


// Read the non-aggregate columns for a finished group

jussisaurio · 2025-04-01T07:03:16Z

core/translate/group_by.rs

+        .result_columns
+        .iter()
+        .filter(|rc| !rc.contains_aggregates)
+        .collect::<Vec<_>>();


This should be called non_aggregate_result_columns and I don't think we need to .collect()

jussisaurio · 2025-04-01T07:32:33Z

core/translate/main_loop.rs

-                }
-                let reg = start_reg + num_aggs + i;
+
+            let if_label = if let Some(flag) = t_ctx.reg_agg_flag {


let label_emit_nonagg_only_once?

jussisaurio · 2025-04-01T07:34:23Z

core/translate/optimizer.rs

+    while i < o.len() {
+        let (key, order) = &o[i];
+
+        if matches!(order, Direction::Descending) {


please add comment

jussisaurio · 2025-04-01T07:34:29Z

core/translate/optimizer.rs

+            if pos != insert_pos {
+                let mut current_pos = pos;
+                while current_pos > insert_pos {
+                    g.exprs.swap(current_pos, current_pos - 1);


please add comment

jussisaurio · 2025-04-01T07:34:38Z

core/translate/optimizer.rs

@@ -108,6 +156,11 @@ fn eliminate_unnecessary_orderby(plan: &mut SelectPlan, schema: &Schema) -> Resu
        return Ok(());
    }

+    // if pk will be removed later


what does 'if pk' mean

jussisaurio · 2025-04-01T07:36:09Z

core/translate/result_row.rs

@@ -25,7 +25,9 @@ pub fn emit_select_result(
    }

    let start_reg = t_ctx.reg_result_cols_start.unwrap();
-    for (i, rc) in plan.result_columns.iter().enumerate() {
+    for (i, rc) in plan.result_columns.iter().enumerate().filter(|(_, rc)| {
+        t_ctx.reg_agg_flag.is_some() && rc.contains_aggregates || t_ctx.reg_agg_flag.is_none()


what is happening here and why

pereman2

looks good for me, I cannot give better review than @jussisaurio

pereman2 · 2025-04-04T09:49:59Z

core/translate/emitter.rs

+    if !plan.aggregates.is_empty()
+        && plan.group_by.is_none()
+        && plan.result_columns.iter().any(|c| !c.contains_aggregates)
+    {
+        let flag = program.alloc_register();
+        program.emit_int(0, flag);
+        t_ctx.reg_nonagg_emit_once_flag = Some(flag);
+    }


Can we have comment here about the behaviour? I know it's in the reg_nonagg_emit_once_flag definition but it could be good to know here too.

jussisaurio

Good stuff, I left some very minor nits

core/translate/main_loop.rs

core/translate/group_by.rs

ihorandrianov requested a review from jussisaurio as a code owner March 27, 2025 19:57

ihorandrianov temporarily deployed to test March 27, 2025 19:58 — with GitHub Actions Inactive

ihorandrianov temporarily deployed to test March 27, 2025 20:06 — with GitHub Actions Inactive

ihorandrianov temporarily deployed to test March 27, 2025 22:55 — with GitHub Actions Inactive

ihorandrianov temporarily deployed to test March 27, 2025 23:06 — with GitHub Actions Inactive

ihorandrianov force-pushed the agg_fix branch from 1d197e2 to 361ecf7 Compare March 30, 2025 15:41

ihorandrianov marked this pull request as draft March 30, 2025 16:55

ihorandrianov marked this pull request as ready for review March 31, 2025 08:10

jussisaurio reviewed Apr 1, 2025

View reviewed changes

ihorandrianov force-pushed the agg_fix branch from 8a6dc62 to 541af6e Compare April 3, 2025 19:07

ihorandrianov requested a review from penberg as a code owner April 3, 2025 19:07

ihorandrianov added 10 commits April 3, 2025 22:28

fix aggregation functions without group by

b47c214

cargo fmt

352fa6f

create if only if non aggregate columns present

36fe859

small refine

4fd1dcd

non group by cols are displayed in group by agg statements

2bcdd4e

some smartie optimizations

816cbac

improve naming and add comments for context

91ceab1

fix output when group by is not part of resulting set

34a132f

add tests

5632b15

fix tests

d4b8fa1

ihorandrianov force-pushed the agg_fix branch from 541af6e to d4b8fa1 Compare April 3, 2025 19:28

pereman2 approved these changes Apr 4, 2025

View reviewed changes

jussisaurio approved these changes Apr 5, 2025

View reviewed changes

core/translate/main_loop.rs Outdated Show resolved Hide resolved

core/translate/group_by.rs Outdated Show resolved Hide resolved

core/translate/group_by.rs Outdated Show resolved Hide resolved

reduce vec allocations, add comments for magic ifs

0c9464e

jussisaurio merged commit c19e4fc into tursodatabase:main Apr 6, 2025
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregation without group by produces incorrect results for scalars #1198

Aggregation without group by produces incorrect results for scalars #1198

ihorandrianov commented Mar 27, 2025

jussisaurio commented Mar 29, 2025

ihorandrianov commented Mar 31, 2025

jussisaurio commented Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

jussisaurio Apr 1, 2025

pereman2 left a comment

pereman2 Apr 4, 2025

jussisaurio left a comment

Aggregation without group by produces incorrect results for scalars #1198

Aggregation without group by produces incorrect results for scalars #1198

Conversation

ihorandrianov commented Mar 27, 2025

jussisaurio commented Mar 29, 2025

ihorandrianov commented Mar 31, 2025

jussisaurio commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jussisaurio left a comment

Choose a reason for hiding this comment