Skip to content

fix(query)!: carry result cells as JSON text - #138

Merged
anoop-narang merged 4 commits into
mainfrom
fix/decimal-precision
Sep 10, 2026
Merged

anoop-narang merged 4 commits into
mainfrom
fix/decimal-precision

Conversation

@anoop-narang

@anoop-narang anoop-narang commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

The bug

A DECIMAL wide enough to need more than ~17 significant digits comes back rounded, silently.

// SELECT CAST('99999999999999999999.99' AS DECIMAL(38,2))
// HTTP response body:  99999999999999999999.99
// via this SDK:        1e20

The response bytes are exact. rows was typed Vec<Vec<serde_json::Value>>, and Value has no arbitrary-precision number variant — anything that does not fit an i64/u64 is parsed through f64. The digits were gone at deserialization, before any caller could see them.

Integers are unaffected (Value has real i64/u64 variants). In practice this is DECIMAL.

The fix

rows now carries JsonCell: a #[serde(transparent)] newtype over Box<RawValue> holding the cell's JSON text and re-emitting it verbatim. Nothing is decided at parse time, so the digits the service wrote are the digits that come back. A number stays an unquoted JSON number — a cell round-trips to the service's own bytes rather than turning into a string.

Reading a cell: as_json_str() (lossless), as_str(), as_array() (elements keep their own digits), is_null(), kind(). to_value() returns the previous serde_json::Value, rounding included, for callers not ready to move.

arbitrary_precision is not an option here and the Cargo.toml comment says so: it is a crate-wide feature that changes how every serde_json::Number serializes, which corrupts non-JSON formats for anything downstream. raw_value only adds a type and changes nothing for code that does not name it.

Keeping it through regeneration

src/models/query_response.rs is generated, and regenerate.yml does rm -rf src/apis src/models docs before generating — so ignore-listing the file would delete it and never recreate it, leaving mod.rs declaring a missing module. Instead:

  • scripts/normalize-openapi.py (which already rewrites the throwaway spec for generator reasons) names the anonymous row-cell schema JsonCell
  • regenerate.yml passes --type-mappings / --import-mappings for it
  • the generator emits Vec<Vec<models::JsonCell>> and skips writing the model; one hand-written src/models/json_cell.rs supplies the type

Only the cell type is pinned — the surrounding models keep regenerating normally, so a future spec change to QueryResponse is not silently dropped. A regen guard fails the workflow if Vec<Vec<serde_json::Value>> reappears or mod.rs stops re-exporting JsonCell, and the normalizer exits non-zero if the spec's rows shape changes out from under it.

This was verified by running openapi-generator 7.22.0 twice — unpatched vs. with the normalizer — and diffing the trees. The whole delta is 4 type substitutions, 2 lines in mod.rs, and 2 doc lines, which is exactly what is committed here. The checked-in tree equals what the next regeneration produces.

Verification

Both paths that carry rows:

  • inline query response — a_wide_decimal_survives_the_inline_path
  • paginated /results/{id} — a_wide_decimal_survives_result_pagination

cargo test --all-features --lib: 113 passed. Full suite green.

Breaking change

QueryResponse::rows, GetResultResponse::rows and QueryResponse::new now take Vec<Vec<JsonCell>>. Comparisons against a literal become JsonCell::from(serde_json::json!(...)). serde_json is the only supported format for a JsonCell — it round-trips through a RawValue, which other serde formats do not recognise; to_value() is the way out to one.

The version is deliberately not bumped here. RELEASING.md states versions are never bumped by hand — notes go under ## [Unreleased] and ./scripts/release.sh prepare minor opens the release PR. That release needs to land before hotdata-cli can pick this up, since its own fix depends on JsonCell.

Unrelated, noticed while testing

cargo test without --all-features fails to compile examples/quickstart.rs: its #[cfg(not(feature = "arrow"))] stubs have drifted from their call sites. Pre-existing on main and untouched here — CI only ever builds --all-features, so the non-arrow configuration is never compiled. Left out of this PR; worth its own fix.

A JSON number in a result row was parsed into a `serde_json::Value`,
which has no arbitrary-precision number variant. Anything wider than an
f64 can hold was rounded at deserialization, before a caller saw it: a
DECIMAL(38,2) the service sent as 99999999999999999999.99 arrived as
1e20. The service's own bytes were exact; the loss was entirely on this
side.

Rows now carry `JsonCell`, a transparent newtype over `Box<RawValue>`
holding the cell's JSON text and re-emitting it verbatim, so a number
stays an unquoted JSON number and round-trips to the service's bytes.

`raw_value` is deliberate and `arbitrary_precision` is not an option:
that feature is crate-wide and changes how every serde_json::Number
serializes, which corrupts non-JSON formats for downstreams.

The row-cell schema is named in the spec normalizer and passed to the
generator via type/import mappings, so regeneration keeps emitting the
type and only the cell is pinned; the surrounding models still flow. A
regen guard fails if `Vec<Vec<serde_json::Value>>` reappears.

BREAKING CHANGE: `QueryResponse::rows`, `GetResultResponse::rows` and
`QueryResponse::new` take `Vec<Vec<JsonCell>>`. `to_value()` returns the
previous `serde_json::Value`, with its rounding, for callers not ready
to move.
@anoop-narang
anoop-narang requested a review from a team as a code owner September 10, 2026 13:03
@anoop-narang
anoop-narang requested review from rohan-hotdata and removed request for a team September 10, 2026 13:03
RELEASING.md is explicit that versions are not bumped by hand: notes go
under [Unreleased] and `./scripts/release.sh prepare` opens the release
PR. Bumping here also failed check-release.py, which wants a matching
CHANGELOG section for a bumped version.
Comment thread src/models/json_cell.rs Outdated
Comment thread src/models/json_cell.rs Outdated
claude[bot]
claude Bot previously approved these changes Sep 10, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. Two non-blocking nits inline on src/models/json_cell.rs.

Checked the type change against every consumer of rows: src/query.rs (fetch_all_rows, cell_len), the generated models, examples/quickstart.rs and README.md. No other call site reads a cell. The generated API layer deserializes with serde_json::from_str, which RawValue supports, and the double_option wrapper on GetResultResponse::rows passes the original deserializer through, so both row paths keep the raw text. QueryResponse derives Clone, Default, Debug, PartialEq, Serialize, Deserialize, all of which JsonCell provides.

The regen guards cover both halves of the mapping: the field type and the mod.rs re-export. The generator behavior behind --import-mappings is not verifiable here, but drift fails the workflow rather than shipping rounded numbers.

CI checks for tests were still running at review time.

`as_str` returned an owned `String`, one allocation per call, where
`Value::as_str` had handed back a borrowed `&str` — a new per-row cost
on the migration path for anyone reading a string column. It now
returns `Cow<'_, str>`, borrowing the slice between the quotes and
decoding only an escaped string.

`to_value` mapped a parse failure to `Value::Null`, which no caller
could tell apart from a cell that really is null. The text is valid
JSON by construction, so a failure is a broken invariant: panic and
name it rather than substitute an absence.
Comment thread src/models/json_cell.rs Outdated
claude[bot]
claude Bot previously approved these changes Sep 10, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both prior nits are addressed. as_str now borrows the unescaped body, which is exact: JSON forbids an unescaped ", \, or control character inside a string, so the text between the quotes is the value. to_value no longer substitutes a null.

Note that CI checks for this push were still queued or in progress when this review ran, so the test results are not visible here.

Both fallbacks invented a value for a state that cannot occur: a failed
quote strip became an empty string, and a failed decode of an escaped
string became `None`, which says "not a string" about a cell that is
one. Either reads as real data. Panic and name the invariant, matching
`to_value`.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cycle 3 review: the two expect calls in as_str sit on branches the RawValue invariant closes, and the escaped/plain split stays correct for a trailing escaped quote, a lone backslash, and an empty string. Response bodies are parsed with serde_json::from_str, which supports raw-value capture. JsonCell supplies every trait the two response models derive. No blocking issues; prior nits are resolved.

Note: the build and test checks were still queued when this review ran, so this approval does not assert that CI passed.

@anoop-narang
anoop-narang merged commit 85317f9 into main Sep 10, 2026
4 checks passed
@anoop-narang
anoop-narang deleted the fix/decimal-precision branch September 10, 2026 13:22
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.

1 participant