Skip to content

fix(native): Don't fail deserializing JS numbers above u64/i64 range#11034

Merged
ovr merged 1 commit into
masterfrom
feat/js-deserializer-number-int
Jun 8, 2026
Merged

fix(native): Don't fail deserializing JS numbers above u64/i64 range#11034
ovr merged 1 commit into
masterfrom
feat/js-deserializer-number-int

Conversation

@ovr

@ovr ovr commented Jun 8, 2026

Copy link
Copy Markdown
Member

A JS number like 5.18e44 is integer-valued (fract() is always 0.0 for |value| >= 2^52, since an f64 has no fractional bits at that magnitude), so it skipped the float path, then failed to fit in any u64/i64 branch and hit "Unsupported number type for deserialization".

Fall back to visit_f64 for such values instead of erroring. The value already arrived as an f64, so nothing is lost beyond JS's own precision.

@ovr ovr requested a review from a team as a code owner June 8, 2026 12:39
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jun 8, 2026
@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 38s —— View job


Code Review

The change is small and correct. JS numbers are all f64, so an integer-valued magnitude above u64::MAX (e.g. 5.18e44) is one whose fract() happens to be 0.0 but which can't be losslessly bucketed into any uN/iN. Falling back to visit_f64 keeps the value at its original precision rather than failing.

Findings

  • Correctness: ✅ Logic is sound. Negative values past i64::MIN also flow into the same fallback, which is the right behavior.
  • Performance/Security: No concerns. Pure deserialization path, no I/O or unsafe.
  • Behavioral change: Downstream consumers that explicitly asked for deserialize_u64 will now get visit_f64 for out-of-range magnitudes. In practice, serde visitors for integer types reject floats, so callers requesting a strict integer will still see an error — just from the visitor instead of the deserializer. Worth being aware of, but matches what already happens for value.fract() != 0.0 at line 73, so it's consistent.
  • Tests: No tests exist for node_obj_deserializer.rs in this package, so the lack of a regression test here is consistent with the file's current state — but adding one for this specific edge case (number above u64::MAX) would be a nice follow-up.
  • Comment quality: The inline comment explains the why clearly. 👍

Nit (non-blocking): The chain value <= u8::MAX as f64u16u32u64 could in theory still misclassify the largest exact u64 values due to u64::MAX as f64 rounding up to 2^64, allowing a value of exactly 2^64 to take the u64 branch and then overflow when cast back. Not introduced by this PR — pre-existing — but worth noting if you're looking at this file anyway.

LGTM.
Branch

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.78%. Comparing base (107e63f) to head (add4d17).

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11034       +/-   ##
===========================================
+ Coverage   58.44%   78.78%   +20.34%     
===========================================
  Files         216      471      +255     
  Lines       17248    94509    +77261     
  Branches     3520     3520               
===========================================
+ Hits        10080    74459    +64379     
- Misses       6660    19542    +12882     
  Partials      508      508               
Flag Coverage Δ
cube-backend 58.44% <ø> (ø)
cubesql 83.32% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

A JS number like 5.18e44 is integer-valued (fract() is always 0.0 for
|value| >= 2^52, since an f64 has no fractional bits at that magnitude),
so it skipped the float path, then failed to fit in any u64/i64 branch
and hit "Unsupported number type for deserialization".

Fall back to visit_f64 for such values instead of erroring. The value
already arrived as an f64, so nothing is lost beyond JS's own precision.
@ovr ovr force-pushed the feat/js-deserializer-number-int branch from 7e4882a to add4d17 Compare June 8, 2026 14:13
@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 29s —— View job


Code Review

Verdict: LGTM — 0 high, 0 medium, 1 low (pre-existing, not introduced by this PR).

Details
  • Read PR diff
  • Inspect surrounding deserializer logic
  • Assess correctness, performance, behavioral impact

Correctness: ✅ The reasoning in the PR description is right — for |value| >= 2^52, f64 has no fractional bits so fract() == 0.0, and the value skips the float branch at line 73. Magnitudes above u64::MAX / below i64::MIN previously errored; falling through to visit_f64(value) keeps the original f64 verbatim. Nothing is lost beyond JS's inherent precision.

Comment quality: The inline comment is clear about the why — good. 👍

Behavioral change worth flagging: Downstream visitors that explicitly asked for an integer (deserialize_u64, etc.) will now receive visit_f64 for out-of-range magnitudes. Strict integer visitors will still reject — just the error now surfaces from the visitor instead of the deserializer. This is consistent with how line 73 already handles non-integral floats, so it's not a regression.

Performance / security: No concerns. Pure deserialization path.

Tests: No tests exist for node_obj_deserializer.rs; lack of a regression test here is consistent with the file's current state. Adding one for the 5.18e44 edge case would be a nice follow-up but not blocking.

Low (pre-existing, not introduced): The unsigned chain at lines 79–93 uses value <= u64::MAX as f64. Since u64::MAX (2^64 − 1) rounds up to 2^64 when cast to f64, a value of exactly 2^64 passes the bound and then overflows on value as u64. Same applies to the i* casts at 96–110 for the upper-bound side. Not in scope for this PR, just noting since the file is open.

• [Branch](https://github.com/cube-js/cube/tree/feat/js-deserializer-number-int)

@ovr ovr merged commit 44bd455 into master Jun 8, 2026
92 of 93 checks passed
@ovr ovr deleted the feat/js-deserializer-number-int branch June 8, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant