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

owned::Value and borrowed::Value don't implement std::cmp::Eq #398

Closed
bassmanitram opened this issue Oct 19, 2024 · 8 comments · Fixed by #399
Closed

owned::Value and borrowed::Value don't implement std::cmp::Eq #398

bassmanitram opened this issue Oct 19, 2024 · 8 comments · Fixed by #399
Assignees

Comments

@bassmanitram
Copy link

My continuing attempts to get simd_json "dropped in" as a replacement for serde_json (with some inevitable mutability hacks of course) are stymied at the moment because, while serde::Value implements std::cmp::Eq, the simd_json equivalents do not. Is there a reason for this, or is it simply and oversight?

@bassmanitram bassmanitram changed the title owned::Value and borrowed::Value don't implement std::cmp:Eq owned::Value and borrowed::Value don't implement std::cmp::Eq Oct 19, 2024
@Licenser
Copy link
Member

This concerns floats (the bane of everyone's existence). Floats notoriously do not offer EQ since numbers that should be equal don't always turn out equal but might end up in a tolerance. On the other hand, numbers in that tolerance might not be equal.

@bassmanitram
Copy link
Author

bassmanitram commented Oct 20, 2024 via email

@bassmanitram
Copy link
Author

And there it is - in serde_json Numbers are stored as strings until they are used via "is_xx" and "and_xx" - so Value CAN be Eq. This actually seems to me to be very sensible, since, when encapsulated by Value we are looking at "JSON" values which are always strings. They only become "float" when the caller says "can you be a float, if so gimme a float".

Looking at the simd_approach, that doesn't work - you assume that the user wants the float when you create the Value rather than when the Value is actually "unwrapped". So your comment above is correct - floats can't be strictly Eq. Unfortunately that's a significant blocker to having simd_json be a "fairly easy" droppin replacement for serde_json.

I'm guessing that modifying the approach to Number storage is a no-go ... what about a feature "serde_json" that changes how OwnedValue is implemented ... oh... or a third "type" of Value ... one that is compatible with serde_json (I could work on either of those, just don't want to "waste my time" if you guys don't think it's feasible or desirable).

@bassmanitram
Copy link
Author

OR ... could we store the string value with the float value in order to handle the "equivalence" constraint?

@bassmanitram
Copy link
Author

bassmanitram commented Oct 21, 2024

OR ... https://crates.io/crates/ordered-float (which implies a change to value-trait)

@Kriskras99
Copy link
Contributor

If you just want Eq, you could also just use the f32::total_cmp function (stable since Rust 1.62.0).
The issue I see, is that multiple string representations can give the same float which means Eq would say two fields are equal while they aren't.
Maybe add a feature flag for Eq, so people can opt-in/out

@Licenser
Copy link
Member

Ja strings are not a great option but @bassmanitram's PR for value-trait looks like a really good way to get the best of everything without the need to compromise.

@Licenser Licenser linked a pull request Oct 22, 2024 that will close this issue
@Licenser
Copy link
Member

released as 0.14.2 thanks @bassmanitram !!!

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 a pull request may close this issue.

3 participants