feat!: Use num-bigint for large numbers - #500
Merged
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
anakrish
force-pushed
the
number-semantics
branch
7 times, most recently
from
November 24, 2025 22:38
46674fb to
3f9ff7a
Compare
- Supply chain: Use the popular num-bigint crate for handling large integers - Optimization: Handle f64, i64, u64 directly. These will be the most common instances of a number. OPA number semantics isn't clear. open-policy-agent/opa#6281 As part of this change, we update the following failing tests: - A local test that relies on what 15.3/3 evaluates to. With our current change, we round in a different direction than what OPA does, but consistent with Rust. We produce 5.1000000000000005 where as the OPA test expects 5.1. There is no clear definition in Rego of what the right answer is. Moreover, policies should not rely on exact floating point value comparison. Therefore this deviations is justified. The test is patched to pass. - Another local vm test that exercised 1.1 + 2.2 - Another local vm test that exercises 5.5 - 2.2 - An OPA test that expects that a large integer number say 10e308 is printed in exponent notation. num-bigint does not print using scientific notation and instead prints all the digits. The benefit of preserving this compatibility is not clear. We skip this test. - Doc tests that exercised handling floating point numbers with more than 15 (what f64 supports) digits of precision. There is no usecase for this scenario. The tests are updated to reflect the behavior. Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
anakrish
force-pushed
the
number-semantics
branch
from
November 24, 2025 22:44
3f9ff7a to
5297f45
Compare
anakrish
changed the title
anakrish
marked this pull request as ready for review
November 24, 2025 22:56
| return Number::UInt(u); | ||
| } else if let Some(i) = value.to_i64() { | ||
| return Number::Int(i); | ||
| } |
Collaborator
There was a problem hiding this comment.
nit: not sure if it matters, but should we try converting to i64 first and then u64? otherwise won't the i64 clause never be hit?
Collaborator
Author
There was a problem hiding this comment.
Good observation. Let me reorder.
| return Ok(Number::Float(self.to_f64_lossy() / rhs.to_f64_lossy())); | ||
| } | ||
|
|
||
| match (&self, rhs) { |
Collaborator
Collaborator
Author
There was a problem hiding this comment.
There are some nuances as well as boilerplate. Let me add the tests in the next PR.
dekomissMSFT
approved these changes
Nov 25, 2025
Collaborator
Author
|
@dekomissMSFT Thanks for the approval. I will address both your comments in a follow up PR, just to keep the momemtum going. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat!: Use num-bigint for large numbers
OPA number semantics isn't clear.
open-policy-agent/opa#6281
As part of this change, we update the following failing tests:
With our current change, we round in a different direction than what OPA does, but consistent
with Rust. We produce 5.1000000000000005 where as the OPA test expects 5.1.
There is no clear definition in Rego of what the right answer is. Moreover, policies should not
rely on exact floating point value comparison. Therefore this deviations is justified.
The test is patched to pass.
num-bigint does not print using scientific notation and instead prints all the digits.
The benefit of preserving this compatibility is not clear. We skip this test.
digits of precision. There is no usecase for this scenario. The tests are updated to reflect
the behavior.
closes #165
Signed-off-by: Anand Krishnamoorthi anakrish@microsoft.com