fix(duckdb): normalize a timestamp operand only when its type needs it (fixes spiceai/spiceai#12574)

[claudespice]

Summary

The DuckDB comparison rewrite renders a bare operand as TO_TIMESTAMP(EPOCH_MS(<operand>) / 1000)
so DuckDB sees two TIMESTAMPTZ values. It decided that without knowing the operand's type,
because no schema reached the unparser — so it also fired on columns that compare exactly without
it, truncating them to whole milliseconds. A </>/= against a literal inside that millisecond
then selected different rows, silently.

That is not a corner case. Timestamp(_, Some(tz)) is what every timezone-aware column in a
DuckDB-accelerated dataset lands on, and it maps to TIMESTAMPTZ, the one type the rewrite could
only harm.

Measured against DuckDB v1.5.5 rather than reasoned about:

Arrow type	DuckDB type	compares with a TIMESTAMPTZ bare?	rewrite
Timestamp(Second, None)	TIMESTAMP_S	no — "Cannot compare values of type TIMESTAMP_S and type TIMESTAMP WITH TIME ZONE"	kept
Timestamp(Millisecond, None)	TIMESTAMP_MS	no, same refusal	kept
Timestamp(Nanosecond, None)	TIMESTAMP_NS	no, same refusal	kept
Timestamp(Microsecond, None)	TIMESTAMP	yes, but in the session's TimeZone while the literal is a UTC instant	kept — it pins the UTC reading
Timestamp(_, Some(tz))	TIMESTAMPTZ	yes, exactly	dropped

So the timezone decides it, not the unit, and only the last row changes behaviour.

The literal on the other side of that comparison was truncated too, by integer division:
2026-01-01 00:00:00.000999Z rendered as TO_TIMESTAMP(1767225600). An exact column compared
against a whole-second literal is still the wrong answer, so both halves are fixed here. A
whole-second literal renders byte-identically, which is why no existing expectation moved.

Changes

• expr.rs: to_sql_with_engine_and_schema takes the schema the expression's columns come from and
  threads it to the one site that needs it. to_sql_with_engine delegates with None, so a caller
  with no schema to offer renders exactly what it rendered before.
• expr.rs: duckdb_normalizes_timestamp_operand resolves the operand against that schema.
  Un-resolvable — no schema, not a Column, or a column the schema does not carry — normalizes, as
  before; only a resolved type declines. A non-temporal operand also stops being normalized: it
  never bound either way, but DuckDB now names the types it could not compare instead of failing on
  the division the rewrite introduced.
• expr.rs: the DuckDB timestamp literal arms keep their sub-second digits. The nanosecond arm
  drops to microseconds in integer arithmetic — 1.7e18 exceeds the 2^53 an f64 holds exactly, and
  a TIMESTAMPTZ cannot carry nanoseconds anyway.
• util/dml.rs: filters_to_sql_with_schema / assignments_to_sql_with_schema, with the existing
  two delegating to them.
• duckdb/write.rs: delete_from and update pass the table's schema.

Performance impact

None on any per-row or per-batch path. filters_to_sql* is reached only from
TableProvider::delete_from / update, which DataFusion calls once at plan time; the rendered
String is stored on the sink and the DELETE/UPDATE then executes entirely inside DuckDB. The
schema lookup is a linear scan over the field list, once per comparison, and Option<&Schema> is a
register-sized Copy through the recursion.

Test plan

• make lint
• make test

New tests, all executed against a real in-memory DuckDB rather than asserting on rendered text
alone:

• a_microsecond_timestamptz_deletes_the_row_inside_the_millisecond — the reported defect. Two rows
  a microsecond apart, a filter naming the instant between them; asserts the right row goes, and
  that with the column-side truncation left in place it does not.
• a_naive_millisecond_column_is_still_normalized_and_still_binds — the rewrite still covers what it
  was written for.
• subtracting_a_timestamp_from_a_timezone_aware_column_still_binds and
  test_duckdb_does_not_normalize_a_non_temporal_column — the two arms the type test newly leaves
  bare.
• an_update_stores_the_sub_second_instant_it_was_assigned — a SET value carries the same literal
  rendering as a filter.
• Rendering tests over all four TimeUnits with and without a timezone, plus the no-schema,
  absent-schema and column-not-in-schema fallbacks.

Both production changes were neutered independently and re-neutered after the review pass: forcing
the type test to always normalize fails 4 tests, restoring integer division on the literal fails 4.

Review gate

• Adversarial review: skipped — codex exited 1 with "Your workspace is out of credits. Ask
  your workspace owner to refill in order to continue."; grok is not installed, so the receipt
  records engine=none exit=1. Its angles were hand-run instead, and two changed the diff: whether
  a non-temporal operand could now bind rather than error (measured — it still refuses, with a
  better message), and whether TIMESTAMPTZ - TIMESTAMPTZ binds once subtraction is no longer
  normalized (measured — it does, returning the same interval). Both are now tests.
• /security-review: skipped — it scoped itself to the invoking session's working directory and
  was handed an empty diff, which would have minted a clean pass over nothing. Hand-audited instead.
  The only category that applies is SQL injection, and the one new rendering path formats an i64
  through f64 Display, which cannot emit a quote, whitespace, exponent, inf or NaN; the new
  schema lookup uses the column name as a lookup key only, and identifiers still render through
  quoted_identifier. The gate cannot widen a WHERE clause into a tautology — it decides whether
  an operand is wrapped, never the predicate's structure.
• /simplify: ran, 4 agents. Applied: collapsed the type test to Timestamp(_, None) (the unit
  arms all agreed); column_with_name for field_with_name, which built and discarded a Vec of
  every field name on a supported path; bound the scaled seconds once per literal arm and the schema
  once in update; a shared fixture and one query helper for the DuckDB tests. It also caught two
  comments this change had made false — is_duckdb_timestamp_operand claiming an operand's type is
  not visible, and handle_cast claiming its rendering must match the literal arms, which now
  differ in precision. Skipped, with reasons: changing the three existing signatures instead of
  adding *_with_schema (this crate is published upstream as datafusion-contrib/datafusion-table-providers,
  so additive is the merge-friendly shape, and to_sql → to_sql_with_engine is the same pattern
  already in this file); merging the new test module into duckdb_execution_tests (its helper is
  hardcoded to a different fixture, so it would mean rewriting tests outside this diff); a
  RenderCtx struct for the two context parameters (churns twelve call sites for no behaviour
  change).

Review round 2 — Copilot

Three findings, all valid, all acted on:

• A resolved Date32/Date64 column was being taken out of the normalization set. A real
  regression, and the one case where the rewrite is load-bearing for the reference frame rather
  than for binding: DuckDB promotes a bare DATE to a TIMESTAMPTZ at midnight in the session's
  TimeZone, while the rendered literal is midnight UTC. Measured on v1.5.5,
  "dt" = TO_TIMESTAMP(1767225600) answers false under America/Los_Angeles and true under
  UTC. Dates stay normalized; a DELETE run under both session timezones pins it.
• The doc called the rewrite lossless for naive timestamps. It is not — EPOCH_MS truncates a
  naive TIMESTAMP or TIMESTAMP_NS to the millisecond, measured. Corrected, with the remainder
  filed as A naive microsecond or nanosecond timestamp column is still truncated to the millisecond by the DuckDB normalization spiceai#13146 and the reason it is kept anyway (dropping it would trade a bounded
  truncation for a session-timezone-dependent result).
• The literal-precision comment overclaimed. Also correct: TO_TIMESTAMP takes a DOUBLE, so
  microsecond resolution holds only to about the year 2255 — Arrow's nanosecond range runs to 2262.
  Its suggested remedy, rendering the fraction from integer arithmetic, does not fix it: at 2260
  both forms land on the same wrong microsecond, because DuckDB's own parse is what rounds, and the
  naive {sec}.{frac:06} form is outright wrong for pre-epoch values (-1µs renders as -1.999999,
  which round-trips to -1999999µs). The rendering is unchanged and the comment now states the real
  bound.

Follow-ups filed

• Timestamp literals lose every digit below the second for ODBC, Spark and ClickHouse spiceai#13143 — the same second-truncation still applies to ODBC, Spark and ClickHouse,
  which share the catch-all arm. Left out because it needs measuring against each engine.
• The DuckDB search-index filter pushdown renders without a schema, so it still truncates a timezone-aware timestamp spiceai#13144 — the DuckDB search-index filter pushdown in spiceai/spiceai renders
  without a schema and has one in scope; blocked on the rev bump for this PR.
• A DuckDB comparison between two timestamp columns is never normalized, so DuckDB refuses the statement spiceai#13145 — a comparison between two timestamp columns is still never normalized, so
  DuckDB refuses it. Fixing that widens the rewrite where this change narrows it.
• A naive microsecond or nanosecond timestamp column is still truncated to the millisecond by the DuckDB normalization spiceai#13146 — a naive microsecond or nanosecond column is still truncated to the
  millisecond by the rewrite it keeps. Pinning UTC losslessly needs AT TIME ZONE, an ICU-extension
  dependency this rendering does not take.

Fixes spiceai/spiceai#12574

[claudespice]

@copilot review

[Copilot]

Pull request overview

Adds schema-aware DuckDB timestamp rendering to avoid unnecessary precision loss during DML operations.

Changes:

• Threads table schemas through expression rendering.
• Restricts timestamp operand normalization by resolved type.
• Preserves sub-second DuckDB timestamp literals and adds regression tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
core/src/util/dml.rs	Adds schema-aware DML rendering and DuckDB tests.
core/src/sql/sql_provider_datafusion/expr.rs	Implements type-aware normalization and sub-second rendering.
core/src/duckdb/write.rs	Passes table schemas into DELETE and UPDATE rendering.

Suppressed comments (2)

core/src/sql/sql_provider_datafusion/expr.rs:319

• Converting the microsecond count to f64 makes the newly added DuckDB rendering inexact for valid timestamps after roughly 2255 (for example, 2^53 + 1 microseconds is rounded before division), so a filter or assignment can still target a neighboring instant. Build the decimal seconds from the integer and scale instead; BigDecimal is already used in this renderer.

                let seconds = *value as f64 / 1_000_000.0;

core/src/sql/sql_provider_datafusion/expr.rs:341

• The millisecond path has the same precision boundary: valid large i64 millisecond values exceed 2^53, so the cast can round away a millisecond before the DuckDB SQL is produced. Render exact scaled decimal seconds rather than passing through f64.

                let seconds = *value as f64 / 1000.0;

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.