aljaz/turso
1
0
Fork 0
mirror of https://github.com/aljazceru/turso.git synced 2026-01-08 18:54:21 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge 'refactor: min max function' from Jean Arhancet

Browse Source 
In the current implementation, there is an issue with the `min`/`max`
functions when the input contains both numbers and text. This merge
request fixes the problem.
I have also changed the `minmax` function by splitting it into two
separate functions to be able to add unit tests for both functions, just
like for the other functions

Closes #347
This commit is contained in:
Pekka Enberg
2024-09-26 06:40:30 -07:00
parent ed3608dc1f 7b5d0fcabd
commit 918578fa94
3 changed files with 62 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
core/types.rs
View File

@@ -100,9 +100,23 @@ impl PartialOrd<OwnedValue> for OwnedValue {
(OwnedValue::Float(float_left), OwnedValue::Float(float_right)) => {
float_left.partial_cmp(float_right)
}
// Numeric vs Text/Blob
(
OwnedValue::Integer(_) | OwnedValue::Float(_),
OwnedValue::Text(_) | OwnedValue::Blob(_),
) => Some(std::cmp::Ordering::Less),
(
OwnedValue::Text(_) | OwnedValue::Blob(_),
OwnedValue::Integer(_) | OwnedValue::Float(_),
) => Some(std::cmp::Ordering::Greater),
(OwnedValue::Text(text_left), OwnedValue::Text(text_right)) => {
text_left.partial_cmp(text_right)
}
// Text vs Blob
(OwnedValue::Text(_), OwnedValue::Blob(_)) => Some(std::cmp::Ordering::Less),
(OwnedValue::Blob(_), OwnedValue::Text(_)) => Some(std::cmp::Ordering::Greater),
(OwnedValue::Blob(blob_left), OwnedValue::Blob(blob_right)) => {
blob_left.partial_cmp(blob_right)
}

74
core/vdbe/mod.rs
View File

@@ -1583,24 +1583,14 @@ impl Program {
[*start_reg..*start_reg + arg_count]
.iter()
.collect();
let min_fn = |a, b| if a < b { a } else { b };
if let Some(value) = exec_minmax(reg_values, min_fn) {
state.registers[*dest] = value;
} else {
state.registers[*dest] = OwnedValue::Null;
}
state.registers[*dest] = exec_min(reg_values);
}
ScalarFunc::Max => {
let reg_values = state.registers
[*start_reg..*start_reg + arg_count]
.iter()
.collect();
let max_fn = |a, b| if a > b { a } else { b };
if let Some(value) = exec_minmax(reg_values, max_fn) {
state.registers[*dest] = value;
} else {
state.registers[*dest] = OwnedValue::Null;
}
state.registers[*dest] = exec_max(reg_values);
}
ScalarFunc::Nullif => {
let first_value = &state.registers[*start_reg];
@@ -2095,11 +2085,18 @@ fn exec_glob(regex_cache: Option<&mut HashMap<String, Regex>>, pattern: &str, te
}
}
fn exec_minmax<'a>(
regs: Vec<&'a OwnedValue>,
op: fn(&'a OwnedValue, &'a OwnedValue) -> &'a OwnedValue,
) -> Option<OwnedValue> {
regs.into_iter().reduce(|a, b| op(a, b)).cloned()
fn exec_min(regs: Vec<&OwnedValue>) -> OwnedValue {
regs.iter()
.min()
.map(|&v| v.to_owned())
.unwrap_or(OwnedValue::Null)
}
fn exec_max(regs: Vec<&OwnedValue>) -> OwnedValue {
regs.iter()
.max()
.map(|&v| v.to_owned())
.unwrap_or(OwnedValue::Null)
}
fn exec_nullif(first_value: &OwnedValue, second_value: &OwnedValue) -> OwnedValue {
@@ -2294,10 +2291,10 @@ mod tests {
use super::{
exec_abs, exec_char, exec_hex, exec_if, exec_length, exec_like, exec_lower, exec_ltrim,
exec_minmax, exec_nullif, exec_quote, exec_random, exec_round, exec_rtrim, exec_sign,
exec_substring, exec_trim, exec_typeof, exec_unicode, exec_upper, execute_sqlite_version,
get_new_rowid, AggContext, Cursor, CursorResult, LimboError, OwnedRecord, OwnedValue,
Result,
exec_max, exec_min, exec_nullif, exec_quote, exec_random, exec_round, exec_rtrim,
exec_sign, exec_substring, exec_trim, exec_typeof, exec_unicode, exec_upper,
execute_sqlite_version, get_new_rowid, AggContext, Cursor, CursorResult, LimboError,
OwnedRecord, OwnedValue, Result,
};
use mockall::{mock, predicate};
use rand::{rngs::mock::StepRng, thread_rng};
@@ -2540,39 +2537,32 @@ mod tests {
}
#[test]
fn test_minmax() {
let min_fn = |a, b| if a < b { a } else { b };
let max_fn = |a, b| if a > b { a } else { b };
fn test_min_max() {
let input_int_vec = vec![&OwnedValue::Integer(-1), &OwnedValue::Integer(10)];
assert_eq!(
exec_minmax(input_int_vec.clone(), min_fn),
Some(OwnedValue::Integer(-1))
);
assert_eq!(
exec_minmax(input_int_vec.clone(), max_fn),
Some(OwnedValue::Integer(10))
);
assert_eq!(exec_min(input_int_vec.clone()), OwnedValue::Integer(-1));
assert_eq!(exec_max(input_int_vec.clone()), OwnedValue::Integer(10));
let str1 = OwnedValue::Text(Rc::new(String::from("A")));
let str2 = OwnedValue::Text(Rc::new(String::from("z")));
let input_str_vec = vec![&str2, &str1];
assert_eq!(
exec_minmax(input_str_vec.clone(), min_fn),
Some(OwnedValue::Text(Rc::new(String::from("A"))))
exec_min(input_str_vec.clone()),
OwnedValue::Text(Rc::new(String::from("A")))
);
assert_eq!(
exec_minmax(input_str_vec.clone(), max_fn),
Some(OwnedValue::Text(Rc::new(String::from("z"))))
exec_max(input_str_vec.clone()),
OwnedValue::Text(Rc::new(String::from("z")))
);
let input_null_vec = vec![&OwnedValue::Null, &OwnedValue::Null];
assert_eq!(exec_min(input_null_vec.clone()), OwnedValue::Null);
assert_eq!(exec_max(input_null_vec.clone()), OwnedValue::Null);
let input_mixed_vec = vec![&OwnedValue::Integer(10), &str1];
assert_eq!(exec_min(input_mixed_vec.clone()), OwnedValue::Integer(10));
assert_eq!(
exec_minmax(input_null_vec.clone(), min_fn),
Some(OwnedValue::Null)
);
assert_eq!(
exec_minmax(input_null_vec.clone(), max_fn),
Some(OwnedValue::Null)
exec_max(input_mixed_vec.clone()),
OwnedValue::Text(Rc::new(String::from("A")))
);
}

16
testing/scalar-functions.test
View File

@@ -295,6 +295,22 @@ do_execsql_test min-str {
select min('b','a','z')
} {a}
do_execsql_test min-str-number {
select min('42',100)
} {100}
do_execsql_test min-blob-number {
select min(3.14,x'616263')
} {3.14}
do_execsql_test max-str-number {
select max('42',100)
} {42}
do_execsql_test max-blob-number {
select max(3.14,x'616263')
} {abc}
do_execsql_test min-null {
select min(null,null)
} {}