ReviewOS

also looking at this

stacks/bun-query-builder

fix(drift): give timezone awareness its own type family

#1178
Merged glennmichael123 wants to merge fix/drift-timestamptz into main
2 files +135 -7
8 days ago

Closes #1176.

auditSchemaDrift compared type families, and naive and aware timestamps collapsed into one. A live timestamptz column under a model declaring datetime reported clean — in both directions, on every dialect.

Before and after, live Postgres 16

Same table, same call, control column included so a clean result cannot be a miswired harness:

casebeforeafter
expires_at declared datetime, live timestamptzclean=trueclean=false
created_at declared timestamptz, live naiveclean=trueclean=false
declared timestamptz, live timestamptzclean=trueclean=true
declared datetime, live naiveclean=trueclean=true
CONTROL amount declared decimal, live integerclean=falseclean=false

The two routes

  1. timestamp with time zone matched the generic /^(?:date|time|timestamp|datetime)\b/ branch, so declared and actual were both date.
  2. The bare timestamptz spelling matched nothing — \b cannot match between timestamp and tz — so it classified as other and left through the unknown-is-not-drift rule.

timestamptz is now its own family, tested before the generic branch and against the whole string so timestamp(6) with time zone still matches. That mirrors normalizePlanType in introspect-db.ts, which already drew this distinction the same way.

Why familyOfSqlType now takes a dialect

Awareness is not a property of the spelling. Postgres timestamp is naive; MySQL's TIMESTAMP is converted to and from the session timezone and DATETIME is the naive one. The same word means opposite things, so a classifier without the dialect gets one of them wrong whichever way it guesses. The parameter is optional and, omitted, reads timestamp as naive — the previous behaviour.

Measured, not assumed — this is what the live servers actually report through introspectDatabase:

Postgres   naive -> "timestamp without time zone"   aware -> "timestamp with time zone"
MySQL      naive -> "datetime"                      aware -> "timestamp"

Two carve-outs, both to avoid firing on schemas bqb itself creates

  • MySQL: drivers/mysql.ts maps both datetime and timestamptz to DATETIME, so a model declaring timestamptz gets a naive column from a correct migration. Flagging that would make every MySQL schema bqb created report drift — the same false positive the boolean/tinyint rule exists to prevent. The opposite direction is still flagged: bqb never emits MySQL TIMESTAMP, so a live one under a naive model came from elsewhere and is the dangerous half. Verified live:

    declared datetime,    live TIMESTAMP (aware)   clean=false   <- caught
    declared timestamptz, live DATETIME (bqb's)    clean=true    <- not a false positive
    CONTROL amount decimal, live int               clean=false
  • SQLite: every date family is stored as TEXT and no session conversion happens, so the distinction cannot arise. timestamptz is added to the existing TEXT carve-out, and date/timestamptz satisfy each other there.

Note on an existing test

reads timestamp with time zone as date asserted the old behaviour, so it encoded the bug. Corrected rather than deleted, and timestamp without time zone added beside it.

Verification

  • Full suite: 2447 pass, 26 skip, 0 fail
  • tsc and pickier clean
  • 8 new tests covering both families, every spelling both servers report, the dialect asymmetry, both carve-outs, and both directions of satisfies
  • Live Postgres 16 and MySQL, each with a control column

The MySQL probe database was created for this and dropped afterwards; no existing database was touched.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.