sql: replace the datetime text parser's two bool parameters with enums - #39153
Conversation
parse() took allow_date_only and allow_t_separator as bools, and both of
its callers passed bare true/false. Each flag is now a two-variant enum
(TimePart::{Optional, Required}, Separator::{Space, SpaceOrT}) so the
call sites say which form they accept. No behavior change.
Drops the corresponding bare_bool_args entry from mordant-baseline.toml.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. The diff is the enum swap in CI: every build lane compiled the change. The test failures on build 98405 are unrelated to it: One open question, from the details block in the description: a full baseline regeneration also drops two unrelated entries ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Verified the enum mapping matches the old bools exactly:
TimePart::Optional/Required↔allow_date_onlytrue/false,Separator::SpaceOrT/Space↔allow_t_separatortrue/false. - Confirmed the two
matcharms insideparseare logically equivalent to the priorif allow_date_onlyand|| (allow_t_separator && ...)expressions. parseis module-private and its only two callers (parse_mysql,parse_postgres_timestamp) are updated in this diff; the new enums are also private.
Extended reasoning...
Overview
This PR touches two files: src/sql_jsc/shared/datetime_text.rs (replaces two bool parameters on the private parse helper with two-variant enums TimePart and Separator) and mordant-baseline.toml (drops the corresponding bare_bool_args lint baseline entry). The change is a readability refactor to satisfy an advisory lint; the accepted input set for both MySQL and Postgres timestamp text is unchanged.
Security risks
None. This is a private parsing helper for datetime text strings received from database servers over the text protocol. No new inputs are accepted, no bounds checks are relaxed, and the enums are module-private. The refactor does not touch any auth, crypto, or permission code.
Level of scrutiny
Low. The diff is a 1:1 mechanical translation: allow_date_only=true → TimePart::Optional (the 10-byte form returns Some), false → TimePart::Required (returns None); allow_t_separator=true → Separator::SpaceOrT, false → Separator::Space. I traced both match sites against the original boolean expressions and they are equivalent for every combination. parse is not pub, so the two in-file callers are the complete set — no other call sites to update.
Other factors
The PR description documents that the existing datetime round-trip tests pass with the change, and the mordant lint no longer fires with the baseline entry removed. The baseline edit removes exactly the one line for this file and leaves the rest of [bun_sql_jsc] intact. No outstanding reviewer comments on the timeline. This is well within the "simple, mechanical, following established patterns" bar for approval.
|
Thanks for the review. One note on tests, since the diff has none of its own: the change is type-level in a private function and accepts exactly the inputs it did before, so no test can tell the old code from the new. The existing text-protocol coverage exercises both settings of each flag and passes with this change: |
There was a problem hiding this comment.
LGTM — mechanical bool→enum swap with identical semantics.
What was reviewed:
- Traced both call sites:
true,true→Optional,SpaceOrTandfalse,false→Required,Spacemap to the same branches in bothmatcharms as the old boolean checks. - Confirmed
parse,TimePart, andSeparatorare all file-private; the two public wrappers (parse_mysql,parse_postgres_timestamp) keep their signatures. - The removed
mordant-baseline.tomlline matches the fixed finding; leaving[bun_sql_jsc]empty is valid TOML.
Extended reasoning...
Overview
This PR replaces two bool parameters on the private parse function in src/sql_jsc/shared/datetime_text.rs with two file-private two-variant enums (TimePart::{Optional,Required}, Separator::{Space,SpaceOrT}), updates the two in-file callers, and deletes the corresponding bare_bool_args entry from mordant-baseline.toml. Net: +23/−5 in one Rust file plus one deleted TOML line.
Security risks
None. Pure type-level refactor of a private helper; no new inputs accepted, no bounds or indexing changed, no allocation, no FFI, no user-controllable surface touched.
Level of scrutiny
Low. The transform is a textbook lint fix: each enum variant maps 1:1 to the previous boolean value, and both use sites inside parse were rewritten as exhaustive match arms whose branches are byte-identical to the previous conditional expressions. I verified the mapping in both directions (MySQL true,true ↔ Optional,SpaceOrT; Postgres false,false ↔ Required,Space) and that the two public wrappers' signatures are unchanged, so downstream callers in postgres/types/date.rs and mysql/MySQLValue.rs are unaffected.
Other factors
No CODEOWNERS entry covers these paths. The "no new test" justification is sound per REVIEW.md: the change is type-level in a private function with no observable behavior difference, and existing text-protocol datetime roundtrip tests already exercise both settings of each flag. The baseline-file edit is the mechanical consequence of the fix and matches what the description says rust:mordant:baseline now produces for this section.
#35505) ### Problem - `timestamptz` values decoded from text come back as `Invalid Date` (no error) whenever the server prints the offset with a seconds field. PostgreSQL does that for instants governed by local mean time, which is most zones before roughly 1880-1920: with `SET TIME ZONE 'America/New_York'`, `'1883-11-18 12:00:00+00'::timestamptz` is sent as `1883-11-18 07:03:58-04:56:02` (verified against PostgreSQL 17.11; Europe/Dublin prints `-00:25:21`, Asia/Kolkata `+05:21:10`). - The same text path windows years 0001..0099 into 1900..2099 (`0044-03-15 12:00:00+00` decodes as 2044). - Cause: `from_bytes` in `src/sql_jsc/postgres/DataCell.rs` handed `timestamptz` text to `Bun__parseDate` (JS `Date.parse`). `±HH:MM:SS` is not a JS date format, and the space separator sends JSC down its non-ISO heuristic parser. The naive `timestamp` decoder already parsed components itself; `timestamptz` was the only temporal type still on `Date.parse`. - Reach: the text path is every simple query (`.simple()`, `unsafe()` without parameters), plus every `timestamptz[]` / `timestamp[]` cell, since arrays are always requested in text format even on the extended protocol. The quoted array elements went through `Date.parse` unconditionally (`parse_array`), so `timestamp[]` elements were additionally being read as host-local time while the scalar `timestamp` decoder reads UTC. - The binary path (`timestamptz` scalars on the extended protocol) decodes microseconds since 2000-01-01 and was already correct, so the two protocols silently disagreed on the same value. ### Fix - `datetime_text::parse` now reports how many bytes it consumed; `parse_mysql` / `parse_postgres_timestamp` reject anything trailing exactly as before, and a new `parse_postgres_timestamptz` parses the trailing `±HH`, `±HH:MM` or `±HH:MM:SS` (the three widths PostgreSQL's `EncodeTimezone` produces) into seconds east of UTC. Minute/second fields above 59 return `None`, so such text takes the `Date.parse` fallback (Invalid Date) rather than being read as 99 minutes. - `date::timestamptz_text_to_ms_utc` converts the wall-clock components with UTC arithmetic and subtracts the offset. Correct because the components are the wall-clock in the printed offset, so `instant = wall_clock_as_utc - offset`. - `DataCell.rs`: the scalar branch and the array-element branch share one `parse_date_time_text`, which tries the component parser for `timestamp` / `timestamptz` (and their array tags) and falls back to `Date.parse` only for shapes it does not cover (`date`, which is the date-only ISO form `Date.parse` handles as UTC; BC dates; 5+ digit years). `timestamp[]` elements thereby pick up the existing UTC decoder. - Verified: - `test/js/sql/postgres-timestamptz-text.test.ts` (scripted backend, no server): `USE_SYSTEM_BUN=1 bun test` 5 fail / 2 pass, `bun bd test` 7 pass. Vectors are the offsets above, fractional seconds combined with each offset width, years 0001..0100, `timestamptz[]` and `timestamp[]` elements (the file runs under `TZ=America/New_York` so a local-time decode of `timestamp[]` is caught), `date` / `timestamp` neighbours, and the fallback test (5-digit year still parsed; `+01:99` / `+00:00:99` rejected). - `test/js/sql/sql-postgres-datetime-tz-fixture.ts` (docker lanes, real server) now also sets the session zone to America/New_York and checks the 1883 instant as a scalar and inside `timestamptz[]`, plus a `timestamp[]`, on both protocols, asserting the server really printed `-04:56:02`. After rebasing onto #39441 (which extended this fixture with a result-format sentinel and a sub-millisecond sweep), the sweep's year 0001 and 0099 literals are now checked on the text path too, since that is the windowing this PR removes. Against the local PostgreSQL 17.11: released bun fails exactly those rows under all three TZ values, the debug build prints `OK` for all three. - `postgres-infinity-date`, `postgres-datestyle`, `sql-mysql-datetime-roundtrip` (covers the shared parser's MySQL caller on the text protocol), `wire-frames`, and the 47 date / `timestamp[]` / `timestamptz[]` / `date[]` tests in `sql.test.ts` pass against the live servers with the debug build. `bun run rust:clippy` (whole workspace, 0 warnings), `bun run rust:miri` (all 15 crates; none of the touched crates are in its set), and `cargo fmt --check` are clean on the rebased branch. ### Background - Postgres result cells arrive either as text or binary, chosen per column by the client. Bun asks for binary only for a fixed set of scalar types (`Tag::is_binary_format_supported`); array types and all simple-protocol queries arrive as text, which is why array decoding is the text path regardless of protocol. - Bun pins `DateStyle=ISO` in the startup packet, so the text shapes are fixed: `timestamp` is `YYYY-MM-DD HH:MM:SS[.ffffff]`, `timestamptz` is the same followed by the session offset, `date` is `YYYY-MM-DD`. The offset width is whatever is needed to print the zone exactly: `+00`, `+05:30`, or `-04:56:02` when the zone's rule at that instant is local mean time (a pre-standardization offset measured to the second). - `gregorian_date_time_to_ms_utc` (WTF `DateCache::gregorianDateTimeToMS` in UTC mode) turns calendar components into epoch milliseconds without consulting the host time zone; both drivers' text decoders build on it so text and binary agree on every host. - The binary decoder's own rounding problem for pre-1970 sub-millisecond values (`.123456` decoding as `.124`) was fixed separately in #39441, which this branch is now rebased onto; the text decoders here truncate the printed digits, which agrees with that floor. - `datetime_text.rs` is shared with the MySQL driver, whose DATETIME text has no offset; this change keeps its reject-trailing-bytes contract through the `consumed == text.len()` check. <details> <summary>Live reproduction (PostgreSQL 17.11, bun 1.4.0-canary, TZ=Asia/Tokyo client)</summary> ``` == session TimeZone=America/New_York server text : 1883-11-18 07:03:58-04:56:02 | 1883-11-18 07:03:58.25-04:56:02 arr text : {"1883-11-18 07:03:58-04:56:02","2024-06-01 08:00:00-04"} | {"2024-06-15 12:00:00"} simple tstz : Invalid Date | frac: Invalid Date simple arr : [ "Invalid Date", "2024-06-01T12:00:00.000Z" ] simple ts_arr : [ "2024-06-15T03:00:00.000Z" ] <- timestamp[] read as local time extended tstz : 1883-11-18T12:00:00.000Z <- binary path, correct extended arr : [ "Invalid Date", "2024-06-01T12:00:00.000Z" ] extended ts_arr : [ "2024-06-15T03:00:00.000Z" ] ``` With this change every line above decodes to `1883-11-18T12:00:00.000Z` / `2024-06-15T12:00:00.000Z` for all five session zones tried (America/New_York, Europe/Dublin, Europe/Amsterdam, Asia/Kolkata, UTC). </details> <details> <summary>History</summary> Opened for the years 0001..0099 symptom; rebased onto current main (the shared parser's parameters became enums in #39153) and extended with the seconds-resolution offset vectors, the array coverage, and the real-server fixture after the same decoder was found to be behind the `Invalid Date` results for historical `timestamptz` values. Before the rebase, the only red CI lanes were build jobs whose agents expired; the test lanes that ran were green. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 8 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/sql/postgres-timestamptz-text.test.ts test/js/sql/sql-postgres-datetime-roundtrip.test.ts bun test v1.4.1 (4448a2e) test/js/sql/sql-postgres-datetime-roundtrip.test.ts: failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory 36 | // mismatch. (ASAN emits a harmless interposition warning.) 37 | const diagnostics = stderr 38 | .split(/\r?\n/) 39 | .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) 40 | .join("\n"); 41 | expect(diagnostics).toBe(""); ^ error: expect(received).toBe(expected) - "" + "FAIL TZ=America/New_York offsetMin=240 + text '0001-01-01 00:00:00.123456'::timestamptz: want -62135596799877 got 978307200123 (server says -62135596799877 ms) + text '0099-12-31 23:59:59.999999'::timestamptz: want -59011459200001 got 946684799999 (server says -59011459200001 ms) + binary historical row=0 tstz_arr: want [1883-11-18T12:00 ... (truncated) release without fix: 8 FAILED bun test v1.4.0-canary.1 (4448a2e) test/js/sql/sql-postgres-datetime-roundtrip.test.ts: failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory 36 | // mismatch. (ASAN emits a harmless interposition warning.) 37 | const diagnostics = stderr 38 | .split(/\r?\n/) 39 | .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) 40 | .join("\n"); 41 | expect(diagnostics).toBe(""); ^ error: expect(received).toBe(expected) - "" + "FAIL TZ=Etc/UTC offsetMin=0 + text '0001-01-01 00:00:00.123456'::timestamptz: want -62135596799877 got 978307200123 (server says -62135596799877 ms) + text '0099-12-31 23:59:59.999999'::timestamptz: want -59011459200001 got 946684799999 (server says -59011459200001 ms) + binary historical row=0 tstz_arr: want [1883-11-18T12:00:00.000Z,2024-06-15T12:00:00.000Z] got [Invalid Date,2024-06-15T12:00:00.000Z] + text historical row=0 tstz: want 1883-11-18T12:00:00.000Z got Invalid Date + text historical row=0 tstz_arr: want [1883-11-18T12:00:00.000Z,2024-06- ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/sql/postgres-timestamptz-text.test.ts test/js/sql/sql-postgres-datetime-roundtrip.test.ts bun test v1.4.1 (4448a2e) test/js/sql/sql-postgres-datetime-roundtrip.test.ts: failed to connect to the docker API at unix:///var/run/docker.sock; check if the path is correct and if the daemon is running: dial unix /var/run/docker.sock: connect: no such file or directory (pass) postgres (local) TZ=America/New_York > TIMESTAMP decode is UTC on both protocols [1337.46ms] (pass) postgres (local) TZ=Asia/Tokyo > TIMESTAMP decode is UTC on both protocols [1424.15ms] (pass) postgres (local) TZ=Etc/UTC > TIMESTAMP decode is UTC on both protocols [1580.85ms] test/js/sql/postgres-timestamptz-text.test.ts: (pass) timestamptz text: every offset width Postgres emits (±HH, ±HH:MM, ±HH:MM:SS) [497.62ms] (pass) timestamptz text: fractional seconds combine with every offset width [114.08ms] (pass) timestamptz text: years 0001..0099 decode literally (text path == binary path) [40.13ms] (pass) timestamptz text outside the fixed-width shape still fal ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 773014e features baseline 23 deps, 129 codegen, 1172 objects in 964ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] install /workspace/bun bun install v1.4.0-canary.1 (4448a2e) Checked 26 installs across 63 packages (no changes) [15.00ms] [2/1244] gen bindgenv2 [3/1244] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (4448a2e) Checked 1 install across 2 packages (no changes) [1.00ms] [4/1244] gen ErrorCode+*.h [5/1244] fetch zlib [zlib] up to date [6/1244] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (4448a2e) Checked 111 installs across 104 packages (no changes) [5.00ms] [7/1244] fetch tinycc [tinycc] up to date [8/1243] gen .bind.ts → GeneratedBindings.cpp [9/1243] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [10/1243] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bin ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/sql_jsc/postgres/DataCell.rs | 61 ++++--- src/sql_jsc/postgres/types/date.rs | 20 ++- src/sql_jsc/shared/datetime_text.rs | 72 +++++--- test/js/sql/postgres-timestamptz-text.test.ts | 189 +++++++++++++++++++++ .../js/sql/sql-postgres-datetime-roundtrip.test.ts | 3 +- test/js/sql/sql-postgres-datetime-tz-fixture.ts | 85 ++++++--- 6 files changed, 358 insertions(+), 72 deletions(-) ``` </details> **gate history** · 6 passed · 1 rejected · iteration 8 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/sql_jsc/postgres/DataCell.rs 4 6 0 src/sql_jsc/postgres/types/date.rs 3 4 0 src/sql_jsc/shared/datetime_text.rs 4 10 0 test/js/sql/postgres-timestamptz-text.test.ts 1 1 0 test/js/sql/sql-postgres-datetime-roundtrip.test.ts 1 0 0 test/js/sql/sql-postgres-datetime-tz-fixture.ts 2 1 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
parseinsrc/sql_jsc/shared/datetime_text.rstookallow_date_only: bool, allow_t_separator: bool, and both callers passed bare literals:parse(text, true, true)inparse_mysql,parse(text, false, false)inparse_postgres_timestamp. At the call site nothing says which literal is which flag.bare_bool_argsfinding recorded for this file inmordant-baseline.toml.Fix
TimePart::{Optional, Required}(is the 10-byteYYYY-MM-DDform accepted) andSeparator::{Space, SpaceOrT}(which byte may sit between the date and the time). MySQL passesOptional, SpaceOrT; Postgres passesRequired, Space, matching the previoustrue, true/false, false.parsematch on the enums; the accepted inputs are unchanged. No behavior change.bare_bool_args:src/sql_jsc/shared/datetime_text.rsentry frommordant-baseline.toml. This is whatbun run rust:mordant:baselinenow writes for the[bun_sql_jsc]section.cargo dylint --all -p bun_sql_jscwith the baseline entry removed: no findings with this change; with the oldparserestored it reports exactly this finding over the baseline (over-baseline.txtcontainsbun_sql_jsc 1).bun bd test test/js/sql/sql-postgres-datetime-roundtrip.test.ts test/js/sql/sql-mysql-datetime-roundtrip.test.ts test/js/sql/postgres-datestyle.test.ts test/js/sql/postgres-infinity-date.test.tsagainst local Postgres and MariaDB servers: all pass. Both round-trip files exercise the text protocol (.simple()), which is the path that goes through this parser.DATE(date-only form) andDATETIME(6)with fractional seconds, and a Postgrestimestampwith fractional seconds, all decode to the same instants as the binary protocol.cargo clippy -p bun_sql_jsc --no-depsis clean.sql-mysql-datetime-roundtrip.test.tsand thedatetest insql-mysql.test.ts(TimePart::Optional,Separator::SpaceOrTwith the space form),sql-postgres-datetime-roundtrip.test.ts(TimePart::Required,Separator::Space).Background
datetime_text.rsis the parser both SQL drivers use for the wall-clockYYYY-MM-DD HH:MM:SS[.ffffff]strings their text protocols return. MySQL additionally returns bare dates forDATEcolumns and the parser toleratesTas the separator; Postgrestimestamptext is always the full shape, and anything else must returnNoneso the caller falls back toDate.parse.mordant-baseline.tomlrecords the pre-existing findings per (lint, file) so only new ones are reported. Fixing a recorded finding lets its entry be deleted.Regenerating the whole baseline also drops two unrelated entries
A full
bun run rust:mordant:baselinerun on top of this branch additionally removesalways_unwrapped_option:src/install/PackageInstall.rsandnarrowed_two_ways:src/runtime/node/node_crypto_binding.rs. Those findings were fixed on main after the baseline was recorded (#37648 for the crypto one; #38271 / #38841 touchedPackageInstall.rs). They are left out of this PR to keep it to the one site; happy to include them or send them separately.