Merge 'vdbe: fix some issues with min() and max() and add ignored fuzz test' from Jussi Saurio

Removes the error prints described in #2033 but doesn't fix the issue.
Instead as a result of this PR I opened
https://github.com/tursodatabase/turso/issues/2040 and
https://github.com/tursodatabase/turso/issues/2041 after I added a fuzz
test 😂
we _need_ to fuzz _everything_.

Closes #2039
This commit is contained in:
Pekka Enberg
2025-07-11 09:52:56 +03:00
3 changed files with 82 additions and 66 deletions

View File

@@ -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 => {

View File

@@ -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}

View File

@@ -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::<String>()
), // 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();