diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 667d306de..1cbfd79c8 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2567,28 +2567,8 @@ pub fn op_agg_step( AggFunc::Count | AggFunc::Count0 => { Register::Aggregate(AggContext::Count(Value::Integer(0))) } - AggFunc::Max => { - let col = state.registers[*col].get_owned_value(); - match col { - Value::Integer(_) => Register::Aggregate(AggContext::Max(None)), - Value::Float(_) => Register::Aggregate(AggContext::Max(None)), - Value::Text(_) => Register::Aggregate(AggContext::Max(None)), - _ => { - unreachable!(); - } - } - } - AggFunc::Min => { - let col = state.registers[*col].get_owned_value(); - match col { - Value::Integer(_) => Register::Aggregate(AggContext::Min(None)), - Value::Float(_) => Register::Aggregate(AggContext::Min(None)), - Value::Text(_) => Register::Aggregate(AggContext::Min(None)), - _ => { - unreachable!(); - } - } - } + AggFunc::Max => Register::Aggregate(AggContext::Max(None)), + AggFunc::Min => Register::Aggregate(AggContext::Min(None)), AggFunc::GroupConcat | AggFunc::StringAgg => { Register::Aggregate(AggContext::GroupConcat(Value::build_text(""))) } @@ -2682,28 +2662,9 @@ pub fn op_agg_step( unreachable!(); }; - match (acc.as_mut(), col.get_owned_value()) { - (None, value) => { - *acc = Some(value.clone()); - } - (Some(Value::Integer(ref mut current_max)), Value::Integer(value)) => { - if *value > *current_max { - *current_max = *value; - } - } - (Some(Value::Float(ref mut current_max)), Value::Float(value)) => { - if *value > *current_max { - *current_max = *value; - } - } - (Some(Value::Text(ref mut current_max)), Value::Text(value)) => { - if value.value > current_max.value { - *current_max = value.clone(); - } - } - _ => { - eprintln!("Unexpected types in max aggregation"); - } + let new_value = col.get_owned_value(); + if *new_value != Value::Null && acc.as_ref().map_or(true, |acc| new_value > acc) { + *acc = Some(new_value.clone()); } } AggFunc::Min => { @@ -2718,28 +2679,10 @@ pub fn op_agg_step( unreachable!(); }; - match (acc.as_mut(), col.get_owned_value()) { - (None, value) => { - *acc.borrow_mut() = Some(value.clone()); - } - (Some(Value::Integer(ref mut current_min)), Value::Integer(value)) => { - if *value < *current_min { - *current_min = *value; - } - } - (Some(Value::Float(ref mut current_min)), Value::Float(value)) => { - if *value < *current_min { - *current_min = *value; - } - } - (Some(Value::Text(ref mut current_min)), Value::Text(text)) => { - if text.value < current_min.value { - *current_min = text.clone(); - } - } - _ => { - eprintln!("Unexpected types in min aggregation"); - } + let new_value = col.get_owned_value(); + + if *new_value != Value::Null && acc.as_ref().map_or(true, |acc| new_value < acc) { + *acc = Some(new_value.clone()); } } AggFunc::GroupConcat | AggFunc::StringAgg => { diff --git a/testing/agg-functions.test b/testing/agg-functions.test index 50c3f44e2..efc01c2f7 100755 --- a/testing/agg-functions.test +++ b/testing/agg-functions.test @@ -55,6 +55,19 @@ do_execsql_test select-min { SELECT min(age) FROM users; } {1} +do_execsql_test_on_specific_db {:memory:} min-null-regression-test { + CREATE TABLE t(a); + INSERT INTO t VALUES ('abc'), (NULL); + SELECT min(a) FROM t; +} {abc} + +do_execsql_test_on_specific_db {:memory:} max-null-regression-test { + CREATE TABLE t(a); + INSERT INTO t VALUES ('abc'), (NULL); + SELECT max(a) FROM t; +} {abc} + + do_execsql_test select-max-text { SELECT max(first_name) FROM users; } {Zoe} diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index ecb478fcc..e8b06d953 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -1373,6 +1373,66 @@ mod tests { } } + #[test] + #[ignore] + /// Ignored because of https://github.com/tursodatabase/turso/issues/2040, https://github.com/tursodatabase/turso/issues/2041 + /// TODO: add fuzzing for other aggregate functions + pub fn min_max_agg_fuzz() { + let _ = env_logger::try_init(); + + let datatypes = ["INTEGER", "TEXT", "REAL", "BLOB"]; + let (mut rng, seed) = rng_from_time(); + log::info!("seed: {}", seed); + + for _ in 0..1000 { + // Create table with random datatype + let datatype = datatypes[rng.random_range(0..datatypes.len())]; + let create_table = format!("CREATE TABLE t(x {})", datatype); + + let db = TempDatabase::new_empty(false); + let limbo_conn = db.connect_limbo(); + let sqlite_conn = rusqlite::Connection::open_in_memory().unwrap(); + + limbo_exec_rows(&db, &limbo_conn, &create_table); + sqlite_exec_rows(&sqlite_conn, &create_table); + + // Insert 5 random values of random types + let mut values = Vec::new(); + for _ in 0..5 { + let value = match rng.random_range(0..4) { + 0 => rng.random_range(-1000..1000).to_string(), // Integer + 1 => format!( + "'{}'", + (0..10) + .map(|_| rng.random_range(b'a'..=b'z') as char) + .collect::() + ), // Text + 2 => format!("{:.2}", rng.random_range(-100..100) as f64 / 10.0), // Real + 3 => "NULL".to_string(), // NULL + _ => unreachable!(), + }; + values.push(format!("({})", value)); + } + + let insert = format!("INSERT INTO t VALUES {}", values.join(",")); + limbo_exec_rows(&db, &limbo_conn, &insert); + sqlite_exec_rows(&sqlite_conn, &insert); + + // Test min and max + for agg in ["min(x)", "max(x)"] { + let query = format!("SELECT {} FROM t", agg); + let limbo = limbo_exec_rows(&db, &limbo_conn, &query); + let sqlite = sqlite_exec_rows(&sqlite_conn, &query); + + assert_eq!( + limbo, sqlite, + "query: {}, limbo: {:?}, sqlite: {:?}, seed: {}, values: {:?}, schema: {}", + query, limbo, sqlite, seed, values, create_table + ); + } + } + } + #[test] pub fn table_logical_expression_fuzz_run() { let _ = env_logger::try_init();