Fixes generate_series to handle NULL arguments

[davidselassie]

SQLite's generate_series behavior has it return an empty result set whenever any of the arguments are NULL.

SQLite version 3.37.2 2022-01-06 13:25:41
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> SELECT value FROM generate_series(0, 3);
┌───────┐
│ value │
├───────┤
│ 0     │
│ 1     │
│ 2     │
│ 3     │
└───────┘
sqlite> SELECT value FROM generate_series(0, NULL);
sqlite> SELECT value FROM generate_series(NULL);
sqlite> SELECT value FROM generate_series(0, 3, NULL);
sqlite> SELECT value FROM generate_series(0, NULL, 2);
sqlite> SELECT value FROM generate_series(NULL, 3, 2);
sqlite> 

rusqlite's re-implementation of generate_series that's behind the series Cargo feature has code suggesting it should have the same behavior of returning no rows, but instead errors out if any of the arguments are NULL.

SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some("Invalid filter parameter type Null at index 0"))

This error was due to the args.get(i)? originally being inferred to be args.get::<i64>(i)? since they're being assigned to the parameters directly and thus NULL causes an error.

This PR fixes the behavior of rusqlite's generate_series to match that of SQLite's extension. It does this by interpreting those arguments as Option<i64> and if they are None`, then marks that the range should be set to result in no rows.

Adds some tests to the existing series module tests that show that NULL arguments result in no rows.

[gwenn]

See:
https://www.sqlite.org/cgi/src/info/3328e828e28ca719
And:
b60ffe1#diff-1592000f020c13e61a6b81bdd6bec4f8c2a54bcadb4949cb660bdb81c5b1c346R215-R222

[davidselassie]

I saw that that b60ffe1#diff-1592000f020c13e61a6b81bdd6bec4f8c2a54bcadb4949cb660bdb81c5b1c346R215-R222 already existed, but it will never be reached because b60ffe1#diff-1592000f020c13e61a6b81bdd6bec4f8c2a54bcadb4949cb660bdb81c5b1c346R196 (and the equivalent lines for the other arguments) does not handle a NULL argument without erroring and returning early.

To confirm the behavior I was seeing, if you run tests on the first commit in this branch 84362c0 which only adds tests asserting no rows returned for NULL args, you'll see that they do not pass.

Is the goal to have the rusqlite version match the original logic as closely as possible? In that case I could change the lines to match the behavior of sqlite3_value_int64 in the original (which seems to handle NULL) be something like

self.min_value = args.get::<Option<i64>>(i)?.unwrap_or_default();

or

self.min_value = args.get(i).or_else(|e| match e {
   Error::InvalidFilterParameterType(_, Type::Null) => Ok(0),
   e => Err(e),
})?;

and keep the null checking loop below.

Or are you bringing this up for another reason?

[gwenn]

self.min_value = args.get::<Option>(i)?.unwrap_or_default();

Yes, could you please use this ?
Thanks.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09 🎉

Comparison is base (3fe9a7f) 80.82% compared to head (256cfdd) 80.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1357      +/-   ##
==========================================
+ Coverage   80.82%   80.92%   +0.09%     
==========================================
  Files          49       49              
  Lines       10057    10059       +2     
==========================================
+ Hits         8129     8140      +11     
+ Misses       1928     1919       -9     

Impacted Files	Coverage Δ
src/vtab/series.rs	83.18% <100.00%> (+3.84%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gwenn]

Thanks.