Skip to content

d1 import: broaden strftime() date/time coverage, fix boolean CHECK literals#1299

Merged
no-itsbackpack merged 6 commits into
mainfrom
fix/d1-strftime-now-timestamptz-default
Jul 22, 2026
Merged

d1 import: broaden strftime() date/time coverage, fix boolean CHECK literals#1299
no-itsbackpack merged 6 commits into
mainfrom
fix/d1-strftime-now-timestamptz-default

Conversation

@claude

@claude claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Requested by Elom Gomez · Slack thread

Summary

D1 imports could generate invalid Postgres DDL in two cases:

  • A timestamp-like TEXT column was converted to TIMESTAMPTZ, but its SQLite strftime(...) default was copied as a string literal.
  • An INTEGER column was converted to BOOLEAN, but its CHECK constraint still compared it with 0 and 1.

This PR translates both patterns into valid Postgres expressions while leaving unrelated columns and defaults alone.

What changed

Date and time defaults

The importer now recognizes common D1 strftime(...) defaults that use current-time values, ISO-8601 or date formats, Unix timestamps, and supported date modifiers.

For example:

DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))

becomes:

DEFAULT date_trunc('second', now())

Only mappings with a known Postgres equivalent are converted. Unsupported typed defaults are omitted instead of producing invalid DDL; TEXT defaults keep the existing literal fallback.

Boolean CHECK constraints

When an SQLite INTEGER column is inferred as a Postgres BOOLEAN, standalone 0 and 1 literals in direct comparisons, IN, and BETWEEN expressions are converted to false and true.

For example:

CHECK (is_active IN (0, 1))

becomes:

CHECK (is_active IN (false, true))

The rewrite applies only to columns already selected for boolean coercion. Decimal, exponent, hexadecimal, signed, and arithmetic expressions are left intact rather than partially rewritten.

Testing

  • go test ./internal/import/d1/...
  • go vet ./internal/import/d1/...
  • GitHub build, lint, license, and Bugbot checks

…columns

A DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')) - the common D1/Drizzle
"current timestamp as ISO-8601 text" idiom - fell into the escaped-literal
fallback because mapSQLiteDefaultFunction had no case for STRFTIME(...),
unlike DATETIME(...)/DATE(...)/TIME(...)/UNIXEPOCH(...). That's harmless
while the column stays TEXT, but once a timestamp-named TEXT column with
timestamp-shaped samples gets promoted to TIMESTAMPTZ, the same escaped
string literal becomes an invalid `timestamp with time zone` default and
Postgres rejects the CREATE TABLE:

  ERROR: invalid input syntax for type timestamp with time zone:
  "(strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))"

Add a STRFTIME(...) case that recognizes a literal 'now' time-value
argument and maps it to now(), mirroring the existing UNIXEPOCH('now')
handling. Only applies when pgType is TIMESTAMPTZ; any other target type
(format argument ignored, other time-value arguments) falls through to the
existing safe literal-quoting fallback unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017hXnh4m7na8ee5FoRrHGo3
@claude
claude Bot requested a review from a team as a code owner July 22, 2026 13:57
…iterals

Broadens the strftime()-default mapping beyond the literal 'now' time-value:
recognizes bare CURRENT_TIMESTAMP/CURRENT_TIME/CURRENT_DATE as time-values,
and applies safe trailing modifiers (±N day/hour/minute/second/month/year,
"start of day/month/year", and no-op "localtime"/"utc") on TIMESTAMPTZ
columns. Adds a %s-format mapping to extract(epoch from ...) for
numeric-typed columns. Any time-value or modifier this can't soundly
translate falls back to the existing safe behavior instead of guessing:
TEXT/other targets get the escaped-literal default as before, while a
TIMESTAMPTZ target with an unrecognized modifier drops just that modifier
and keeps the correctly identified base time-value (dropping is preferred
over the literal fallback there, since that literal is exactly the
"invalid input syntax for type timestamp with time zone" bug this exists
to fix).

Also fixes boolean CHECK constraints being left as 0/1 integer comparisons
after their column is coerced to Postgres BOOLEAN (e.g.
CHECK (is_active IN (0,1)) against a coerced boolean column, which
Postgres rejects). Threads the existing TypeCoercionContext into the
CHECK-constraint conversion path and rewrites literal 0/1 comparisons
(=, <>, !=, IN, and reversed operand order) against boolean-coerced columns
to false/true, leaving non-boolean columns untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017hXnh4m7na8ee5FoRrHGo3
@claude claude Bot changed the title d1 import: map strftime(fmt, 'now') defaults on inferred TIMESTAMPTZ columns d1 import: broaden strftime() date/time coverage, fix boolean CHECK literals Jul 22, 2026
Comment thread internal/import/d1/convert.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2d71548. Configure here.

Comment thread internal/import/d1/convert.go
Comment thread internal/import/d1/convert.go
Comment thread internal/import/d1/constraints.go Outdated
…ime no-ops

Two Cursor Bugbot findings on PR #1299 were still present:

- rewriteBooleanCheckLiterals used \b immediately after the "0"/"1" token when
  rewriting boolean-coerced CHECK literals to true/false. \b only asserts a
  word/non-word transition, which still holds between a digit and a following
  ".", so `"is_active" = 0.0` and `BETWEEN 0 AND 1.0` had only their integer
  prefix rewritten, producing invalid Postgres like `"is_active" = false.0`.
  Reworked the matching to capture the full numeric token (integer, decimal,
  or exponent form) and only rewrite it when it is exactly "0" or "1",
  otherwise leaving the literal untouched.

- applyStrftimeModifiers treated an unrecognized 'utc'/'localtime' strftime
  modifier as unmappable, aborting the whole DEFAULT conversion and dropping
  the DEFAULT clause on inferred TIMESTAMPTZ columns. Both modifiers are
  no-ops for a TIMESTAMPTZ, which stores an absolute instant regardless of
  the display-timezone modifier SQLite would otherwise apply when rendering
  the formatted string.

The third finding (strftime argument splitting closing a quoted argument on
any single quote without treating SQL's doubled '' escape specially) does not
reproduce against the current splitTopLevelArgs: toggling on every quote
character is mathematically equivalent to proper '' escape handling (verified
by fuzzing 200k random inputs against a reference lookahead-based splitter
with zero mismatches), so no change was needed there. Added a regression test
locking in the correct behavior anyway.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017hXnh4m7na8ee5FoRrHGo3
@no-itsbackpack
no-itsbackpack merged commit de54c9a into main Jul 22, 2026
4 checks passed
@no-itsbackpack
no-itsbackpack deleted the fix/d1-strftime-now-timestamptz-default branch July 22, 2026 17:42
orware added a commit to sluicesync/sluice that referenced this pull request Jul 22, 2026
…/strftime gaps

Two investigations, both ending in a corrected premise rather than a fix —
recording the ground truth so neither gets re-derived.

ADR-0179 (Discovery, design of record, NOT adopted) — why column-DEFAULT
changes aren't forwarded mid-sync. The cause is STRUCTURAL, not a missing
branch: pgoutput's Relation message carries no defaults, so projectRelation
yields relationColumn{Name, OID, TypeMod, Type, KeyColumn} and a shape
classifier cannot see the change. Records the REVERTED obvious fix (adding
Default to diffAlteredColumn) as both dangerous — seed->CDC compares a
real-defaults `pre` against a nil-defaults `post`, so it would emit DROP
DEFAULT on every table at the first boundary of every sync — and ineffective
(CDC->CDC is nil vs nil, so a real SET DEFAULT is still never seen). Same trap
as Bug 83's missing attnotnull. Generalisable lesson, stated in the ADR: a
shape classifier can only classify what the change stream actually carries.

The known-correct mechanism is credited to supabase/etl (read at 6d21f3f),
which hit the same wall and did NOT use a client-side probe: a ddl_command_end
EVENT TRIGGER reads pg_attribute JOIN pg_attrdef and emits a JSONB catalog
snapshot via TRANSACTIONAL pg_logical_emit_message, so it is ordered in-band
relative to Relation and DML events, and bootstrap + DDL share one schema
builder. A client-side catalog probe is explicitly REJECTED: no privilege
needed, but unorderable — by probe time the source may have moved on and the
observed state has no position in the stream. NOT adopted here because CREATE
EVENT TRIGGER needs superuser, which sluice's managed-provider matrix often
lacks; it could only ever be opt-in with a loud refusal. If ever costed, cost
it as "PG DDL fidelity" — the same payload carries attnotnull (closing Bug
83's compensation) and attidentity.

SQLite CHECK / strftime gaps (from reviewing planetscale/cli#1299) — CONFIRMED
by a failing pin, and the pin corrected the prediction. Expected: PG rejects
`boolean = integer` in the DEFERRED constraint phase, after the copy. Actual:
sluice refuses at CREATE TABLE, before any data moves — better-placed than
assumed, but the migration is blocked outright. Cause is NOT type coercion:
internal/translate/sqlite_expr.go has no IN node, so a portable
`CHECK (col IN (0,1))` cannot be parsed and hits the catch-all non-portable
refusal. Since SQLite has no BOOLEAN type that idiom is canonical, so any
SQLite/D1 database using it currently cannot migrate at all. Gap B (boolean-
coercion-aware CHECK rewriting, PlanetScale's actual fix) is only reachable
after Gap A. The strftime() DEFAULT translation set is filed in the same entry.

New pins cover every operator shape (IN / = / <> / != / reversed), a
non-boolean control column, ENFORCEMENT assertions (a rewritten CHECK must
still reject a bad row — silently relaxing it to a tautology would be worse
than the bug), and the strftime DEFAULT round-trip incl. a defaulted INSERT.
They are t.Skip-gated so main stays green; the skip constant records the exact
pre-fix failure text and states that removing the skip is part of the fix's
definition of done.

Also filed: target-side autovacuum / dead-tuple health during a migration or
CDC catch-up, from reviewing the Hatchet "Postgres survival guide". Most of
that post doesn't apply (single-node OLTP; it omits logical replication/WAL/
slots) and several items are already covered, but autovacuum falling behind
under sustained writes is exactly sluice's bulk-copy and CDC-catch-up shape,
and nothing currently watches n_dead_tup or wraparound headroom on the target.
Proposed as one more rule family in the EXISTING ADR-0107 threshold alerter —
advisory, not a correctness gate.

Docs + skipped tests only; no runtime behaviour changes. Full gate green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
orware added a commit to sluicesync/sluice that referenced this pull request Jul 22, 2026
…types; translate strftime DEFAULTs

Three SQLite/D1→Postgres DDL-fidelity fixes, found by reviewing
planetscale/cli#1299 against sluice and confirmed by a pin written FIRST
(committed skipped in c00ccd4, now un-skipped and green).

1. `CHECK (col IN (0,1))` no longer blocks the migration. SQLite has no
   BOOLEAN type, so an INTEGER column constrained to 0/1 is THE canonical
   idiom — and sluice refused it at create-tables as "non-portable", before
   any data moved. The expression was in fact perfectly portable; the
   translator simply had no IN node, so it could not be parsed and hit the
   catch-all refusal. `x [NOT] IN (a, b, …)` now translates verbatim to both
   PG and MySQL (same syntax, same NULL three-valued logic). Genuinely
   non-portable shapes still refuse: `IN (SELECT …)`, the `IN <table>`
   shorthand, and the SQLite-only empty list `IN ()` (a PG syntax error).

2. --infer-types now re-types those constraints along with the column. A
   column promoted to PG BOOLEAN needs its CHECK re-typed or PG rejects it
   (`operator does not exist: boolean = integer`). Adds a sqEmitCtx threaded
   through every emit + SQLiteExprToPGBoolAware, rewriting 0/1 to false/true
   for `=`, `<>`/`!=`, reversed operands (`1 = flag`), and IN members.
   DELIBERATELY NOT rewritten, because each would be a guess: ordering
   comparisons (`flag > 0` — PG boolean ordering is its own semantic) and
   out-of-range literals (`flag = 2` — never valid against a two-valued
   column) emit unchanged so PG rejects them loudly. MySQL never rewrites:
   its BOOLEAN is TINYINT and takes 0/1 natively. Empty/nil bool set is
   byte-identical to the context-free call — pinned, so no existing call
   site changes behaviour.

3. strftime() DEFAULTs translate instead of dropping. Previously only
   datetime/date/time('now') were recognised and every strftime spelling
   took the loud-WARN drop path, so the target column had NO default and a
   DEFAULT-omitting INSERT after cutover produced NULL. Whole-format
   current-instant spellings now map across with PRECISION PRESERVED —
   they render at second precision while PG's bare now() carries
   microseconds, so the faithful translation is date_trunc('second', now()),
   not now(). `%s` → floor(extract(epoch from now())). Partial formats and
   non-'now' bases still refuse: a general strftime→to_char guess is how a
   DEFAULT silently changes meaning.

Why both 1 and 2 ship together: shipping 1 alone would REGRESS the
--infer-types path. Today it gets a clear sluice refusal; with IN parsing
but no re-typing it would emit `col IN (0,1)` against a BOOLEAN column and
surface a raw PG driver error instead — worse UX on exactly the flag D1
imports use.

Three pre-existing tests asserted the OLD refusing behaviour and were
updated rather than deleted, keeping the refusal coverage: the non-portable
list swaps `x IN (1,2)` for the three IN shapes that genuinely aren't
portable, and the DEFAULT drop tests swap the now-supported strftime
spellings for a partial format, a non-'now' base, and randomblob().

Enforcement is pinned, not just parsing: the integration test asserts a
rewritten CHECK still REJECTS a non-conforming row and ACCEPTS a conforming
one — a constraint silently relaxed to a tautology would be worse than the
original bug — plus a non-boolean control column keeps its integer CHECK,
and the strftime pin does a real defaulted INSERT.

Regression-swept the full SQLite/D1 integration suite (143s, green) plus the
pre-existing --infer-types family matrix. Full gate green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants