Skip to content

Commit 9774b78

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): Field.time canonical storage — HH:MM:SS[.fff] wall-clock text on every dialect (#3994) (#4020)
* feat(spec,ci): temporal hooks onto the IDataDriver contract; conformance job with live non-UTC servers Closes the two debts the datetime storage work (#3912/#3942/#3954) left open in ADR-0053. D-A2 — contract promotion. temporalFilterValue and temporalFilterColumnSql were duck-typed: analytics probed `typeof driver.x === 'function'` against a locally-invented interface, and nothing at the type level said a driver must implement both or neither. #3912's lesson is precisely that coercing the comparand without normalising the column reintroduces half the bug, so a driver implementing one hook alone would silently regress. Both are now optional members of IDataDriver, documented as a PAIR with "absent = identity" semantics for drivers whose storage form is the wire form. SqlDriver `implements IDataDriver`, so its signatures are compile-checked from here on; analytics Picks the contract instead of inventing a local shape, keeping the runtime typeof guards as the correct way to consume an optional member. coerceFilterValueForSql already sits behind the hook as the last-resort boolean/number recovery — the demotion D-A2 asked for — and stays in exactly that role. ADR-0053 records the resolution. D-A3 — the conformance backstop. The live-server suites from #3912/#3942 are opt-in (OS_TEST_POSTGRES_URL / OS_TEST_MYSQL_URL) and skip without a server, so nothing ran them in CI and the seam could regress silently. New `temporal-conformance` job: postgres:16 and mysql:8.0 service containers, servers pointed at +08:00 post-start (services cannot override the image command), the Node process at America/New_York, and assertions in UTC — the three-way skew that caught every bug in this family, with both suites asserting a non-UTC server so a mis-provisioned service fails loudly rather than passing vacuously. The WHOLE driver-sql suite runs under the skewed zone, so a TZ-sensitive assumption anywhere in the driver's tests fails here before it ships. mysql:8.0 also covers the other half of the #3942 compatibility claim — the hands-on verification ran on MariaDB 10.11, the stricter dialect. Verified locally with the exact CI command against live PG 16 (Asia/Shanghai) and MariaDB (+08:00): 47 files, 480/480, zero skips. Full pnpm build + test green (132 tasks); spec doc gates green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ * fix(driver-sql): Field.time gets a canonical storage form — HH:MM:SS[.fff] wall-clock text on every dialect (#3994) Field.time repeated the pre-#3912 Field.datetime pattern: writes were never normalised, only reads were repaired, so one SQLite column accumulated bare time-of-day TEXT, full-timestamp TEXT and INTEGER epoch ms side by side. find() looked right; everything comparing the STORED form was wrong — measured: a business-hours window filter silently dropped 4 of 7 rows, ORDER BY sorted 14:30 before 08:00, a full-ISO write failed the statement on both Postgres and MySQL, a bound Date stored a process-timezone wall clock on pg, MySQL's bare TIME rounded fractional seconds, and NOW() defaults read three different clocks across the three dialects. The #3912#3942#3954 construction, transplanted (ADR-0053 D-C1..D-C3): - One canonicalTimeOfDay — HH:MM:SS, .fff only when non-zero; Date/epoch/ full-timestamp fold to the UTC time-of-day — applied on write (formatInput), to filter comparands (coerceFilterValue → temporalFilterValue) and on read (toTimeOnly). - SQLite: backfillCanonicalTimes converges legacy columns at schema sync (IS NOT-guarded UPDATE, log-and-swallow); until then filters wrap the column in sqliteCanonicalTimeSql — correct, just unindexed. os migrate plan lists the work as normalize_time_storage with a row count. - MySQL: new time columns are TIME(3); legacy TIME(0) widens at schema sync (migrateMysqlTimeColumns, plan kind widen_time_columns) since zero-precision TIME rounds fractional writes. - NOW() defaults read the UTC clock on every dialect; MySQL 8.0 rejects a plain CURRENT_TIMESTAMP default on TIME entirely, so the expression default is also a compatibility fix. - distinct()/aggregate() present time columns exactly as find() does (ReadPresentationKind gains 'time'). HH:MM:SS writes round-trip byte-identically (field-zoo f_time, #2022); a minutes-only HH:MM completes to HH:MM:00; uninterpretable values pass through untouched. Verified on live servers: SQLite, PG 16 at Asia/Shanghai, MariaDB 10.11 at +08:00, Node at America/New_York — 505/505 driver tests with zero skips. Closes #3994 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f5a4ef0 commit 9774b78

11 files changed

Lines changed: 1084 additions & 81 deletions
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
"@objectstack/driver-sql": minor
3+
"@objectstack/cli": patch
4+
"@objectstack/spec": patch
5+
---
6+
7+
fix(driver-sql): `Field.time` gets a canonical storage form — `HH:MM:SS[.fff]` wall-clock text on every dialect (#3994)
8+
9+
`Field.time` repeated the pre-#3912 `Field.datetime` pattern: writes were never
10+
normalised and only reads were repaired, so one SQLite column accumulated bare
11+
time-of-day TEXT, full-timestamp TEXT and INTEGER epoch ms side by side.
12+
`find()` looked right; everything that compared the STORED form was wrong —
13+
measured: a business-hours window filter silently dropped 4 of 7 rows, ORDER BY
14+
sorted 14:30 before 08:00, a full-ISO write failed the statement outright on
15+
both Postgres and MySQL, a bound `Date` stored a process-timezone wall clock on
16+
pg, MySQL's bare `TIME` rounded `…00.500` up to `…01`, and a `NOW()` default
17+
resolved against three different clocks on the three dialects.
18+
19+
The #3912#3942#3954 construction, transplanted (ADR-0053 D-C1..D-C3):
20+
21+
- One `canonicalTimeOfDay``HH:MM:SS`, `.fff` only when non-zero; `Date`/
22+
epoch/full-timestamp fold to the UTC time-of-day — applied on write
23+
(`formatInput`), to filter comparands (`coerceFilterValue`, and thereby the
24+
`temporalFilterValue` contract hook) and on read (`toTimeOnly`).
25+
- SQLite: legacy columns converge at schema sync (`backfillCanonicalTimes`,
26+
same `IS NOT`-guarded UPDATE, same log-and-swallow policy); until then the
27+
filter paths wrap the column in the repair expression — correct, just
28+
unindexed. `os migrate plan` lists the work as `normalize_time_storage` with
29+
a row count.
30+
- MySQL: new time columns are `TIME(3)`; legacy `TIME(0)` columns widen at
31+
schema sync (`migrateMysqlTimeColumns`, plan kind `widen_time_columns`),
32+
since zero-precision TIME *rounds* fractional writes.
33+
- `NOW()` defaults read the UTC clock on every dialect (Postgres previously
34+
used the server zone, MySQL the inserting session's zone — and MySQL 8.0
35+
rejects a plain `CURRENT_TIMESTAMP` default on TIME entirely).
36+
- `distinct()`/`aggregate()` present time columns exactly as `find()` does.
37+
38+
`HH:MM:SS` writes round-trip byte-identically (the field-zoo `f_time`
39+
contract); a minutes-only `HH:MM` now completes to `HH:MM:00`, and uninterpretable
40+
values still pass through untouched.

docs/adr/0053-date-and-datetime-semantics.md

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# ADR-0053: `date` is a timezone-naive calendar day; `datetime` is an instant rendered in a reference timezone
22

3-
**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 resolved 2026-07-30: `temporalFilterValue` + `temporalFilterColumnSql` are optional `IDataDriver` contract members with identity semantics, and analytics types its driver seam from the contract. **Partly superseded (2026-07-29, addendum D-B1..D-B4):** Phase 1's "`Field.datetime` stays stored as UTC epoch ms" is replaced by one canonical UTC instant per dialect — `YYYY-MM-DDTHH:MM:SS.sssZ` text on SQLite, `timestamptz` on Postgres, `DATETIME(3)` on MySQL — applied on write and to filter comparands alike (#3912, #3942).
3+
**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 resolved 2026-07-30: `temporalFilterValue` + `temporalFilterColumnSql` are optional `IDataDriver` contract members with identity semantics, and analytics types its driver seam from the contract. **Partly superseded (2026-07-29, addendum D-B1..D-B4):** Phase 1's "`Field.datetime` stays stored as UTC epoch ms" is replaced by one canonical UTC instant per dialect — `YYYY-MM-DDTHH:MM:SS.sssZ` text on SQLite, `timestamptz` on Postgres, `DATETIME(3)` on MySQL — applied on write and to filter comparands alike (#3912, #3942). **Extended (2026-07-30, addendum D-C1..D-C3):** `Field.time` takes the same construction — canonical `HH:MM:SS[.fff]` wall-clock text, one function on write/filter/read, `TIME(3)` on MySQL, UTC `NOW()` defaults on every dialect (#3994).
44
**Deciders**: ObjectStack Protocol Architects
55
**Builds on**: [ADR-0032](./0032-unified-expression-layer.md) (unified expression layer — CEL dialect, `today()`/`daysFromNow()`), [ADR-0014](./0014-record-form-field-type.md) (field types)
66
**Consumers**: `@objectstack/spec` (`Field.date`/`Field.datetime`), `@objectstack/driver-sql` (`coerceFilterValue`, `formatInput`/`formatOutput`, `dateFields`/`datetimeFields`), `@objectstack/formula` (`stdlib` time functions, `cel-engine` hydration), `@objectstack/objectql` (`applyFormulaPlan`), schedule/cron executors, report/analytics date bucketing, `sys-user-preference.timezone`.
@@ -574,3 +574,78 @@ The same caveat as Postgres applies and is worth stating plainly: the migration
574574
Regression cover is `sql-driver-datetime-mysql-storage.test.ts`, opt-in via
575575
`OS_TEST_MYSQL_URL` (CI provisions no server), asserting a non-UTC server so it
576576
cannot pass vacuously. 10 of its 13 cases fail without this change.
577+
578+
---
579+
580+
## Addendum (2026-07-30) — `Field.time` gets the same storage convention (D-C1..D-C3, #3994)
581+
582+
`Field.time` was the last temporal field type with **no storage convention at
583+
all** — the exact meta-problem #3912 exposed for `Field.datetime`. `formatInput`
584+
never touched it, `coerceFilterValue` returned its comparands unchanged, and the
585+
read paths papered over the drift so well (`toTimeOnly`) that `find()` always
586+
looked right. Measured on real servers (SQLite; PG 16 at `Asia/Shanghai`;
587+
MariaDB 10.11 / MySQL 8.0 at `+08:00`; Node at `America/New_York`), the same
588+
failure family followed: a business-hours window filter silently dropped 4 of 7
589+
rows on SQLite (INTEGER epoch rows fail `>= '09:00'` because `INTEGER < TEXT`;
590+
full-timestamp text fails `<= '18:00'` lexicographically), ORDER BY put 14:30
591+
before 08:00, a full-ISO write failed the statement outright on PG **and**
592+
MySQL, a bound `Date` stored a process-timezone wall clock on pg, MySQL's bare
593+
`TIME` rounded `…00.500` up to `…01`, and a `NOW()` default resolved against
594+
three different clocks on the three dialects.
595+
596+
### D-C1 — The canonical form is `HH:MM:SS`, `.fff` only when non-zero
597+
598+
A `Field.time` is a **timezone-naive wall-clock time-of-day** (#2004), so the
599+
canonical text carries no zone: `HH:MM:SS`, extended to `HH:MM:SS.fff` exactly
600+
when the milliseconds are non-zero. One function (`canonicalTimeOfDay`) produces
601+
it and is applied on write (`formatInput`), to filter comparands
602+
(`coerceFilterValue`/`temporalFilterValue`) and on read (`toTimeOnly`), the
603+
D-B1 construction transplanted.
604+
605+
Why variable-width rather than datetime's fixed `.sss`: `.` sorts below every
606+
digit, so lexicographic order is still chronological order across mixed widths
607+
(`'14:30:00.100' < '14:30:01'`), and the zero-millisecond spelling `HH:MM:SS`
608+
is what every dialect's native TIME emits — the field-zoo `f_time` round-trip
609+
(#2022) keeps holding byte-for-byte. Determinism is what matters for equality
610+
and `distinct()`, and each wall clock has exactly one spelling. A minutes-only
611+
`'14:30'` completes to `'14:30:00'`.
612+
613+
A `Date` / epoch-ms / full-timestamp value folds to its **UTC** time-of-day —
614+
matching the platform's instant semantics, the SQLite read repair's historical
615+
behaviour, and `nowColumnDefault`; never the process or server timezone.
616+
Uninterpretable values (including out-of-range wall clocks like `'25:00'`) pass
617+
through untouched.
618+
619+
### D-C2 — Physical form per dialect
620+
621+
- **SQLite**: canonical TEXT (no native type). Legacy columns converge at
622+
schema sync via `backfillCanonicalTimes` — one `UPDATE` per column whose SET
623+
expression IS the read-repair SQL (`sqliteCanonicalTimeSql`), `IS NOT` guard,
624+
log-and-swallow failure policy, `os migrate plan` row-counted entry
625+
(`normalize_time_storage`) — D-B3 verbatim. Until it runs, filters wrap the
626+
column in the repair expression: correct, just unindexed.
627+
- **Postgres**: native `time` (µs precision). The canonical literal binds
628+
unambiguously; the pre-fix failures were the *input* shapes, not the column.
629+
- **MySQL**: `TIME(3)` — the `DATETIME(3)` precedent applied: bare `TIME` is
630+
zero-precision and **rounds** fractional literals, changing the stored wall
631+
clock. Legacy `TIME(0)` columns widen at schema sync
632+
(`migrateMysqlTimeColumns`, plan kind `widen_time_columns`), restating a
633+
`NOW()` expression default where declared.
634+
635+
### D-C3 — `NOW()` defaults are the UTC wall clock on every dialect
636+
637+
`knex.fn.now()` compiles to `CURRENT_TIMESTAMP`, which a TIME column resolves
638+
in the **server's** zone on Postgres and the **inserting session's** zone on
639+
MySQL (so the same column's default depended on who inserted). MySQL 8.0
640+
additionally rejects a plain `CURRENT_TIMESTAMP` default on TIME. The defaults
641+
are now expression-spelled per dialect — `strftime('%H:%M:%f','now')` with a
642+
canonical `.000` trim on SQLite, `timezone('utc', now())::time(3)` on Postgres,
643+
`(cast(utc_timestamp(3) as time(3)))` on MySQL (8.0.13+/MariaDB 10.2+ for the
644+
expression-default syntax) — all reading the UTC clock.
645+
646+
The D-B3 caveat applies unchanged: the backfill converges *shapes*; a wall
647+
clock the old path never recorded correctly (a pg `Date` bind serialised in the
648+
host's zone) is not recoverable. Regression cover:
649+
`sql-driver-time-canonical-storage.test.ts`, `sql-driver-time-of-day.test.ts`
650+
(SQLite), `sql-driver-time-live-dialects.test.ts` (live PG + MySQL, in the CI
651+
temporal-conformance job's non-UTC matrix).

packages/cli/src/utils/schema-migrate.pending-render.test.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ const ADDITIVE: PendingSchemaWork[] = [
4141
const IN_PLACE: PendingSchemaWork[] = [
4242
{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'], rows: 1234567 },
4343
{ table: 'legacy', kind: 'widen_datetime_columns', columns: ['at', 'created_at'], rows: 42 },
44+
{ table: 'shift', kind: 'normalize_time_storage', columns: ['starts_at'], rows: 7 },
45+
{ table: 'shift_my', kind: 'widen_time_columns', columns: ['starts_at'], rows: 9 },
4446
];
4547

4648
describe('renderPendingSchemaWork (#3954)', () => {
@@ -74,6 +76,11 @@ describe('renderPendingSchemaWork (#3954)', () => {
7476
expect(out()).toContain('widen_datetime_columns: at, created_at');
7577
// A MySQL widen is ALTER … MODIFY — a rebuild, said outright.
7678
expect(out()).toContain('42 row table rebuild');
79+
// The Field.time twins (#3994) take the same rendering rules.
80+
expect(out()).toContain('normalize_time_storage: starts_at');
81+
expect(out()).toContain('7 row update(s)');
82+
expect(out()).toContain('widen_time_columns: starts_at');
83+
expect(out()).toContain('9 row table rebuild');
7784
});
7885

7986
it('shows both sections when both kinds are pending', () => {
@@ -102,7 +109,7 @@ describe('summarizePendingSchemaWork (#3954)', () => {
102109
const summary = summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]);
103110
expect(summary).toContain('1 table(s) to create');
104111
expect(summary).toContain('1 column(s) to add');
105-
expect(summary).toContain('3 datetime column(s) to converge in place');
106-
expect(summary).toContain('~1,234,609 rows');
112+
expect(summary).toContain('5 temporal column(s) to converge in place');
113+
expect(summary).toContain('~1,234,625 rows');
107114
});
108115
});

packages/cli/src/utils/schema-migrate.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -285,17 +285,14 @@ export function renderPendingSchemaWork(pending: PendingSchemaWork[]): void {
285285
if (inPlace.length > 0) {
286286
console.log(` ${chalk.bold('In place (existing rows converged when you apply)')}`);
287287
for (const p of inPlace) {
288-
const label = p.kind === 'normalize_datetime_storage'
289-
? 'normalize_datetime_storage'
290-
: 'widen_datetime_columns';
291288
// A MySQL widen is `ALTER … MODIFY`, i.e. a full table rebuild holding a
292289
// metadata lock — worth saying outright, not just implying via the count.
293-
const cost = p.kind === 'widen_datetime_columns'
290+
const cost = p.kind === 'widen_datetime_columns' || p.kind === 'widen_time_columns'
294291
? `${formatRows(p.rows)} row table rebuild`
295292
: `${formatRows(p.rows)} row update(s)`;
296293
console.log(
297294
` ${chalk.yellow('~')} ${chalk.yellow(p.table)} ` +
298-
`${chalk.dim(`[${label}: ${p.columns.join(', ')}${cost}]`)}`,
295+
`${chalk.dim(`[${p.kind}: ${p.columns.join(', ')}${cost}]`)}`,
299296
);
300297
}
301298
console.log('');
@@ -320,7 +317,7 @@ export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string
320317
if (inPlace.length > 0) {
321318
const cols = inPlace.reduce((n, p) => n + p.columns.length, 0);
322319
const rows = inPlace.reduce((n, p) => n + (p.rows ?? 0), 0);
323-
parts.push(`${cols} datetime column(s) to converge in place (~${formatRows(rows)} rows)`);
320+
parts.push(`${cols} temporal column(s) to converge in place (~${formatRows(rows)} rows)`);
324321
}
325322
return parts.join(', ');
326323
}

packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,24 @@ export class LegacyStorageDriver extends SqlDriver {
3939
this.canonicalDatetimeFields[table]?.delete(field);
4040
}
4141

42+
/**
43+
* The `Field.time` twin (#3994): insert raw pre-canonical time values and
44+
* clear the column's canonical marker.
45+
*/
46+
async seedLegacyTimeRows(
47+
table: string,
48+
field: string,
49+
rows: Array<Record<string, unknown>>,
50+
): Promise<void> {
51+
await this.knex(table).insert(rows);
52+
this.forgetCanonicalTime(table, field);
53+
}
54+
55+
/** Drop the "already backfilled" marker for one `Field.time` column. */
56+
forgetCanonicalTime(table: string, field: string): void {
57+
this.canonicalTimeFields[table]?.delete(field);
58+
}
59+
4260
/** Raw stored form of a column, for asserting on the fixture's premise. */
4361
async storedForms(table: string, field: string): Promise<Array<{ id: string; type: string; value: unknown }>> {
4462
const res: any = await this.knex.raw(

packages/plugins/driver-sql/src/schema-drift.ts

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -133,23 +133,26 @@ export interface PendingSchemaWork {
133133
/**
134134
* What kind of physical work a {@link PendingSchemaWork} entry represents.
135135
*
136-
* The first two are purely additive and never touch existing rows. The datetime
137-
* pair is NOT: `normalize_datetime_storage` rewrites rows in place (the SQLite
138-
* canonical-UTC backfill) and `widen_datetime_columns` rebuilds a column (the
139-
* MySQL `TIMESTAMP` → `DATETIME(3)` widening) — both from #3912/#3942. They are
140-
* rendered under their own heading for that reason: the additive section tells
141-
* the operator the work is never data-losing, and that claim must not silently
142-
* come to cover a row rewrite.
136+
* The first two are purely additive and never touch existing rows. The rest are
137+
* NOT: `normalize_datetime_storage` / `normalize_time_storage` rewrite rows in
138+
* place (the SQLite canonical-text backfills, #3912/#3994) and
139+
* `widen_datetime_columns` / `widen_time_columns` rebuild a column (the MySQL
140+
* `TIMESTAMP` → `DATETIME(3)` and `TIME` → `TIME(3)` widenings, #3942/#3994).
141+
* They are rendered under their own heading for that reason: the additive
142+
* section tells the operator the work is never data-losing, and that claim must
143+
* not silently come to cover a row rewrite.
143144
*/
144145
export type PendingSchemaWorkKind =
145146
| 'create_table'
146147
| 'add_columns'
147148
| 'normalize_datetime_storage'
148-
| 'widen_datetime_columns';
149+
| 'normalize_time_storage'
150+
| 'widen_datetime_columns'
151+
| 'widen_time_columns';
149152

150153
/** True for the kinds that rewrite or rebuild existing data rather than adding to it. */
151154
export function isInPlaceSchemaWork(kind: PendingSchemaWorkKind): boolean {
152-
return kind === 'normalize_datetime_storage' || kind === 'widen_datetime_columns';
155+
return kind !== 'create_table' && kind !== 'add_columns';
153156
}
154157

155158
/** Ops that act on an index rather than a column — reconciled without a table rebuild. */

0 commit comments

Comments
 (0)