sql: set a cell's column index and kind in one place - #39135
Conversation
The ColumnIdentifier -> (index, is_indexed_column) mapping was written out four times: both MySQL row decoders, the Postgres Putter, and SQLDataCell::null_for_column. Move it into SQLDataCell::set_column and call that from all four sites. No behavior change. Drops the same_match_twice baseline entry for mysql/protocol/ResultSet.rs.
|
Warning Review limit reached
Next review available in: 25 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 (4)
Comment |
|
Status: refactor only, no behavior change intended.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure dedup refactor, behavior-preserving at all four sites.
What was reviewed:
- Traced each of the four call sites (
decode_text,decode_binary,put_impl,null_for_column) against the newset_column— same two fields set to the same values; the swapped write order ofis_indexed_column/indexis unobservable. - Confirmed
ColumnIdentifierhas exactly the three variants the match covers, andIndex's payload isu32(matchescell.index). - Checked the now-unconditional
u32::try_from(index)in the MySQL loops —indexis a bounded column ordinal, and the same conversion already ran on the commonNamearm, so no new panic surface. - Removed imports (
NameOrIndexalias,ColumnIdentifierin DataCell.rs) have no remaining references; the dropped mordant-baseline entry matches the removed duplicate match.
Extended reasoning...
Overview
This PR extracts four byte-identical copies of the ColumnIdentifier → (index, is_indexed_column) mapping into a single SQLDataCell::set_column(position, &ColumnIdentifier) helper. The four sites — MySQL decode_text and decode_binary in ResultSet.rs, Postgres Putter::put_impl in DataCell.rs, and null_for_column in SQLDataCell.rs — now call the helper with the same arguments they previously fed into the inline matches. The stale same_match_twice mordant-baseline entry for ResultSet.rs is removed, and two now-unused imports are dropped. Net -19 lines.
Security risks
None. This is an internal refactor of how two POD fields on a #[repr(C)] cell are populated from an already-classified column identifier. No parsing, no untrusted input handling, no allocation, no FFI signature changes — the C++ side (SQLClient.cpp) sees identical bytes.
Level of scrutiny
Low-to-medium. The touched code is on the per-row hot path for both SQL drivers, but the change is a mechanical extract-method with no control-flow or data-flow changes. I verified line-by-line that each call site produces the same index and is_indexed_column values as before. The one microscopic difference — u32::try_from(loop_index) now runs unconditionally in the MySQL decoders instead of only on the Name/Duplicate arms — cannot introduce a new panic because the loop index is bounded by the column count and the same conversion was already executed for the (far more common) non-Index variants.
Other factors
The ColumnIdentifier enum has exactly three variants (Name, Index(u32), Duplicate), so the helper's matches are exhaustive and equivalent to all four originals. null_for_column's rewrite from a struct literal to null() + set_column is equivalent because null() is SQLDataCell::default(), which the original spread from. The PR description documents thorough verification against local MariaDB/Postgres including the digit-named-column, duplicate-column, and short-DataRow test cases that exercise every branch of this mapping, plus a byte-identical output probe against released 1.4.0. No CODEOWNERS cover these paths and there are no outstanding reviewer comments.
### Problem - mordant's `reimplemented_helper` finding baselined for `src/jsc/webcore_types.rs`: `Blob::is_bun_file` and `Blob::needs_to_read_file` had the same signature and the same body (the store is `Some` and its data is `Store::File`), so a change to one would have silently missed the other. - The duplication is inherited, not a porting mistake: `Blob.zig` already had `isBunFile` and `needsToReadFile` with the same body, and neither side has changed since the port (#30412). Nothing relies on them differing. ### Fix - Delete `is_bun_file` and keep `needs_to_read_file`, which is what the ~40 other call sites (and the `blob_needs_to_read_file` hook that `bun_sql_jsc` goes through) already use. - Point the two `is_bun_file` callers at it: `Response.rs` (`getCompleteWebRequestOrResponseBodyValueAsArrayBuffer` returns `undefined` for a file-backed body) and `CryptoHasher.rs` (the synchronous hashers reject `Bun.file()` input). Same predicate, so no behavior change. - Remove the `reimplemented_helper:src/jsc/webcore_types.rs` entry from `mordant-baseline.toml`, and the two stale comments in `webcore/Blob.rs` that named the deleted method. The entry is removed by hand rather than by regenerating the file: a full `bun run rust:mordant:baseline` on Linux also drops two unrelated entries (`always_unwrapped_option` in `PackageInstall.rs`, `narrowed_two_ways` in `node_crypto_binding.rs`) that this PR has no business touching. - No new test: there is no observable behavior to pin (the two predicates were byte-for-byte the same), so no test can distinguish before from after. The `Bun.file()` rejection at the `CryptoHasher` call site is already covered by `bun-cryptohasher.test.ts` ("Bun.file in CryptoHasher is not supported yet"), and the mordant run below is the check for the finding itself. Same shape as #39124, #39116 and #39135 from this batch. - Verified: - `bun run rust:mordant` (full workspace) on this branch: clean, no `target/mordant/over-baseline.txt`. With main's `webcore_types.rs` restored on top of the trimmed baseline it reports `reimplemented_helper ... over the mordant baseline (0 recorded for src/jsc/webcore_types.rs)` and `1 finding(s) over the baseline in bun_jsc`, so the entry was this site and the lint no longer fires. - The `mordant` workflow also runs on this PR since the baseline changed; with the entry removed it fails if the finding is still reported. - `bun bd test test/js/bun/util/bun-cryptohasher.test.ts` (covers the `CryptoHasher` call site: `Bun.file()` input still throws): 402 pass. - `bun bd test` on `test/js/web/fetch/{blob,blob-write,body,blob-file-name-ownership,response}.test.ts`, `test/js/web/structured-clone-blob-file.test.ts` and `test/js/bun/io/bun-write.test.js` (`--timeout 60000`, since several of these spawn a debug+ASAN child and exceed the 5s default): everything passes except `blob.test.ts` "Bun.file(path).slice(start, end) streams only the slice", which fails identically on a clean checkout of main (88a6398) and is being handled separately; it is a `FileReader` streaming bug unrelated to this predicate. - Two open PRs add `is_bun_file()` calls in `Blob.rs` (#32434, #33659); whichever lands after this one needs those calls renamed to `needs_to_read_file()`. That is a compile error, not a silent change. ### Background - `Blob` (`src/jsc/webcore_types.rs`) is a view (offset + size) onto a refcounted `Store`, whose `data` is one of `Bytes` (in memory), `File` (a path or fd; what `Bun.file()` creates) or `S3`. `needs_to_read_file()` asks whether the blob is `File`-backed, i.e. whether its bytes have to be read off disk before anything in memory can look at them; synchronous consumers use it to bail out. - mordant is the advisory Rust lint pack the `rust-lints` workflow runs (`bun run rust:mordant`). `mordant-baseline.toml` holds the per-(lint, file) counts of findings that predate the job, so CI only fails when a count goes up; once a baselined finding is fixed its entry is deleted. Co-authored-by: Alistair Smith <hi@alistair.sh>
2 participants
Problem
ColumnIdentifierto the two cell fields SQLClient.cpp reads (index,is_indexed_column) was written out four times:mysql/protocol/ResultSet.rs(decode_textanddecode_binary),postgres/DataCell.rs(Putter::put_impl), andshared/SQLDataCell.rs(null_for_column).ResultSet.rsassame_match_twice(the entry inmordant-baseline.toml); a change to one copy would not reach the others.Fix
SQLDataCell::set_column(position, &ColumnIdentifier), which holds the mapping once; the four sites call it (null_for_columnis nownull()+set_column).same_match_twice:src/sql_jsc/mysql/protocol/ResultSet.rsbaseline entry. The CI mordant job is the check that nothing else moved in the baseline.test/js/sql/sql-mysql-binary-null-indexed.test.ts,sql-mysql-column-name-digits.test.ts,sql-mariadb-json.test.ts(the digit-named column tests cover both MySQL decoders).test/js/sql/sql.test.ts -t "data row that omits columns"(coversnull_for_columnandput_impltogether, with duplicate and digit-named columns).postgres-datarow-overrun,postgres-multi-statement-fields,postgres-binary-array-bounds,sql-mysql-mediumint,sql-mysql-bigint-out-of-range,sql-mysql-datetime-roundtrip: all pass..values(),.raw()and.simple()on both drivers produced byte-identical output on this build and on released bun 1.4.0 (the debug build also keeps theisIndexedColumn/isNamedColumnasserts in SQLClient.cpp live).sql-mysql.test.ts/sql.test.tsneeds docker, which this environment does not have; CI runs it.Background
SQLDataCellis the#[repr(C)]cell both drivers fill per column and hand toJSC__constructObjectFromDataCell(SQLClient.cpp) to build the row object.ColumnIdentifieris how a result column's name is classified when the column list arrives:Name(an ordinary key),Index(n)(the name is all digits, so the value is stored at array indexn, which can be out of order and larger than the column count), orDuplicate(an earlier occurrence of a repeated name, which object results skip; the last occurrence wins).is_indexed_columncarries that classification as 0 / 1 / 2, andindexis theIndexnumber or, otherwise, the column's ordinal position in the row.