fix: parse SQL-style datetime strings with a space separator - #99
fix: parse SQL-style datetime strings with a space separator#99roberthovsepyan wants to merge 2 commits into
Conversation
korvin89
left a comment
There was a problem hiding this comment.
🤖 AI generated
This review was done with AI assistance. I validated every comment myself, but the wording — including the points I found myself — was written by the AI.
Severity legend
- 🔴 blocker — should be fixed before merge.
- 🟠 should-fix — a real problem; worth fixing before or right after merge. If you can't get to it now, please open an issue and leave a comment linking it.
- 🔵 nit — minor; author's discretion.
- ❓ question — I need more context; not necessarily a defect.
| ['2023-12-31T01:00', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31T01:00Z', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31T03:00+02:00', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31 01:00', '2023-12-31T01:00:00.000Z'], |
There was a problem hiding this comment.
🤖 AI generated
🟠 should-fix
I don't think these four cases will actually guard the regression on CI. TZ isn't pinned anywhere — jest.config.js has no globalSetup, setup-tests.ts only does import 'dayjs/locale/ru', and .github/workflows/ci.yml runs npm run test on ubuntu-latest, where the runner defaults to UTC.
I checked by reverting regexParse.ts to main and running the new tests against the unfixed parser:
| TZ | failures among the 12 new cases |
|---|---|
UTC (what CI uses) |
1 — only '2016-05-25 09:08:34,123' |
Europe/Moscow |
4 |
That's expected: the bug is that the native Date fallback resolves the string in the system zone, so under UTC it lands on the right answer anyway. The single case that does fail is failing because Date rejects the comma — a syntax accident rather than the timezone bug.
Would you mind pinning a non-UTC zone for the suite?
"test": "TZ=Europe/Moscow jest"(or a globalSetup that sets process.env.TZ). I ran the full 516-test suite under Europe/Moscow and America/New_York and it stays green, so this looks safe. Pinning UTC would have the opposite effect here.
| ['2023-12-31 01:00', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31 01:00:00', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31 01:00:00.000000', '2023-12-31T01:00:00.000Z'], | ||
| ['2023-12-31 03:00:00+02:00', '2023-12-31T01:00:00.000Z'], |
There was a problem hiding this comment.
🤖 AI generated
🔵 nit
Worth noting that this case can't guard the regression — and no offset-bearing case can. The native Date fallback already honours an explicit offset, so it returns the same instant with or without the fix: I checked +02, +0200 and +02:00 under both UTC and Europe/Moscow, and all six combinations give 2023-12-31T01:00:00.000Z. The three zoneless cases above are the ones doing the real work, once TZ is pinned (see my other comment).
That's fine — it still earns its place as format-acceptance coverage. Since that's its job, though, maybe it could cover a shape that isn't covered yet? +02:00 is already asserted for the ISO path on line 49, whereas the short form Postgres actually emits isn't asserted anywhere:
['2023-12-31 03:00:00+02', '2023-12-31T01:00:00.000Z'],Per the Postgres docs, the offset is printed as hh when it's a whole number of hours. offsetRegex makes the minutes optional, so it parses correctly today — it's just untested.
| ); | ||
| const isoTimeFullRegex = new RegExp(`^${isoTimeRegex.source}$`); | ||
|
|
||
| // SQL-style datetime: YYYY-MM-DD hh:mm:ss.sss±00:00 (a space instead of T) |
There was a problem hiding this comment.
🤖 AI generated
🔵 nit
The comment promises a narrower grammar than the pattern implements. Because isoTimeRegex is interpolated whole, this also accepts hh, hhmm, hhmmss, a comma as the decimal separator, 1–30 fraction digits, ±hh, ±hhmm, and a trailing [Area/Zone]. Expanded, line 88 comes out as:
^(\d{4})-(\d\d)-(\d\d) (\d\d)(?::?(\d\d)(?::?(\d\d)(?:[.,](\d{1,30}))?)?)?(?:(?:(Z)|([+-]\d\d)(?::?(\d\d))?)?(?:\[([A-Za-z_+-]{1,256}(?::?\/[A-Za-z0-9_+-]{1,256}(?:\/[A-Za-z0-9_+-]{1,256})?)?)\])?)?$
The same goes for the DateTime from SQL test-block name below — "SQL" promises something narrower than what's implemented. '2016-05-25 09:08:34,123' in that block is a nice illustration: the comma comes from ISO 8601, not SQL (RFC 3339 has time-secfrac = "." 1*DIGIT, the SQL BNF has <period>). The behaviour is correct; it's the label that over-promises.
Could we say it plainly in both places? Something like:
// Datetime with a space instead of T — the shape SQL databases emit
// (ClickHouse DateTime64, Postgres timestamptz, MySQL DATETIME).
// Grammar: YYYY-MM-DD + the ISO time part, so compact/partial forms,
// a comma fraction and a trailing [IANA] zone are accepted too.
// https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-OUTPUTOn the link choice: RFC 3339 §5.6 only permits the space in a prose NOTE — its own ABNF is date-time = full-date "T" full-time — and the SQL grammar the name refers to lives in ISO/IEC 9075-2, which has no stable free URL. The Postgres page is the one source that's free, stable and bridges both: it says outright that it emits a space "for readability and for consistency with RFC 3339".
| ]).toEqual(expected); | ||
| }); | ||
|
|
||
| test.each<[string, [number, number, number, number, number, number, number]]>([ |
There was a problem hiding this comment.
🤖 AI generated
🔵 nit
Would it be worth adding a few boundary cases? Several forms flipped from isValid() === false to true through the public API, and nothing names them:
['2016-05-25 09', ...],
['2016-05-25 0908', ...],
['2016-05-25 090834.123', ...],
['2016-05-25 09:08:34[Europe/Paris]', ...],The ISO block above does exactly this for its own grammar — lines 50–60 spell out T09, T0908, T090834 — so it'd be nice for the SQL block to read the same way.
The bracketed one is the case I'd most like to see covered. grep for [Europe/, [America/, [Asia/ across src/ returns nothing, so that branch is untested for the ISO path too — and it carries a capture group that extractISOYmdTimeAndOffset's cursor arithmetic depends on. It's not an exotic shape either: Temporal accepts '2016-05-25 09:08:34[Europe/Paris]' (I checked on Node 24 with --harmony-temporal), and RFC 9557 standardises the bracket notation — so it seems worth locking in rather than leaving implicit.
Whichever way you want these to behave is fine by me; the value is in having a test that says so.
| ); | ||
| } | ||
|
|
||
| export function parseSQLDate(s: string) { |
There was a problem hiding this comment.
// ISO 8601 specifies the use of uppercase letter T to separate the date and time. PostgreSQL accepts that format on input, but on output it uses a space rather than T
// In the ISO style, the time zone is always shown as a signed numeric offset from UTC.
// However, PostgreSQL accepts the full time zone name as input, not enclosed in square brackets and separated from the time by a space.
// https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-OUTPUT
const sqlYmdRegex = /(\d{4})-(\d\d)-(\d\d)/;
const sqlTimeRegex = RegExp(
`${isoTimeBaseRegex.source}(?:${offsetRegex.source}| (${ianaRegex.source}))?`,
);
const sqlYmdWithTimeExtensionRegex = new RegExp(`^${sqlYmdRegex.source} ${sqlTimeRegex.source}$`);
const sqlTimeFullRegex = new RegExp(`^${sqlTimeRegex.source}$`);
export function parseSQLDate(s: string) {
return parse(
s,
[sqlYmdWithTimeExtensionRegex, extractISOYmdTimeAndOffset],
[sqlTimeFullRegex, extractISOTimeAndOffset],
);
}or
// ISO 8601 specifies the use of uppercase letter T to separate the date and time. PostgreSQL accepts that format on input, but on output it uses a space rather than T
// In the ISO style, the time zone is always shown as a signed numeric offset from UTC.
// https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-OUTPUT
const sqlYmdRegex = /(\d{4})-(\d\d)-(\d\d)/;
const sqlTimeRegex = RegExp(`${isoTimeBaseRegex.source}(?:${offsetRegex.source})?`);
const sqlYmdWithTimeExtensionRegex = new RegExp(`^${sqlYmdRegex.source} ${sqlTimeRegex.source}$`);
export function parseSQLDate(s: string) {
return parse(s, [sqlYmdWithTimeExtensionRegex, extractISOYmdTimeAndOffset]);
}
Strings like "2026-08-31 09:02:42.000000" did not match any regex in parseDateString and fell back to the native Date parser, which reads them in the system time zone and ignores the requested one, so dateTimeUtc() returned a value shifted by the local UTC offset.
Added a SQL-style pattern (YYYY-MM-DD hh:mm:ss.sss±hh:mm) next to the ISO one