From d5f58f5feaa54437cad2fd023c8ca23e3bdae690 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Thu, 6 Feb 2025 18:36:21 +0200 Subject: [PATCH 1/5] Add quickcheck tests for generate_series() and refine implementation --- Cargo.lock | 14 + core/lib.rs | 5 +- core/translate/main_loop.rs | 1 + core/vdbe/builder.rs | 5 +- core/vdbe/explain.rs | 5 +- core/vdbe/insn.rs | 1 + core/vdbe/mod.rs | 9 +- extensions/series/Cargo.toml | 4 + extensions/series/src/lib.rs | 660 ++++++++++++++++++++++++++++++++++- 9 files changed, 684 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c0eb8db1..d26da84a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -738,6 +738,16 @@ dependencies = [ "log", ] +[[package]] +name = "env_logger" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a19187fea3ac7e84da7dacf48de0c45d63c6a76f9490dae389aead16c243fce3" +dependencies = [ + "log", + "regex", +] + [[package]] name = "env_logger" version = "0.10.2" @@ -1709,6 +1719,8 @@ dependencies = [ "limbo_ext", "log", "mimalloc", + "quickcheck", + "quickcheck_macros", ] [[package]] @@ -2387,6 +2399,8 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "588f6378e4dd99458b60ec275b4477add41ce4fa9f64dcba6f15adccb19b50d6" dependencies = [ + "env_logger 0.8.4", + "log", "rand 0.8.5", ] diff --git a/core/lib.rs b/core/lib.rs index 8fde24402..94853aa80 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -530,7 +530,7 @@ impl VirtualTable { cursor: &VTabOpaqueCursor, arg_count: usize, args: Vec, - ) -> Result<()> { + ) -> Result { let mut filter_args = Vec::with_capacity(arg_count); for i in 0..arg_count { let ownedvalue_arg = args.get(i).unwrap(); @@ -551,7 +551,8 @@ impl VirtualTable { (self.implementation.filter)(cursor.as_ptr(), arg_count as i32, filter_args.as_ptr()) }; match rc { - ResultCode::OK => Ok(()), + ResultCode::OK => Ok(true), + ResultCode::EOF => Ok(false), _ => Err(LimboError::ExtensionError(rc.to_string())), } } diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 35cc505c9..3b918e9ea 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -300,6 +300,7 @@ pub fn open_loop( } program.emit_insn(Insn::VFilter { cursor_id, + pc_if_empty: loop_end, arg_count: args.len(), args_reg: start_reg, }); diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index c3ead0d38..67f0f2749 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -410,7 +410,10 @@ impl ProgramBuilder { Insn::VNext { pc_if_next, .. } => { resolve(pc_if_next, "VNext"); } - _ => continue, + Insn::VFilter { pc_if_empty, .. } => { + resolve(pc_if_empty, "VFilter"); + } + _ => {} } } self.label_to_resolved_offset.clear(); diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index a0bb63023..6f55d5dc1 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -383,13 +383,14 @@ pub fn insn_to_str( ), Insn::VFilter { cursor_id, + pc_if_empty, arg_count, - args_reg, + .. } => ( "VFilter", *cursor_id as i32, + pc_if_empty.to_debug_int(), *arg_count as i32, - *args_reg as i32, OwnedValue::build_text(Rc::new("".to_string())), 0, "".to_string(), diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 223f321aa..2d673b018 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -224,6 +224,7 @@ pub enum Insn { /// Initialize the position of the virtual table cursor. VFilter { cursor_id: CursorID, + pc_if_empty: BranchOffset, arg_count: usize, args_reg: usize, }, diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 5003b72c6..6403d2565 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -879,6 +879,7 @@ impl Program { } Insn::VFilter { cursor_id, + pc_if_empty, arg_count, args_reg, } => { @@ -892,8 +893,12 @@ impl Program { for i in 0..*arg_count { args.push(state.registers[args_reg + i].clone()); } - virtual_table.filter(cursor, *arg_count, args)?; - state.pc += 1; + let has_rows = virtual_table.filter(cursor, *arg_count, args)?; + if !has_rows { + state.pc = pc_if_empty.to_offset_int(); + } else { + state.pc += 1; + } } Insn::VColumn { cursor_id, diff --git a/extensions/series/Cargo.toml b/extensions/series/Cargo.toml index cca322294..823e95472 100644 --- a/extensions/series/Cargo.toml +++ b/extensions/series/Cargo.toml @@ -19,3 +19,7 @@ log = "0.4.20" [target.'cfg(not(target_family = "wasm"))'.dependencies] mimalloc = { version = "*", default-features = false } + +[dev-dependencies] +quickcheck = "1.0.3" +quickcheck_macros = "1.0.0" diff --git a/extensions/series/src/lib.rs b/extensions/series/src/lib.rs index 83dd334ea..4105a00f0 100644 --- a/extensions/series/src/lib.rs +++ b/extensions/series/src/lib.rs @@ -51,16 +51,30 @@ impl VTabModule for GenerateSeriesVTab { let start = try_option!(args[0].to_integer(), ResultCode::InvalidArgs); let stop = try_option!( args.get(1).map(|v| v.to_integer().unwrap_or(i64::MAX)), - ResultCode::InvalidArgs - ); - let step = try_option!( - args.get(2).map(|v| v.to_integer().unwrap_or(1)), - ResultCode::InvalidArgs + ResultCode::EOF // Sqlite returns an empty series for wacky args ); + let mut step = args + .get(2) + .map(|v| v.to_integer().unwrap_or(1)) + .unwrap_or(1); + + // Convert zero step to 1, matching SQLite behavior + if step == 0 { + step = 1; + } + cursor.start = start; - cursor.current = start; cursor.step = step; cursor.stop = stop; + + // Set initial value based on range validity + // For invalid input SQLite returns an empty series + cursor.current = if cursor.is_invalid_range() { + return ResultCode::EOF; + } else { + start + }; + ResultCode::OK } @@ -69,11 +83,33 @@ impl VTabModule for GenerateSeriesVTab { } fn next(cursor: &mut Self::VCursor) -> ResultCode { - GenerateSeriesCursor::next(cursor) + // Check for invalid ranges (empty series) first + if cursor.is_invalid_range() { + return ResultCode::EOF; + } + + // Check if we've reached the end of the sequence + if cursor.would_exceed() { + return ResultCode::EOF; + } + + // Handle overflow by truncating to MAX/MIN + cursor.current = match cursor.step { + step if step > 0 => match cursor.current.checked_add(step) { + Some(val) => val, + None => i64::MAX, + }, + step => match cursor.current.checked_add(step) { + Some(val) => val, + None => i64::MIN, + }, + }; + + ResultCode::OK } fn eof(cursor: &Self::VCursor) -> bool { - cursor.eof() + cursor.is_invalid_range() || cursor.would_exceed() } } @@ -86,20 +122,56 @@ struct GenerateSeriesCursor { current: i64, } +impl GenerateSeriesCursor { + /// Returns true if this is an ascending series (positive step) but start > stop + fn is_invalid_ascending_series(&self) -> bool { + self.step > 0 && self.start > self.stop + } + + /// Returns true if this is a descending series (negative step) but start < stop + fn is_invalid_descending_series(&self) -> bool { + self.step < 0 && self.start < self.stop + } + + /// Returns true if this is an invalid range that should produce an empty series + fn is_invalid_range(&self) -> bool { + self.is_invalid_ascending_series() || self.is_invalid_descending_series() + } + + /// Returns true if we would exceed the stop value in the current direction + fn would_exceed(&self) -> bool { + (self.step > 0 && self.current + self.step > self.stop) + || (self.step < 0 && self.current + self.step < self.stop) + } +} + impl VTabCursor for GenerateSeriesCursor { type Error = ResultCode; fn next(&mut self) -> ResultCode { - let next_val = self.current.saturating_add(self.step); - if (self.step > 0 && next_val > self.stop) || (self.step < 0 && next_val < self.stop) { + // Check for invalid ranges (empty series) first + if self.eof() { return ResultCode::EOF; } - self.current = next_val; + + // Handle overflow by truncating to MAX/MIN + self.current = self.current.saturating_add(self.step); + ResultCode::OK } fn eof(&self) -> bool { - (self.step > 0 && self.current > self.stop) || (self.step < 0 && self.current < self.stop) + // Check for invalid ranges (empty series) first + if self.is_invalid_range() { + return true; + } + + // Check if we would exceed the stop value in the current direction + if self.would_exceed() { + return true; + } + + false } fn column(&self, idx: u32) -> Value { @@ -113,6 +185,568 @@ impl VTabCursor for GenerateSeriesCursor { } fn rowid(&self) -> i64 { - ((self.current - self.start) / self.step) + 1 + let sub = self.current.saturating_sub(self.start); + + // Handle overflow in rowid calculation by capping at MAX/MIN + match sub.checked_div(self.step) { + Some(val) => val.saturating_add(1), + None => { + if self.step > 0 { + i64::MAX + } else { + i64::MIN + } + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use quickcheck_macros::quickcheck; + + // Helper function to collect all values from a cursor, returns Result with error code + fn collect_series(start: i64, stop: i64, step: i64) -> Result, ResultCode> { + let mut cursor = GenerateSeriesCursor { + start: 0, + stop: 0, + step: 0, + current: 0, + }; + + // Create args array for filter + let args = vec![ + Value::from_integer(start), + Value::from_integer(stop), + Value::from_integer(step), + ]; + + // Validate inputs through filter + match GenerateSeriesVTab::filter(&mut cursor, 3, &args) { + ResultCode::OK => (), + ResultCode::EOF => return Ok(vec![]), + err => return Err(err), + } + + let mut values = Vec::new(); + loop { + values.push(cursor.column(0).to_integer().unwrap()); + match GenerateSeriesVTab::next(&mut cursor) { + ResultCode::OK => (), + ResultCode::EOF => break, + err => return Err(err), + } + } + Ok(values) + } + + #[quickcheck] + /// Test that the series length is correct for a positive step + /// Example: + /// start = 1, stop = 10, step = 1 + /// expected length = 10 + fn prop_series_length_positive_step(start: i64, stop: i64) { + // Limit the range to make test run faster, since we're testing with step 1. + let start = start % 1000; + let stop = stop % 1000; + let step = 1; + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start > stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}, got {:?}", + start, + stop, + step, + values + ); + } else { + let expected_len = ((stop - start) / step + 1) as usize; + assert_eq!( + values.len(), + expected_len, + "Series length mismatch for start={}, stop={}, step={}: expected {}, got {}", + start, + stop, + step, + expected_len, + values.len() + ); + } + } + + #[quickcheck] + /// Test that the series length is correct for a negative step + /// Example: + /// start = 10, stop = 1, step = -1 + /// expected length = 10 + fn prop_series_length_negative_step(start: i64, stop: i64) { + // Limit the range to make test run faster, since we're testing with step -1. + let start = start % 1000; + let stop = stop % 1000; + let step = -1; + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start < stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else { + let expected_len = ((start - stop) / step.abs() + 1) as usize; + assert_eq!( + values.len(), + expected_len, + "Series length mismatch for start={}, stop={}, step={}: expected {}, got {}", + start, + stop, + step, + expected_len, + values.len() + ); + } + } + + #[quickcheck] + /// Test that the series is monotonically increasing + /// Example: + /// start = 1, stop = 10, step = 1 + /// expected series = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] + fn prop_series_monotonic_increasing(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 10000; + let stop = stop % 10000; + let step = (step % 100).max(1); // Ensure positive non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start > stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else { + assert!( + values.windows(2).all(|w| w[0] < w[1]), + "Series not monotonically increasing: {:?} (start={}, stop={}, step={})", + values, + start, + stop, + step + ); + } + } + + #[quickcheck] + /// Test that the series is monotonically decreasing + /// Example: + /// start = 10, stop = 1, step = -1 + /// expected series = [10, 9, 8, 7, 6, 5, 4, 3, 2, 1] + fn prop_series_monotonic_decreasing(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 10000; + let stop = stop % 10000; + let step = -((step % 100).max(1)); // Ensure negative non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start < stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else { + assert!( + values.windows(2).all(|w| w[0] > w[1]), + "Series not monotonically decreasing: {:?} (start={}, stop={}, step={})", + values, + start, + stop, + step + ); + } + } + + #[quickcheck] + /// Test that the series step size is consistent + /// Example: + /// start = 1, stop = 10, step = 1 + /// expected step size = 1 + fn prop_series_step_size_positive(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 1000; + let stop = stop % 1000; + let step = (step % 100).max(1); // Ensure positive non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start > stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if !values.is_empty() { + assert!( + values.windows(2).all(|w| (w[1] - w[0]).abs() == step.abs()), + "Step size not consistent: {:?} (expected step size: {})", + values, + step.abs() + ); + } + } + + #[quickcheck] + /// Test that the series step size is consistent for a negative step + /// Example: + /// start = 10, stop = 1, step = -1 + /// expected step size = 1 + fn prop_series_step_size_negative(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 1000; + let stop = stop % 1000; + let step = -((step % 100).max(1)); // Ensure negative non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start < stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if !values.is_empty() { + assert!( + values.windows(2).all(|w| (w[1] - w[0]).abs() == step.abs()), + "Step size not consistent: {:?} (expected step size: {})", + values, + step.abs() + ); + } + } + + #[quickcheck] + /// Test that the series bounds are correct for a positive step + /// Example: + /// start = 1, stop = 10, step = 1 + /// expected bounds = [1, 10] + fn prop_series_bounds_positive(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 10000; + let stop = stop % 10000; + let step = (step % 100).max(1); // Ensure positive non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start > stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if !values.is_empty() { + assert_eq!( + values.first(), + Some(&start), + "Series doesn't start with start value: {:?} (expected start: {})", + values, + start + ); + assert!( + values.last().map_or(true, |&last| last <= stop), + "Series exceeds stop value: {:?} (stop: {})", + values, + stop + ); + } + } + + #[quickcheck] + /// Test that the series bounds are correct for a negative step + /// Example: + /// start = 10, stop = 1, step = -1 + /// expected bounds = [10, 1] + fn prop_series_bounds_negative(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster. + let start = start % 10000; + let stop = stop % 10000; + let step = -((step % 100).max(1)); // Ensure negative non-zero step + + let values = collect_series(start, stop, step).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ) + }); + + if start < stop { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if !values.is_empty() { + assert_eq!( + values.first(), + Some(&start), + "Series doesn't start with start value: {:?} (expected start: {})", + values, + start + ); + assert!( + values.last().map_or(true, |&last| last >= stop), + "Series exceeds stop value: {:?} (stop: {})", + values, + stop + ); + } + } + + #[test] + + fn test_series_empty_positive_step() { + let values = collect_series(10, 5, 1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Series should be empty when start > stop with positive step" + ); + } + + #[test] + fn test_series_empty_negative_step() { + let values = collect_series(5, 10, -1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Series should be empty when start < stop with negative step" + ); + } + + #[test] + fn test_series_single_element() { + let values = collect_series(5, 5, 1).expect("Failed to generate single element series"); + assert_eq!( + values, + vec![5], + "Single element series should contain only the start value" + ); + } + + #[test] + fn test_zero_step_is_interpreted_as_1() { + let values = collect_series(1, 10, 0).expect("Failed to generate series"); + assert_eq!( + values, + vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10], + "Zero step should be interpreted as 1" + ); + } + + #[test] + fn test_invalid_inputs() { + // Test that invalid ranges return empty series instead of errors + let values = collect_series(10, 1, 1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Invalid positive range should return empty series, got {:?}", + values + ); + + let values = collect_series(1, 10, -1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Invalid negative range should return empty series" + ); + + // Test that extreme ranges return empty series + let values = collect_series(i64::MAX, i64::MIN, 1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Extreme range (MAX to MIN) should return empty series" + ); + + let values = collect_series(i64::MIN, i64::MAX, -1).expect("Failed to generate series"); + assert!( + values.is_empty(), + "Extreme range (MIN to MAX) should return empty series" + ); + } + + #[quickcheck] + /// Test that rowid is always monotonically increasing regardless of step direction + fn prop_series_rowid_monotonic(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster + let start = start % 1000; + let stop = stop % 1000; + let step = if step == 0 { 1 } else { step % 100 }; // Ensure non-zero step + + let mut cursor = GenerateSeriesCursor { + start: 0, + stop: 0, + step: 0, + current: 0, + }; + + let args = vec![ + Value::from_integer(start), + Value::from_integer(stop), + Value::from_integer(step), + ]; + + // Initialize cursor through filter + GenerateSeriesVTab::filter(&mut cursor, 3, &args); + + let mut rowids = vec![]; + while !GenerateSeriesVTab::eof(&cursor) { + let cur_rowid = cursor.rowid(); + match GenerateSeriesVTab::next(&mut cursor) { + ResultCode::OK => rowids.push(cur_rowid), + ResultCode::EOF => break, + err => panic!( + "Unexpected error {:?} for start={}, stop={}, step={}", + err, start, stop, step + ), + } + } + + assert!( + rowids.windows(2).all(|w| w[1] == w[0] + 1), + "Rowids not monotonically increasing: {:?} (start={}, stop={}, step={})", + rowids, + start, + stop, + step + ); + } + + // #[quickcheck] + // /// Test that integer overflow is properly handled + // fn prop_series_overflow(start: i64, step: i64) { + // // Use values close to MAX/MIN to test overflow + // let start = if start >= 0 { + // i64::MAX - (start % 100) + // } else { + // i64::MIN + (start.abs() % 100) + // }; + // let step = if step == 0 { 1 } else { step }; // Ensure non-zero step + // let stop = if step > 0 { i64::MAX } else { i64::MIN }; + + // let result = collect_series(start, stop, step); + + // // If we would overflow, expect InvalidArgs + // if start.checked_add(step).is_none() { + // assert!( + // matches!(result, Err(ResultCode::InvalidArgs)), + // "Expected InvalidArgs for overflow case: start={}, stop={}, step={}", + // start, stop, step + // ); + // } else { + // // Otherwise the series should be valid + // let values = result.unwrap_or_else(|e| { + // panic!( + // "Failed to generate series for start={}, stop={}, step={}: {:?}", + // start, stop, step, e + // ) + // }); + + // // Verify no values overflow + // for window in values.windows(2) { + // assert!( + // window[0].checked_add(step).is_some(), + // "Overflow occurred in series: {:?} + {} (start={}, stop={}, step={})", + // window[0], step, start, stop, step + // ); + // } + // } + // } + + #[quickcheck] + /// Test that empty series are handled consistently + fn prop_series_empty(start: i64, stop: i64, step: i64) { + // Limit the range to make test run faster + let start = start % 1000; + let stop = stop % 1000; + let step = if step == 0 { 1 } else { step % 100 }; // Ensure non-zero step + + let result = collect_series(start, stop, step); + + match result { + Ok(values) => { + if (step > 0 && start > stop) || (step < 0 && start < stop) { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if start == stop { + assert_eq!( + values, + vec![start], + "Series with start==stop should contain exactly one element" + ); + } + } + Err(e) => panic!( + "Unexpected error for start={}, stop={}, step={}: {:?}", + start, stop, step, e + ), + } } } From bf045da9dc0bedf083295a73b4b6717d2b4ca8f4 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Thu, 6 Feb 2025 19:11:04 +0200 Subject: [PATCH 2/5] Fix python extension test for generate_series() --- testing/extensions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/extensions.py b/testing/extensions.py index cda953f86..7c76ac46e 100755 --- a/testing/extensions.py +++ b/testing/extensions.py @@ -434,7 +434,7 @@ def test_series(pipe): run_test( pipe, "SELECT * FROM generate_series(1, 10);", - lambda res: "Invalid Argument" in res, + lambda res: res == "1\n2\n3\n4\n5\n6\n7\n8\n9\n10", ) run_test( pipe, From 93c3689070b2d631d170f97ddcdd38de53d87905 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Thu, 6 Feb 2025 23:41:31 +0200 Subject: [PATCH 3/5] make tests better and fix more edge cases --- extensions/series/src/lib.rs | 526 ++++++++++++++--------------------- 1 file changed, 211 insertions(+), 315 deletions(-) diff --git a/extensions/series/src/lib.rs b/extensions/series/src/lib.rs index 4105a00f0..ffa0871c6 100644 --- a/extensions/series/src/lib.rs +++ b/extensions/series/src/lib.rs @@ -84,25 +84,16 @@ impl VTabModule for GenerateSeriesVTab { fn next(cursor: &mut Self::VCursor) -> ResultCode { // Check for invalid ranges (empty series) first - if cursor.is_invalid_range() { + if cursor.eof() { return ResultCode::EOF; } - // Check if we've reached the end of the sequence - if cursor.would_exceed() { - return ResultCode::EOF; - } - - // Handle overflow by truncating to MAX/MIN - cursor.current = match cursor.step { - step if step > 0 => match cursor.current.checked_add(step) { - Some(val) => val, - None => i64::MAX, - }, - step => match cursor.current.checked_add(step) { - Some(val) => val, - None => i64::MIN, - }, + // Handle overflow + cursor.current = match cursor.current.checked_add(cursor.step) { + Some(val) => val, + None => { + return ResultCode::EOF; + } }; ResultCode::OK @@ -140,8 +131,8 @@ impl GenerateSeriesCursor { /// Returns true if we would exceed the stop value in the current direction fn would_exceed(&self) -> bool { - (self.step > 0 && self.current + self.step > self.stop) - || (self.step < 0 && self.current + self.step < self.stop) + (self.step > 0 && self.current.saturating_add(self.step) > self.stop) + || (self.step < 0 && self.current.saturating_add(self.step) < self.stop) } } @@ -155,7 +146,12 @@ impl VTabCursor for GenerateSeriesCursor { } // Handle overflow by truncating to MAX/MIN - self.current = self.current.saturating_add(self.step); + self.current = match self.current.checked_add(self.step) { + Some(val) => val, + None => { + return ResultCode::EOF; + } + }; ResultCode::OK } @@ -171,6 +167,14 @@ impl VTabCursor for GenerateSeriesCursor { return true; } + if self.current == i64::MAX && self.step > 0 { + return true; + } + + if self.current == i64::MIN && self.step < 0 { + return true; + } + false } @@ -204,25 +208,50 @@ impl VTabCursor for GenerateSeriesCursor { #[cfg(test)] mod tests { use super::*; + use quickcheck::{Arbitrary, Gen}; use quickcheck_macros::quickcheck; + #[derive(Debug, Clone)] + struct Series { + start: i64, + stop: i64, + step: i64, + } + + impl Arbitrary for Series { + fn arbitrary(g: &mut Gen) -> Self { + let mut start = i64::arbitrary(g); + let mut stop = i64::arbitrary(g); + let mut iters = 0; + while stop.checked_sub(start).is_none() { + start = i64::arbitrary(g); + stop = i64::arbitrary(g); + iters += 1; + if iters > 1000 { + panic!("Failed to generate valid range after 1000 attempts"); + } + } + // step should be a reasonable value proportional to the range + let mut divisor = i8::arbitrary(g); + if divisor == 0 { + divisor = 1; + } + let step = (stop - start).saturating_abs() / divisor as i64; + Series { start, stop, step } + } + } // Helper function to collect all values from a cursor, returns Result with error code - fn collect_series(start: i64, stop: i64, step: i64) -> Result, ResultCode> { - let mut cursor = GenerateSeriesCursor { - start: 0, - stop: 0, - step: 0, - current: 0, - }; + fn collect_series(series: Series) -> Result, ResultCode> { + let mut cursor = GenerateSeriesVTab::open(); // Create args array for filter let args = vec![ - Value::from_integer(start), - Value::from_integer(stop), - Value::from_integer(step), + Value::from_integer(series.start), + Value::from_integer(series.stop), + Value::from_integer(series.step), ]; - // Validate inputs through filter + // Initialize cursor through filter match GenerateSeriesVTab::filter(&mut cursor, 3, &args) { ResultCode::OK => (), ResultCode::EOF => return Ok(vec![]), @@ -232,6 +261,12 @@ mod tests { let mut values = Vec::new(); loop { values.push(cursor.column(0).to_integer().unwrap()); + if values.len() > 1000 { + panic!( + "Generated more than 1000 values, expected this many: {:?}", + (series.stop - series.start) / series.step + 1 + ); + } match GenerateSeriesVTab::next(&mut cursor) { ResultCode::OK => (), ResultCode::EOF => break, @@ -242,24 +277,22 @@ mod tests { } #[quickcheck] - /// Test that the series length is correct for a positive step + /// Test that the series length is correct /// Example: /// start = 1, stop = 10, step = 1 /// expected length = 10 - fn prop_series_length_positive_step(start: i64, stop: i64) { - // Limit the range to make test run faster, since we're testing with step 1. - let start = start % 1000; - let stop = stop % 1000; - let step = 1; - - let values = collect_series(start, stop, step).unwrap_or_else(|e| { + fn prop_series_length(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; + let values = collect_series(series.clone()).unwrap_or_else(|e| { panic!( "Failed to generate series for start={}, stop={}, step={}: {:?}", start, stop, step, e ) }); - if start > stop { + if series_is_invalid_or_empty(&series) { assert!( values.is_empty(), "Series should be empty for invalid range: start={}, stop={}, step={}, got {:?}", @@ -269,57 +302,17 @@ mod tests { values ); } else { - let expected_len = ((stop - start) / step + 1) as usize; + let expected_len = series_expected_length(&series); assert_eq!( values.len(), - expected_len, - "Series length mismatch for start={}, stop={}, step={}: expected {}, got {}", + expected_len as usize, + "Series length mismatch for start={}, stop={}, step={}: expected {}, got {}, values: {:?}", start, stop, step, expected_len, - values.len() - ); - } - } - - #[quickcheck] - /// Test that the series length is correct for a negative step - /// Example: - /// start = 10, stop = 1, step = -1 - /// expected length = 10 - fn prop_series_length_negative_step(start: i64, stop: i64) { - // Limit the range to make test run faster, since we're testing with step -1. - let start = start % 1000; - let stop = stop % 1000; - let step = -1; - - let values = collect_series(start, stop, step).unwrap_or_else(|e| { - panic!( - "Failed to generate series for start={}, stop={}, step={}: {:?}", - start, stop, step, e - ) - }); - - if start < stop { - assert!( - values.is_empty(), - "Series should be empty for invalid range: start={}, stop={}, step={}", - start, - stop, - step - ); - } else { - let expected_len = ((start - stop) / step.abs() + 1) as usize; - assert_eq!( values.len(), - expected_len, - "Series length mismatch for start={}, stop={}, step={}: expected {}, got {}", - start, - stop, - step, - expected_len, - values.len() + values ); } } @@ -329,20 +322,19 @@ mod tests { /// Example: /// start = 1, stop = 10, step = 1 /// expected series = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] - fn prop_series_monotonic_increasing(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 10000; - let stop = stop % 10000; - let step = (step % 100).max(1); // Ensure positive non-zero step + fn prop_series_monotonic_increasing_or_decreasing(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; - let values = collect_series(start, stop, step).unwrap_or_else(|e| { + let values = collect_series(series.clone()).unwrap_or_else(|e| { panic!( "Failed to generate series for start={}, stop={}, step={}: {:?}", start, stop, step, e ) }); - if start > stop { + if series_is_invalid_or_empty(&series) { assert!( values.is_empty(), "Series should be empty for invalid range: start={}, stop={}, step={}", @@ -352,46 +344,11 @@ mod tests { ); } else { assert!( - values.windows(2).all(|w| w[0] < w[1]), - "Series not monotonically increasing: {:?} (start={}, stop={}, step={})", - values, - start, - stop, - step - ); - } - } - - #[quickcheck] - /// Test that the series is monotonically decreasing - /// Example: - /// start = 10, stop = 1, step = -1 - /// expected series = [10, 9, 8, 7, 6, 5, 4, 3, 2, 1] - fn prop_series_monotonic_decreasing(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 10000; - let stop = stop % 10000; - let step = -((step % 100).max(1)); // Ensure negative non-zero step - - let values = collect_series(start, stop, step).unwrap_or_else(|e| { - panic!( - "Failed to generate series for start={}, stop={}, step={}: {:?}", - start, stop, step, e - ) - }); - - if start < stop { - assert!( - values.is_empty(), - "Series should be empty for invalid range: start={}, stop={}, step={}", - start, - stop, - step - ); - } else { - assert!( - values.windows(2).all(|w| w[0] > w[1]), - "Series not monotonically decreasing: {:?} (start={}, stop={}, step={})", + values + .windows(2) + .all(|w| if step > 0 { w[0] < w[1] } else { w[0] > w[1] }), + "Series not monotonically {}: {:?} (start={}, stop={}, step={})", + if step > 0 { "increasing" } else { "decreasing" }, values, start, stop, @@ -405,20 +362,19 @@ mod tests { /// Example: /// start = 1, stop = 10, step = 1 /// expected step size = 1 - fn prop_series_step_size_positive(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 1000; - let stop = stop % 1000; - let step = (step % 100).max(1); // Ensure positive non-zero step + fn prop_series_step_size(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; - let values = collect_series(start, stop, step).unwrap_or_else(|e| { + let values = collect_series(series.clone()).unwrap_or_else(|e| { panic!( "Failed to generate series for start={}, stop={}, step={}: {:?}", start, stop, step, e ) }); - if start > stop { + if series_is_invalid_or_empty(&series) { assert!( values.is_empty(), "Series should be empty for invalid range: start={}, stop={}, step={}", @@ -428,69 +384,37 @@ mod tests { ); } else if !values.is_empty() { assert!( - values.windows(2).all(|w| (w[1] - w[0]).abs() == step.abs()), + values + .windows(2) + .all(|w| (w[1].saturating_sub(w[0])).abs() == step.abs()), "Step size not consistent: {:?} (expected step size: {})", - values, + values + .windows(2) + .map(|w| w[1].saturating_sub(w[0])) + .collect::>(), step.abs() ); } } #[quickcheck] - /// Test that the series step size is consistent for a negative step - /// Example: - /// start = 10, stop = 1, step = -1 - /// expected step size = 1 - fn prop_series_step_size_negative(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 1000; - let stop = stop % 1000; - let step = -((step % 100).max(1)); // Ensure negative non-zero step - - let values = collect_series(start, stop, step).unwrap_or_else(|e| { - panic!( - "Failed to generate series for start={}, stop={}, step={}: {:?}", - start, stop, step, e - ) - }); - - if start < stop { - assert!( - values.is_empty(), - "Series should be empty for invalid range: start={}, stop={}, step={}", - start, - stop, - step - ); - } else if !values.is_empty() { - assert!( - values.windows(2).all(|w| (w[1] - w[0]).abs() == step.abs()), - "Step size not consistent: {:?} (expected step size: {})", - values, - step.abs() - ); - } - } - - #[quickcheck] - /// Test that the series bounds are correct for a positive step + /// Test that the series bounds are correct /// Example: /// start = 1, stop = 10, step = 1 /// expected bounds = [1, 10] - fn prop_series_bounds_positive(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 10000; - let stop = stop % 10000; - let step = (step % 100).max(1); // Ensure positive non-zero step + fn prop_series_bounds(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; - let values = collect_series(start, stop, step).unwrap_or_else(|e| { + let values = collect_series(series.clone()).unwrap_or_else(|e| { panic!( "Failed to generate series for start={}, stop={}, step={}: {:?}", start, stop, step, e ) }); - if start > stop { + if series_is_invalid_or_empty(&series) { assert!( values.is_empty(), "Series should be empty for invalid range: start={}, stop={}, step={}", @@ -507,50 +431,11 @@ mod tests { start ); assert!( - values.last().map_or(true, |&last| last <= stop), - "Series exceeds stop value: {:?} (stop: {})", - values, - stop - ); - } - } - - #[quickcheck] - /// Test that the series bounds are correct for a negative step - /// Example: - /// start = 10, stop = 1, step = -1 - /// expected bounds = [10, 1] - fn prop_series_bounds_negative(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster. - let start = start % 10000; - let stop = stop % 10000; - let step = -((step % 100).max(1)); // Ensure negative non-zero step - - let values = collect_series(start, stop, step).unwrap_or_else(|e| { - panic!( - "Failed to generate series for start={}, stop={}, step={}: {:?}", - start, stop, step, e - ) - }); - - if start < stop { - assert!( - values.is_empty(), - "Series should be empty for invalid range: start={}, stop={}, step={}", - start, - stop, - step - ); - } else if !values.is_empty() { - assert_eq!( - values.first(), - Some(&start), - "Series doesn't start with start value: {:?} (expected start: {})", - values, - start - ); - assert!( - values.last().map_or(true, |&last| last >= stop), + values.last().map_or(true, |&last| if step > 0 { + last <= stop + } else { + last >= stop + }), "Series exceeds stop value: {:?} (stop: {})", values, stop @@ -561,7 +446,12 @@ mod tests { #[test] fn test_series_empty_positive_step() { - let values = collect_series(10, 5, 1).expect("Failed to generate series"); + let values = collect_series(Series { + start: 10, + stop: 5, + step: 1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Series should be empty when start > stop with positive step" @@ -570,7 +460,12 @@ mod tests { #[test] fn test_series_empty_negative_step() { - let values = collect_series(5, 10, -1).expect("Failed to generate series"); + let values = collect_series(Series { + start: 5, + stop: 10, + step: -1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Series should be empty when start < stop with negative step" @@ -579,7 +474,12 @@ mod tests { #[test] fn test_series_single_element() { - let values = collect_series(5, 5, 1).expect("Failed to generate single element series"); + let values = collect_series(Series { + start: 5, + stop: 5, + step: 1, + }) + .expect("Failed to generate single element series"); assert_eq!( values, vec![5], @@ -589,7 +489,12 @@ mod tests { #[test] fn test_zero_step_is_interpreted_as_1() { - let values = collect_series(1, 10, 0).expect("Failed to generate series"); + let values = collect_series(Series { + start: 1, + stop: 10, + step: 0, + }) + .expect("Failed to generate series"); assert_eq!( values, vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10], @@ -600,27 +505,47 @@ mod tests { #[test] fn test_invalid_inputs() { // Test that invalid ranges return empty series instead of errors - let values = collect_series(10, 1, 1).expect("Failed to generate series"); + let values = collect_series(Series { + start: 10, + stop: 1, + step: 1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Invalid positive range should return empty series, got {:?}", values ); - let values = collect_series(1, 10, -1).expect("Failed to generate series"); + let values = collect_series(Series { + start: 1, + stop: 10, + step: -1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Invalid negative range should return empty series" ); // Test that extreme ranges return empty series - let values = collect_series(i64::MAX, i64::MIN, 1).expect("Failed to generate series"); + let values = collect_series(Series { + start: i64::MAX, + stop: i64::MIN, + step: 1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Extreme range (MAX to MIN) should return empty series" ); - let values = collect_series(i64::MIN, i64::MAX, -1).expect("Failed to generate series"); + let values = collect_series(Series { + start: i64::MIN, + stop: i64::MAX, + step: -1, + }) + .expect("Failed to generate series"); assert!( values.is_empty(), "Extreme range (MIN to MAX) should return empty series" @@ -629,18 +554,12 @@ mod tests { #[quickcheck] /// Test that rowid is always monotonically increasing regardless of step direction - fn prop_series_rowid_monotonic(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster - let start = start % 1000; - let stop = stop % 1000; - let step = if step == 0 { 1 } else { step % 100 }; // Ensure non-zero step + fn prop_series_rowid_monotonic(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; - let mut cursor = GenerateSeriesCursor { - start: 0, - stop: 0, - step: 0, - current: 0, - }; + let mut cursor = GenerateSeriesVTab::open(); let args = vec![ Value::from_integer(start), @@ -674,79 +593,56 @@ mod tests { ); } - // #[quickcheck] - // /// Test that integer overflow is properly handled - // fn prop_series_overflow(start: i64, step: i64) { - // // Use values close to MAX/MIN to test overflow - // let start = if start >= 0 { - // i64::MAX - (start % 100) - // } else { - // i64::MIN + (start.abs() % 100) - // }; - // let step = if step == 0 { 1 } else { step }; // Ensure non-zero step - // let stop = if step > 0 { i64::MAX } else { i64::MIN }; - - // let result = collect_series(start, stop, step); - - // // If we would overflow, expect InvalidArgs - // if start.checked_add(step).is_none() { - // assert!( - // matches!(result, Err(ResultCode::InvalidArgs)), - // "Expected InvalidArgs for overflow case: start={}, stop={}, step={}", - // start, stop, step - // ); - // } else { - // // Otherwise the series should be valid - // let values = result.unwrap_or_else(|e| { - // panic!( - // "Failed to generate series for start={}, stop={}, step={}: {:?}", - // start, stop, step, e - // ) - // }); - - // // Verify no values overflow - // for window in values.windows(2) { - // assert!( - // window[0].checked_add(step).is_some(), - // "Overflow occurred in series: {:?} + {} (start={}, stop={}, step={})", - // window[0], step, start, stop, step - // ); - // } - // } - // } - #[quickcheck] /// Test that empty series are handled consistently - fn prop_series_empty(start: i64, stop: i64, step: i64) { - // Limit the range to make test run faster - let start = start % 1000; - let stop = stop % 1000; - let step = if step == 0 { 1 } else { step % 100 }; // Ensure non-zero step + fn prop_series_empty(series: Series) { + let start = series.start; + let stop = series.stop; + let step = series.step; - let result = collect_series(start, stop, step); - - match result { - Ok(values) => { - if (step > 0 && start > stop) || (step < 0 && start < stop) { - assert!( - values.is_empty(), - "Series should be empty for invalid range: start={}, stop={}, step={}", - start, - stop, - step - ); - } else if start == stop { - assert_eq!( - values, - vec![start], - "Series with start==stop should contain exactly one element" - ); - } - } - Err(e) => panic!( - "Unexpected error for start={}, stop={}, step={}: {:?}", + let values = collect_series(series.clone()).unwrap_or_else(|e| { + panic!( + "Failed to generate series for start={}, stop={}, step={}: {:?}", start, stop, step, e - ), + ) + }); + + if series_is_invalid_or_empty(&series) { + assert!( + values.is_empty(), + "Series should be empty for invalid range: start={}, stop={}, step={}", + start, + stop, + step + ); + } else if start == stop { + assert_eq!( + values, + vec![start], + "Series with start==stop should contain exactly one element" + ); + } + } + + fn series_is_invalid_or_empty(series: &Series) -> bool { + let start = series.start; + let stop = series.stop; + let step = series.step; + (start > stop && step > 0) || (start < stop && step < 0) || (step == 0 && start != stop) + } + + fn series_expected_length(series: &Series) -> usize { + let start = series.start; + let stop = series.stop; + let step = series.step; + if step == 0 { + if start == stop { + 1 + } else { + 0 + } + } else { + ((stop.saturating_sub(start)).saturating_div(step)).saturating_add(1) as usize } } } From cb9d929eab5ce91f55dcf63120dd39d214699d52 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Fri, 7 Feb 2025 10:09:42 +0200 Subject: [PATCH 4/5] call cursor methods instead of duplicating logic --- extensions/series/src/lib.rs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/extensions/series/src/lib.rs b/extensions/series/src/lib.rs index ffa0871c6..ddae6aa99 100644 --- a/extensions/series/src/lib.rs +++ b/extensions/series/src/lib.rs @@ -83,24 +83,11 @@ impl VTabModule for GenerateSeriesVTab { } fn next(cursor: &mut Self::VCursor) -> ResultCode { - // Check for invalid ranges (empty series) first - if cursor.eof() { - return ResultCode::EOF; - } - - // Handle overflow - cursor.current = match cursor.current.checked_add(cursor.step) { - Some(val) => val, - None => { - return ResultCode::EOF; - } - }; - - ResultCode::OK + cursor.next() } fn eof(cursor: &Self::VCursor) -> bool { - cursor.is_invalid_range() || cursor.would_exceed() + cursor.eof() } } From 49e08c43b729772a877d4210ea5453795ad64051 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Fri, 7 Feb 2025 10:11:31 +0200 Subject: [PATCH 5/5] remove invalid comments --- extensions/series/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/series/src/lib.rs b/extensions/series/src/lib.rs index ddae6aa99..df2d67da1 100644 --- a/extensions/series/src/lib.rs +++ b/extensions/series/src/lib.rs @@ -127,12 +127,10 @@ impl VTabCursor for GenerateSeriesCursor { type Error = ResultCode; fn next(&mut self) -> ResultCode { - // Check for invalid ranges (empty series) first if self.eof() { return ResultCode::EOF; } - // Handle overflow by truncating to MAX/MIN self.current = match self.current.checked_add(self.step) { Some(val) => val, None => {