fix(core/translate): handle NULL values correctly in SQLite min/max special-case

Author: nunogoncalves03

core/translate/group_by.rs

@@ -520,28 +520,48 @@ fn emit_min_max_comparison(
         dest: update_flag_reg,
     });
 
-    // Skip comparison if aggregate argument is NULL (NULLs are ignored in MIN/MAX)
     let after_comparison_label = program.allocate_label();
-    let check_first_row_label = program.allocate_label();
+    let handle_non_null_arg_label = program.allocate_label();
+    let set_flag_for_null_label = program.allocate_label();
 
+    // Check if aggregate argument is NULL
     program.emit_insn(Insn::NotNull {
         reg: agg_arg_reg,
-        target_pc: check_first_row_label,
+        target_pc: handle_non_null_arg_label,
+    });
+
+    // Aggregate argument is NULL: check if current_min_max is also NULL
+    // If both are NULL, we should update bare columns so last NULL row wins
+    program.emit_insn(Insn::IsNull {
+        reg: current_min_max_reg,
+        target_pc: set_flag_for_null_label,
+    });
+    // If current_min_max is NOT NULL, we've seen a non-NULL value before
+    // Don't update bare columns for this NULL value
+    program.emit_insn(Insn::Goto {
+        target_pc: after_comparison_label,
+    });
+
+    // Both agg_arg and current_min_max are NULL: update bare columns
+    program.preassign_label_to_next_insn(set_flag_for_null_label);
+    program.emit_insn(Insn::Integer {
+        value: 1,
+        dest: update_flag_reg,
     });
     program.emit_insn(Insn::Goto {
         target_pc: after_comparison_label,
     });
 
     // Aggregate argument is not NULL: proceed with comparison
-    program.preassign_label_to_next_insn(check_first_row_label);
+    program.preassign_label_to_next_insn(handle_non_null_arg_label);
 
-    // Check if this is the first row in the group (current_min_max_reg is NULL)
+    // Check if this is the first non-NULL value in the group (current_min_max_reg is NULL)
     let compare_label = program.allocate_label();
     program.emit_insn(Insn::NotNull {
         reg: current_min_max_reg,
         target_pc: compare_label,
     });
-    // First row: always update bare columns
+    // First non-NULL value: always update bare columns
     program.emit_insn(Insn::Integer {
         value: 1,
         dest: update_flag_reg,
@@ -550,7 +570,7 @@ fn emit_min_max_comparison(
         target_pc: after_comparison_label,
     });
 
-    // Not first row: compare aggregate argument with current min/max
+    // Not first non-NULL value: compare aggregate argument with current min/max
     program.preassign_label_to_next_insn(compare_label);
 
     // Get collation for comparison

testing/groupby.test

@@ -410,6 +410,24 @@ do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_with_nulls_min {
     SELECT a, b, min(c) FROM t GROUP BY a;
 } {1|a|a}
 
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_all_nulls_max {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL), (1, NULL, NULL);
+  SELECT a, b, max(c) FROM t GROUP BY a;
+} {1||}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_all_nulls_min {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL), (1, NULL, NULL);
+  SELECT a, b, min(c) FROM t GROUP BY a;
+} {1||}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_all_nulls_min_2 {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, NULL, NULL), (1, 'b', NULL);
+  SELECT a, b, min(c) FROM t GROUP BY a;
+} {1|b|}
+
 do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_ties_max {
     CREATE TABLE t(a, b, c);
     INSERT INTO t VALUES (1, 'a', 'x'), (1, 'b', 'x'), (1, 'c', 'y');
@@ -433,3 +451,47 @@ do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_multiple_bare_col
     INSERT INTO t VALUES (1, 'a', 'x', 10), (1, 'b', 'y', 20), (1, 'c', 'z', 30);
     SELECT a, b, c, min(d) FROM t GROUP BY a;
 } {1|a|x|10}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_nulls_then_value_max {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL), (1, 'c', 5);
+  SELECT a, b, max(c) FROM t GROUP BY a;
+} {1|c|5}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_nulls_then_value_min {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL), (1, 'c', 5);
+  SELECT a, b, min(c) FROM t GROUP BY a;
+} {1|c|5}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_value_then_nulls_max {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', 5), (1, 'b', NULL), (1, 'c', NULL);
+  SELECT a, b, max(c) FROM t GROUP BY a;
+} {1|a|5}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_value_then_nulls_min {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', 5), (1, 'b', NULL), (1, 'c', NULL);
+  SELECT a, b, min(c) FROM t GROUP BY a;
+} {1|a|5}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_multiple_groups_with_nulls_max {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL);
+  INSERT INTO t VALUES (2, 'x', 5), (2, 'y', NULL);
+  INSERT INTO t VALUES (3, 'p', NULL), (3, 'q', 10);
+  SELECT a, b, max(c) FROM t GROUP BY a ORDER BY a;
+} {1|b|
+2|x|5
+3|q|10}
+
+do_execsql_test_on_specific_db {:memory:} min_max_bare_columns_multiple_groups_with_nulls_min {
+  CREATE TABLE t(a, b, c);
+  INSERT INTO t VALUES (1, 'a', NULL), (1, 'b', NULL);
+  INSERT INTO t VALUES (2, 'x', 5), (2, 'y', NULL);
+  INSERT INTO t VALUES (3, 'p', NULL), (3, 'q', 10);
+  SELECT a, b, min(c) FROM t GROUP BY a ORDER BY a;
+} {1|b|
+2|x|5
+3|q|10}