Skip to content

fix: parse SQL-style datetime strings with a space separator - #99

Open
roberthovsepyan wants to merge 2 commits into
mainfrom
CHARTS-13670-fix
Open

fix: parse SQL-style datetime strings with a space separator#99
roberthovsepyan wants to merge 2 commits into
mainfrom
CHARTS-13670-fix

Conversation

@roberthovsepyan

Copy link
Copy Markdown

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

@korvin89 korvin89 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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'],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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.

Comment thread src/dateTime/dateTimeUtc.test.ts Outdated
['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'],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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.

Comment thread src/dateTime/regexParse.ts Outdated
);
const isoTimeFullRegex = new RegExp(`^${isoTimeRegex.source}$`);

// SQL-style datetime: YYYY-MM-DD hh:mm:ss.sss±00:00 (a space instead of T)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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-OUTPUT

On 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]]>([

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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.

Comment thread src/dateTime/regexParse.ts
);
}

export function parseSQLDate(s: string) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

// 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]);
}

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.

3 participants