-
-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add/comment/multipleof #1219
Add/comment/multipleof #1219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to the JSON Schema Community. Thanks a lot for creating your first pull request!! 🎉🎉 We are so excited you are here! We hope this is only the first of many! For more details check out README.md file.
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1219 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 373 373
Branches 94 94
=========================================
Hits 373 373 ☔ View full report in Codecov by Sentry. |
To ensure consistent validation: | ||
- Use scaled integers (e.g., represent $4.02 as 402 with 'multipleOf: 1') | ||
- If floating-point values are necessary, use binary-friendly numbers (e.g., 0.5, 0.25, 0.125)</p><a href="#section-6.2.1-2" class="pilcrow">¶</a></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the recommendation we want to make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say "no".
Use of scaled integers is ok but is a choice not a recommendation. And we don't have direct support for a scaled integer - e.g. defining the scale. You push a whole new problem onto consumers.
When it comes to non-integers it isn't really meaningful; why would we be recommending that people only use "particular values". And then there is the problem of representation on all platforms (e.g. 1.3, 2.4, 5.6 cannot be represented precisely as a 32 bit float).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A use case could be defining currency, for example dollars & cents. You'd want to ensure that your data only has a penny ($0.01) resolution.
But the point is that we don't want to force users into a scenario where they should be trying to figure out the nuances of binary float arithmetic while writing their schemas.
I think that the very most, we should be leaving a note that some implementations may use IEEE754 math.
I do think the way that the spec is worded is pretty clever, though.
6.2.1. multipleOf
The value of "multipleOf" MUST be a number, strictly greater than 0.
A numeric instance is valid only if division by this keyword's value results in an integer.
In C#,
double a = .0075; // IEEE754
double b = .0001;
decimal c = .0075m;
decimal d = .0001m;
Console.WriteLine("double");
Console.WriteLine(a / b);
Console.WriteLine(a % b);
Console.WriteLine();
Console.WriteLine("decimal");
Console.WriteLine(c / d);
Console.WriteLine(c % d);
outputs
double
75
9.999999999999937E-05
decimal
75
0.0000
So following the method prescribed by the spec (using division instead of modulation) actually produces the correct result, even with IEEE754 math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing monetary values as cents is a good idea, to keep all math as integers, but a better way of specifying that in the schema is with type: integer
not multipleOf: 1
.
I don't think we want to add anything like scale
into the spec, though -- this should be defined and controlled by the individual application.
We may want to add a note that tooling might implement the multipleOf
keyword using modulo calculations, as a lead-in to the warning about imprecision with floating point storage.
Hello @gregsdennis This issue was originally assigned to me & I have also opened a PR #1206 on this. I believe you might have missed that, so please check that out. Also, this PR edits the draft itself, however in a conversation with @valeriahhdez, she told me the following:
So I don't think it would be meaningful to merge this PR. Moreover, @akshat09867 I understand your excitement to contribute, but please first check if the issue is assigned to someone else. This was mentioned by multiple maintainers in their slack channel recently, so you might want to consider joining that. If you don't know where to check if the issue was assigned to someone else, in the issue page, there's a tab on the right side and you'll generally find the Assignees the first thing there. |
@inclinedadarsh is right. I'm closing this PR. |
Documentation Update: multipleOf Validation Specification
Related to #1113
Summary
Updates multipleOf validation specification to address IEEE 754 floating-point precision issues, providing clearer guidance on implementation.
Documentation Changes