Repository navigation
fix(sqlite): shift a timestamp by an interval through datetime(), choosing date() by the operand's type - #88
Conversation
…osing date() by the operand's type `SQLiteIntervalVisitor` rewrites `expr +/- INTERVAL` into SQLite's `date()` when the interval has no positive intraday part, and into `datetime()` otherwise. SQLite's `date()` returns the calendar day alone, so `ts + INTERVAL '1 day'` and every subtracted intraday interval (`ts - INTERVAL '1 hour'`, whose negated hours fail the `> 0` test) discarded the time of day of a timestamp column: a projection answered midnight, and a filter over the shifted timestamp compared the day alone and kept rows the predicate excludes (spiceai/spiceai#14714). Choose the function by the operand's type rather than by the interval. The type is only known on the logical plan, so the federated scan's `SQLTable` now carries a logical optimizer, `mark_date_operands`, that wraps every `DATE` operand of interval arithmetic in an identity marker function before the plan is unparsed; the visitor then renders a marked operand through `date()` and anything else through `datetime()`, and consumes the marker. A `DATE` stays date-only text, so it still compares equal to a date literal, which is rendered date-only too; a timestamp, or a date cast to one, keeps its time of day. A chain of intervals is rewritten from the outside in, one call per interval. Every rewritten expression keeps its name, aliased back where the marker changed it: the plan's schema may not change under a logical optimizer, and the unparser finds a window expression by its name. Rendering `datetime()` unconditionally would have broken the date comparison; dispatching on the text's length in SQL would have misread a date cast to a timestamp (its text is still ten characters) and, by cloning the operand into the branches, grown a chain of intervals exponentially; and SQLite's `'subsec'` modifier would have put `.000` on every whole-second result and broken `=`/`<=` against a whole-second timestamp literal. None is used.
There was a problem hiding this comment.
🟡 Changes recommended
The identity marker changes non-null date expressions to nullable, violating logical optimizer schema preservation.
1 open finding
What changed in this PR
Fixes SQLite interval arithmetic to select date() or datetime() based on operand type.
Changes:
- Marks typed
DATEoperands before SQL unparsing. - Preserves timestamp time components and date-only comparisons.
- Adds comprehensive federated regression tests.
| File | Description |
|---|---|
core/src/sqlite/sqlite_interval.rs |
Adds operand marking, type-aware rewriting, and tests. |
core/src/sqlite/federation.rs |
Installs logical optimizers for SQLite federation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Status: blocked on two things this bot can't clear from here. 1. Red checks on
Re-running the failed jobs on the same commit needs admin here: 2. One open Copilot thread on |
…marker
`DateOperand` is an identity — `invoke_with_args` returns its argument and
`return_type` returns the argument's type — but it left
`ScalarUDFImpl::return_field_from_args` at the default, which builds
`Field::new(self.name(), self.return_type(..)?, true)`. The `true` is
unconditional, so wrapping a `NOT NULL` `DATE` column reported it nullable:
Field { name: "sqlite_date_operand(d)", data_type: Date32, nullable: true }
and every expression whose field is computed from it widened with it, changing
the optimized plan's schema against the one the unparsed plan is validated on.
Return the argument's field instead, so type, nullability and metadata all
survive the marker.
Raised by Copilot on spiceai#88.
…ot the projection
`mark_date_operands` resolves a node's expressions against its inputs, falling
back to the node's own schema when it has none — which for a `TableScan` is the
*projected* schema. A scan's pushed-down filters are not evaluated against the
projection, though: they read the table. So
SELECT id FROM t WHERE d + INTERVAL '1 day' = DATE '2026-10-04'
pushes the filter into the scan and projects `id` alone, `d` is absent from the
schema the marker is handed, `operand.get_type` fails, and the operand is left
unmarked. Since this PR chooses the SQLite function by the marker rather than by
the interval, an unmarked operand now renders through `datetime()`:
datetime(d, '+1 days') -> '2026-10-04 00:00:00'
which equals no `DATE` literal, so SQLite drops rows the predicate matches.
Before this PR the same filter rendered `date(d, '+1 days')` and kept them, so
this is a regression the rest of the change introduces rather than a pre-existing
gap.
Merge the scan's qualified source schema in before the projected one, so a column
the projection drops is still resolvable for the filters that name it.
Found by the pre-push adversarial review (codex) on spiceai#88.
|
Status update, not a new escalation. The standing hold listed two blockers; one of them is cleared, and the work that cleared it turned up a third thing worth knowing about. Cleared: the Copilot nullability threadIt was recorded here as "looks valid but hasn't been reproduced (unverified, code inspection only)". It is now reproduced and fixed in New, and the reason this push was worth making: a silent wrong-results regression in this PRThe pre-push adversarial review caught it, and I reproduced it before changing anything. SELECT id FROM t WHERE d + INTERVAL '1 day' = DATE '2026-10-04'
Because this PR moves the choice of SQLite function from the interval to the marker, an unmarked operand now renders Fixed in Split out rather than widenedThe review's second round raised sub-second precision loss: Still blocking, unchanged
One correction to the earlier note here, now that there is a measurement behind it: |
|
Status update, not a new escalation. The Across the five One honest wrinkle specific to this PR: the error text differs between its two runs. Table, ruled-out confounders and calibration: #89's comment. Recommended action is merge #89, then re-run this check rather than waiting it out. |
|
Retracting my previous comment ( The refuting artifact is a same-commit pass/fail pair: The "honest wrinkle" I flagged — that your two runs failed with different error text — was in fact the answer, not a wrinkle. Ignore "merge #89, then re-run this check". The standing hold stands as written: both red checks are pre-existing repo-wide instability ( |
Summary
SQLiteIntervalVisitorrewritesexpr +/- INTERVALinto SQLite'sdate()when the interval has no positive intraday part, and intodatetime()otherwise. SQLite'sdate()returns the calendar day alone, sots + INTERVAL '1 day'and every subtracted intraday interval (ts - INTERVAL '1 hour', whose negated hours fail the> 0test) discarded the time of day of a timestamp column: a projection answered midnight, and a filter over the shifted timestamp compared the day alone and kept rows the predicate excludes (spiceai/spiceai#14714; Turso in Spice shares this visitor).The function is now chosen by the operand's type, never by the interval. The type is only known on the logical plan, so the federated scan's
SQLTable(SQLiteRemoteTable) carries a logical optimizer,mark_date_operands, that wraps everyDATEoperand of interval arithmetic in an identity marker function before the plan is unparsed; the visitor renders a marked operand throughdate()and anything else throughdatetime(), and consumes the marker. ADATEstays date-only text, so it still compares equal to a date literal (rendered date-only too); a timestamp, or a date cast to one, keeps its time of day; a chain of intervals is rewritten from the outside in, one call per interval; every rewritten expression keeps its name (aliased back where the marker changed it), since the plan's schema may not change under a logical optimizer and the unparser finds a window expression by its name; and subqueries are traversed, since a federated statement keeps its same-sourceEXISTS.Refs spiceai/spiceai#14714 (closed by the pin bump in spiceai/spiceai that picks this up and wires Turso to
SQLiteRemoteTable).Approaches that were tried and rejected
Each was reproduced against SQLite before being dropped (the adversarial review below found the first three):
datetime()unconditionally:d + INTERVAL '1 day' = DATE '2026-10-03'stops matching, because the date literal is rendered date-only while the shifted value carries00:00:00.'subsec'modifier: puts.000on every whole-second result, sots + INTERVAL '1 hour' = TIMESTAMP '2026-10-02 01:30:00'and<=at that boundary stop matching.CASE WHEN length(x) <= 10 THEN date(...) ELSE datetime(...) ENDin SQL: a date cast to a timestamp renders asCAST(d AS TEXT), still ten characters, so its hour is discarded; and cloning the operand into the branches grows a chain of intervals exponentially.Changes
core/src/sqlite/sqlite_interval.rs:mark_date_operands(the logical optimizer, with its identity marker UDFsqlite_date_operand),create_shifted_exprreplacescreate_datetime_function/SQLiteIntervalType/intraday, and the visitor consumes the marker. Unit tests pin the rendering for whole-day, intraday, negated and marked operands, the full SQL rewrite for the report's shapes plus a chain and a leftover marker, and the plan marking (only aDATEoperand, a cast to timestamp left alone, names preserved).core/src/sqlite/federation.rs:SQLiteRemoteTable, theSQLTablethe federated scan reads, whoselogical_optimizerismark_date_operands.=,<=,>), dates (projection, 25-hour and chained intervals,=/<=/>against a date literal), a date cast to a timestamp, an unaliased projection, anORDER BY, a windowORDER BY, and anEXISTSsubquery.Reproduction
A probe crate drove the federated
SQLiteTableover an in-memory table (tstimestamps2026-10-02T12:34:56,2026-10-02T00:30:00,2026-01-31T23:59:59;dthe same days), the wayspiceddoes. On the base of this branch (b4a86350d5), every timestamp case was wrong and the filter kept the excluded row:On this branch, with the SQL sent to SQLite now reading
datetime(rows.ts, '-1 hours')anddate(rows.d, '+1 days'):The federated tests, run against the base implementation (its production code with these tests) and then with the fix:
The guards for the rejected approaches were watched failing against each one: the boundary and date-literal tests against unconditional
datetime()+subsec; the cast-to-timestamp test against theCASErewrite; the window and name tests against the marker without name preservation; theEXISTStest against the marker without subquery traversal.Test plan
cargo test -p datafusion-table-providers --lib --no-default-features --features sqlite-federation,sqlite-bundled -- sqlite::sqlite_interval— 19 passed, 0 failedcargo 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 only used under other features;cargo clippy --all-features, which CI runs, does not report them)make test)Known limitation, pre-existing
datetime()renders whole seconds, so a stored timestamp's fractional seconds are truncated by the shift, as they already were on the intraday path; keeping them needs a representation the dialect's timestamp literals and comparisons agree on, and is tracked in spiceai/spiceai#14903.Review gate
Attests the two fix commits pushed onto this PR (
591ca13..28178f9), not the author's original change.codex— scoped toorigin/spiceai-55...HEAD, run twice (the approach changed between them).12b07eb— verdictneeds-attention, 1 medium: a scan's pushed-down filter was resolved against the projected schema, so a filteredDATEcolumn the projection drops went unmarked and rendered throughdatetime(). Reproduced at the logical-plan level (the filter renderedd + <interval> = …where the projected case renderssqlite_date_operand(t.d) + …) and fixed in28178f9, witha_scan_filter_marks_a_date_column_its_projection_dropsfailing before it and passing after.28178f9— the round-1 finding is gone; verdictneeds-attentionon a new high:datetime()formats to whole seconds, so sub-second precision is still lost. Measured on SQLite 3.50.4 (datetime('2026-10-02 12:34:56.789123','+1 days')→2026-10-03 12:34:56). Out of scope and filed as SQLite federation: interval arithmetic through datetime() truncates sub-second precision spiceai#14950 — before this PR the same expression rendereddate()and lost the entire time of day, so this PR narrows the loss rather than introducing it.591ca13..28178f9— no findings. The commits add two imports, oneScalarUDFImplmethod and two tests: no user-input parsing, no SQL string construction, no I/O, no authn/authz surface, no secrets, no logging.cargo fmt --all -- --checkclean;cargo clippy -p datafusion-table-providers --lib --features sqlite-bundled,sqlite-federation -- -D warningsclean;cargo test … -- sqlite_interval→22 passed; 0 failed(rustc 1.98.1). The host cannot link the defaultsqlite3.lib, so these usesqlite-bundled; the repo's fullmake test/make lintneed system libs this host lacks and were left to CI.