Skip to content
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 more multipleOf tests #413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jul 8, 2020

There were already some tests for fractional multipleOf values, but while writing an implementation I discovered that those were
not enough to catch a certain kind of poor implementations.

As JSON encodes arbitrary numbers in decimal form, the correct implementation should either use lossless decimal presentation, or at least operate in "shortest decimal which is presented as A is multiple of shortest decimal which is presented as B" to be conforming.

With these small numbers, that is definitely possible even for systems where numbers are presented as half-precision floating-point values.

@ChALkeR ChALkeR force-pushed the chalker/multipleOf branch 2 times, most recently from 3546015 to 82b27d8 Compare July 8, 2020 14:08
There were already some tests for fractional multipleOf values,
but while writing an implementation I discovered that those were
not enough to catch a certain kind of poor implementations.

As JSON encodes arbitrary numbers in decimal form, the correct
implementation should either use lossless decimal presentation, or at
least operate in "shortest decimal which is presented as A is multiple
of shortest decimal which is presented as B" to be conforming.
@Julian
Copy link
Member

Julian commented Jul 8, 2020

I don't think this is actually the case -- definitely the spec doesn't mandate using decimals.

And "shortest decimal which is presented as A is multiple of shortest decimal which is presented as B" -- an implementation isn't required to assume so either as far as I know. There's not really any way to assume if you give me 0.6 that you meant 0.6 and return true and not in fact 0.09999999999999995 originally and return false once that float has been consumed by the JSON parser.

To be specific, my implementation fails these under the default behavior. My response to users is always "JSON Schema generally sits downstream of your JSON parser. If you want decimals, then parse into decimals and things will work as you expect, otherwise you get float division behavior, which some folks may very well want".

I suspect many if not most implementations are in the same boat.

Adding these to optional seems reasonable for implementations that want to test decimal behavior (certainly seems useful) -- but yeah what makes you say this behavior is required?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

JSON spec:

Screenshot_20200708_172507

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

@Julian
Why are tests for 0.0075 is multiple of 0.0001 in non-optional then?
How is 0.3 is multiple of 0.1 worse than that?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

If JSON Schema validators should not be expected to work reliably with even trivial non-integer values like 0.1, then my take would be that the spec should be amended to allow only integer multipleOf values. Otherwise consumers can't expect a consistent behavior.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

There's not really any way to assume if you give me 0.6 that you meant 0.6 and return true and not in fact 0.09999999999999995 originally and return false once that float has been consumed by the JSON parser.

That is not correct, there is a way and I described it, which will pass here even if numbers are parsed as half-precision floats:
convert both values back to decimal according to the precision, divide them.

There's not really any way to assume if you give me 0.6 that you meant 0.6

If this was correct, then floating point numbers wouldn't have been convertible back to decimal anywhere. But they are (if precision allows, which is certainly true for these numbers). 0.6 parsed from string "0.6" is always convertible back to "0.6" as a string, in all implementations that I'm aware of. It's arithmetic operations that skew things -- but they are not required here pre-division.

This means that it's also convertible to a decimal, which means that this multipleOf operation can be done correctly even for half-precision floats.

@Julian
Copy link
Member

Julian commented Jul 8, 2020

Will review your comments, but just since I already see the first one:

Why are tests for 0.0075 is multiple of 0.0001 in non-optional then?

Good question :) -- probably indeed a mistake (according to my interpretation)!

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

0.09999999999999995

That is a question of which numbers are representable in the system, which is a completely different issue from this test.

I.e.: 0.6 is always representable if fraction numbers are supported, 0.0999999999999999995 might not be.
This test does not include numbers that might be non representable.

@Julian
Copy link
Member

Julian commented Jul 8, 2020

I.e.: 0.6 is always representable if fraction numbers are supported, 0.0999999999999999995 might not be.
This test does not include numbers that might be non representable.

My point was a JSON Schema implementation being used alongside a JSON parser that parses into floats (and then the JSON parser encounters "0.6" in JSON, and parses it) -- will see the float 0.0999999999999999995 -- how does the JSON Schema implementation know whether the original schema contained 0.6 and was parsed into the closest float, or whether the original schema actually contained the exact number 0.0999999999999999995?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

As an demonstration: you won't be opposed to tests for { "const": 0.1 } on the basis that { "const": 0.0999999999999999995 } might not work if 0.0999999999999999995 is not representable? But, if so, what's the reason to be opposed to this test on the exact same basis?

Those are entirely similar in my point of view, this operation is well-defined for representable numbers, because JSON deals with decimal strings and we can expect that the number came in as a decimal.

This test uses 0.1 and not 0.0999999999999999995, and it should work.

Unrepresentable numbers are a completely different issue from the thing this test is checking.
And I don't think it's a reason to throw out everything with a non-round number into /optional.

@Julian
Copy link
Member

Julian commented Jul 8, 2020

I'm not sure I'm following you, so we may need to chat quickly on Slack just so I fundamentally understand what you're saying, because yeah I'm having difficulties (which is likely me being misinformed, or not giving enough attention yet).

But I don't understand a sentence like:

Those are entirely similar in my point of view, this operation is well-defined for representable numbers, because JSON deals with decimal strings and we can expect that the number came in as a decimal.

JSON allows implementations to parse both into decimals or into floats, yes?

And JSON Schema implementations in general in practical places deal with language level objects, not the original strings, yes? So an arbitrary JSON Schema implementation does not know that you wrote the string "0.6" in the schema, which isn't representable as a float, if the JSON parser gives it something else, right?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 8, 2020

"0.6" in the schema, which isn't representable as a float

That's incorrect. 0.6 is representable as a float (knowing that it came from decimal). String "0.6" converts to float 0.6 and vice versa.
Let's go to Slack.

JSON allows implementations to parse both into decimals or into floats, yes?

Afaik JSON just decribes the interchange format. It doesn't care about floats (or even precision).

And JSON Schema implementations in general in practical places deal with language level objects, not the original strings, yes?

Same is true for literal numbers in the languages, i.e. this should as well work with literal 0.6 checked against this schema.
Might not work for 0.1 * 6 though due to precision loss on arithmetic operations. ;-) But that's the problem of consumers and where they did their numbers from.

@karenetheridge
Copy link
Member

If JSON Schema validators should not be expected to work reliably with even trivial non-integer values like 0.1...

Floats are not trivial, no matter how simple the number appears in base 10 :)

@karenetheridge
Copy link
Member

karenetheridge commented Jul 8, 2020

FWIW, 3.3 / 1.1 = 2.99999999999999956 on my system (x86_64 darwin)

@karenetheridge
Copy link
Member

My current inclination is to move all non-integer multipleOf tests into the optional directory, because real-world implementations may experience limitations in the same vein as the optional/bigNum.json tests.

@karenetheridge
Copy link
Member

karenetheridge commented Jul 13, 2020

I made all these tests pass much more easily -- by checking if rounding the quotient to 16 decimal places was an integer. i.e. in perl: return 1 if sprintf("%0.16f", $quotient) !~ /\./'

@watuwo
Copy link

watuwo commented Nov 19, 2022

There is a difference between the 0.0075|0.0001 and the 0.3|0.1 test that I consider relevant:

Number.isSafeInteger(0.0075 / 0.0001)  // is `true` <-- !!!
0.0075 % 0.0001 === 0 // is `false` 
Number.isSafeInteger(0.3 / 0.1) // is `false`
0.3 % 0.1 === 0 // is `false`

Seeing how much confusion (and actual interoperability problems) is caused by the "multipleOf" keyword (many issues in the specification repository, the test suit repository and implementation repositories) and seeing how easily I could come up with a naive incorrect implementation that would still pass the test, as a schema author I would not want to use the "multipleOf" keyword with anything else then a small-ish integer.

We need as many tests for this keywords as we can get!

@karenetheridge
Copy link
Member

0.0075 % 0.0001 === 0 // is false

The modulo operator is not valid for non-integers on the RHS. Attempts to implement the multipleOf keyword using solely the modulo operator are incorrect.

@watuwo
Copy link

watuwo commented Nov 21, 2022

Number.isSafeInteger(a / b) is also an incorrect implementation of "multipleOf". The problem is that it passes the test!

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.

None yet

4 participants