Repository navigation
fix(sqlite): return the error SQLite raises mid-query instead of ending the rows early - #87
Conversation
…ng the rows early `rows_to_arrow` read rows with `if let Ok(Some(row))` and `while let Ok(Some(row))`, so an `Err` from `rows.next()` was treated as the end of the result. A statement that SQLite fails partway through, such as `SUM` over `BIGINT` values whose total passes `i64::MAX` (`integer overflow`), therefore returned the rows produced before the error, or none at all, as a successful answer: an ungrouped aggregate came back as zero rows with no error logged (spiceai/spiceai#14661). Propagate the error as `FailedToReadRow`, so the query fails with SQLite's message and never returns a partial or empty result as if it were the answer.
There was a problem hiding this comment.
🟢 Approved
Both row-fetch paths correctly propagate errors with direct and federated regression coverage.
0 open findings
What changed in this PR
Fixes SQLite row reading so mid-query errors propagate instead of returning partial or empty results.
Changes:
- Adds
FailedToReadRowerror propagation for both row-fetch paths. - Adds direct and federated regression tests for SQLite integer overflow.
| File | Description |
|---|---|
core/src/sql/arrow_sql_gen/sqlite.rs |
Propagates row-read errors and adds regression tests. |
core/src/sqlite/federation.rs |
Tests error propagation through federated queries. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The failureJob Every other job in that run is green, including The same test fails on unrelated branches, with three different engine errors
Three distinct DuckDB storage/transaction faults, one test, four branches, none of which share a subject. The job's own retry wrapper already special-cases Why this PR cannot be the causeIts entire diff is SQLite-side:
Unverified, offered only as a leadIn the failing cycle the reported What I am asking forThis PR is |
|
Correcting my own earlier triage on this thread. I classified the
Full table, the confounders I ruled out, and the calibration (n=5, and I have The part of the original triage that stands: this PR's diff still cannot reach that test. It touches The standing hold is otherwise unchanged: |
|
Retracting my previous comment ( What refutes my correction is a same-commit pass/fail pair I had not looked for. The same push produced It is also already tracked: So ignore my "merge #89, then re-run this check" recommendation. #89 does not fix this, and its own description says so. The standing hold is unchanged and correct: |
…error and interval fixes, and read Turso's shifted timestamps (fixes spiceai#14661) (spiceai#15444) * test(accelerators): DuckDB upsert keeps a repeated key's last copy; SQLite fails a query it aborts; SQLite and Turso interval shifts keep the time of day Regression tests for spiceai#14643, spiceai#14661 and spiceai#14714, written against trunk before the datafusion-table-providers pin moves to the fork fixes they guard. * fix(accelerators): move datafusion-table-providers to the DuckDB upsert, SQLite row-error and SQLite interval fixes, and mark Turso's date operands Pins datafusion-table-providers at 87c2cba (spiceai/datafusion-table-providers#87, spiceai#88, spiceai#89). Turso builds its own federated table, so it now installs SQLiteRemoteTable and mark_federated_date_operands, which the shared interval rewrite reads to shift a DATE through date() and everything else through datetime(). * test(turso): read the timestamp SQLite's datetime() answers; check interval shifts reach the engine through federated projections * fix(turso): read the YYYY-MM-DD HH:MM:SS text SQLite's datetime() answers as a UTC timestamp, not NULL A shifted timestamp reaches Turso as datetime(ts, '-1 hours'), whose answer is not RFC3339, so the reader returned NULL for every row. * refactor(tests): share the CSV dataset and federated-SQL helpers, fold the upsert tests onto one helper, and pin rows inside the interval cases * test(sqlite): require the overflowing call in the SQL sent to SQLite, not anywhere in the plan * fix(accelerators): pin datafusion-table-providers at the SQLite fixes, before the DuckDB upsert statements that invalidate a DuckDB database spiceai/datafusion-table-providers#89 splits a DuckDB upsert into several statements in one transaction, and on DuckDB 1.4.4 a reused table then fails with an internal error that invalidates the database (604 of 1000 overwrites in a release A/B; 0 of 1000 before it). Pin spiceai#88's merge commit instead, and drop the spiceai#89 guard tests and ledger row. --------- Co-authored-by: t <a@b.c>
Summary
rows_to_arrow(the SQLite row reader behindquery_arrowandget_schema) read rows withif let Ok(Some(row))/while let Ok(Some(row)), so anErrfromrows.next()was taken as the end of the result. A statement SQLite fails partway through returned the rows produced before the error, or none at all, as a successful answer: an ungroupedSUMoverBIGINTvalues whose total passesi64::MAXcame back as zero rows, with no error (spiceai/spiceai#14661).The error is now propagated as
FailedToReadRow, so the query fails with SQLite's own message and never returns a partial or empty result as if it were the answer.Refs spiceai/spiceai#14661 (closed by the pin bump in spiceai/spiceai that picks this up).
Changes
core/src/sql/arrow_sql_gen/sqlite.rs: bothrows.next()reads propagate the error (FailedToReadRow { source }, displayed asFailed to read the next row: {source}); the function's# Errorsdoc says so.an_error_sqlite_raises_on_the_first_row_fails_the_read(the only row of an ungroupedSUMoverflows) andan_error_sqlite_raises_after_a_row_fails_the_read(a row is produced before the error fires, so a dropped error would have truncated the answer rather than emptied it).core/src/sqlite/federation.rs:an_error_sqlite_raises_mid_query_fails_the_federated_queryruns the overflowingSUMthrough the federated provider (asserting it is pushed down) and requires the failure.Reproduction
A probe crate drove the federated
SQLiteTableover an in-memory table of threeBIGINT 9000000000000000000values, the wayspiceddoes. On the base of this branch (b4a86350d5):On this branch:
The three tests, run with the fix reverted (the two
rows.next()lines put back) and then with it:Test plan
cargo test -p datafusion-table-providers --lib --no-default-features --features sqlite-federation,sqlite-bundled -- arrow_sql_gen::sqlite sqlite::federation— 12 passed, 0 failed (output above for the three new tests)cargo clippy -p datafusion-table-providers --no-default-features --features sqlite-federation,sqlite-bundled --tests -- -D warnings— no finding in the two changed files (the pre-existing dead-code findings it reports incore/tests/andsql_provider_datafusion/mod.rscome from test helpers that are only used under other features;cargo clippy --all-features, which CI runs, does not report them)make test)Review gate
codex— jobreview-mv09decb-c994p8— verdict approve, no findings ("Both row-reading paths now propagate SQLite errors before returning a batch, and the federation path preserves the failure")/code-review: 2 cosmetic findings in the new tests (a lost line continuation in a SQL literal, a raw newline in an assertion message), both fixed; 2 informational notes, no change needed/security-review: no findings (no new input path; the change only propagates an error SQLite already raised)/simplify: not run (the skill diffs the main checkout, not this fork worktree); the diff was read for cleanup by hand