fix(graph): stop coercing oversized integer strings into lossy floats - #740
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Re-triggering CI: the |
Note for reviewers: this PR has no
|
|
The red macOS leg that has been blocking this PR is not this PR. Diagnosed and fixed in #742 (issue #741).
Independently, this PR cannot affect that leg: Plan: merge #742 first, rebase this on it, then the bar should be green for the right reason. |
|
Correction to my earlier comment about this PR having no hosted checks — that was wrong. The
So the hosted bar for this PR is complete. The only outstanding item is the local macOS leg, |
GRAPH.ADDNODE and GRAPH.ADDEDGE share parse_property_value, which coerced any
bulk string parsing as i64 or f64 into a number. A value too long for i64 fell
through to f64, which keeps 15-17 significant digits and zeroes the rest. A
32-digit key stored through GRAPH.ADDNODE came back as
12345678901234567000000000000000, and the node could no longer be found by the
key it was created with -- irreversible, and silent.
RESP puts no type tag on a bulk string, so guessing is reasonable; a guess that
CHANGES the value is not. An integer-syntax string that does not fit i64 is an
identifier, not a number, so it now stays a string.
Only the integer case is withheld. moon#724 suggested refusing whenever the
round-trip is inexact (format!("{f}") != s), but that also demotes "3.0" and
"1e5" to strings and would break MATCH (n {x: 3.0}), so genuine float syntax
still coerces exactly as before. "42" is still Int(42), "2.5" still Float(2.5);
i64::MAX still coerces and i64::MAX + 1 no longer does.
Red first: the unit test failed with Float(1.2345678901234567e31) against the
expected string, reproducing the issue's measured corruption exactly. Verified
end-to-end afterwards through the real command path -- the 32-digit key
round-trips and MATCH (n:Doc {_key: '<key>'}) finds it, where before it returned
nothing -- with `WHERE n.age > 20` as the control that numbers still coerce.
is_integer_syntax borrows only and allocates nothing, since this sits on the
command path.
The Cypher path was never affected: it is fed by the grammar, which already
knows the type.
Closes #724
author: Tin Dang
a8a9874 to
98c8042
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesGraph property parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change preserves valid numeric parsing while preventing oversized integer strings from being silently corrupted, with no actionable merge-blocking risk remaining after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is mostly complete. It explains the problem, implementation, rationale, evidence, regression coverage, and notes. It does not use the template headings for Summary, Checklist, or Performance Impact, and it does not explicitly report the checklist results, but the required technical context is present. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Closes #724.
What
GRAPH.ADDNODEandGRAPH.ADDEDGEshareparse_property_value, which coerced any bulk string that parsed asi64orf64into a number. A value too long fori64fell through tof64— 15–17 significant digits kept, the rest zeroed:Irreversible, and silent.
How
RESP puts no type tag on a bulk string, so guessing is reasonable — but a guess that changes the value is not. An integer-syntax string that does not fit
i64is an identifier, not a number, so it stays a string.Why not the fix the issue suggested
#724 proposed refusing whenever the round-trip is inexact (
format!("{f}") != s). I did not use that rule, because it also demotes"3.0"→"3"and"1e5"→"100000", turning legitimate float properties into strings and breakingMATCH (n {x: 3.0}). Withholding only the integer case fixes the reported data loss with no change to float behaviour.Unchanged:
"42"→Int(42),"2.5"→Float(2.5),"3.0"→Float(3.0),"1e5"→Float(1e5),i64::MAX→Int.Changed:
i64::MAX + 1and-9223372036854775809→Stringinstead of a lossyFloat.Evidence
Red first. The unit test failed with
Float(1.2345678901234567e31)against the expected string — reproducing the issue's measured corruption exactly, not a proxy for it.Green, then end-to-end through the real command path:
No regressions: 610 lib tests and 641 graph/cypher/property integration tests pass.
Notes
is_integer_syntaxborrows only and allocates nothing — this sits on the command path.parse_property_valuewas widened topub(super)so the test can address it directly. Test lives inmod.rsper the split convention.Summary by CodeRabbit
GRAPH.ADDNODEandGRAPH.ADDEDGEare now preserved exactly as strings instead of being converted inaccurately.