d1 import: broaden strftime() date/time coverage, fix boolean CHECK literals#1299
Merged
Merged
Conversation
…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
…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
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ 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.
…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
approved these changes
Jul 22, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Requested by Elom Gomez · Slack thread
Summary
D1 imports could generate invalid Postgres DDL in two cases:
TEXTcolumn was converted toTIMESTAMPTZ, but its SQLitestrftime(...)default was copied as a string literal.INTEGERcolumn was converted toBOOLEAN, but itsCHECKconstraint still compared it with0and1.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:
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;
TEXTdefaults keep the existing literal fallback.Boolean
CHECKconstraintsWhen an SQLite
INTEGERcolumn is inferred as a PostgresBOOLEAN, standalone0and1literals in direct comparisons,IN, andBETWEENexpressions are converted tofalseandtrue.For example:
becomes:
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/...