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

Is negative zero a default value for float/double fields? #7062

Open
tbkka opened this issue Jan 6, 2020 · 18 comments
Open

Is negative zero a default value for float/double fields? #7062

tbkka opened this issue Jan 6, 2020 · 18 comments

Comments

@tbkka
Copy link

tbkka commented Jan 6, 2020

This question recently came up in discussion of apple/swift-protobuf#935:

Should proto3 implementations serialize float/double values of -0.0?

Proto3 omits any field whose value is the default. For float and double fields, this means that zero values are not serialized. Does this apply to both negative and positive zero? Or should negative zero be serialized in order to ensure that all float and double values can be accurately communicated?

@tbkka
Copy link
Author

tbkka commented Jan 6, 2020

The desired behavior should probably be included in conformance tests for binary, text format, and JSON serializations.

@acozzette
Copy link
Member

CC @gerben-s who was looking into this problem a couple months ago

@elharo
Copy link
Contributor

elharo commented Sep 13, 2021

While we're at it, I wonder what we do with NaN and Inf?

@elharo elharo added the json label Sep 13, 2021
@fmeum
Copy link

fmeum commented Apr 17, 2023

We ran into this issue while working on a Java-based mutator for Protobuf messages and have to rely on rather hacky workarounds. Is there a plan to settle this edge case?

@tbkka
Copy link
Author

tbkka commented Apr 17, 2023

While we're at it, I wonder what we do with NaN and Inf?

NaN and Inf are transferred correctly in all cases already. There's no problem there. The problem with -0.0 is that it tests equal to +0.0. So the question here is really whether a field that has the value -0.0:

  • Should be transferred because it is not the same as the default +0.0
  • Should not be transferred because it is equal to the default +0.0

Or put different, is the default +0.0 or is it "any double value that is equal to +0.0"?

@fmeum
Copy link

fmeum commented Apr 17, 2023

While 0.0 == -0.0 in Java, Double.compare(0.0, -0.0) returns 1, not 0, so this even depends on the kind of equality test.

@fowles
Copy link
Contributor

fowles commented Apr 17, 2023

Inf, and -Inf are guaranteed to round trip.
NaN will remain NaN but the exact bit pattern of which NaN is not guaranteed.
-0.0 and 0.0 are treated as equivalent, so an explicitly set -0.0 will not be serialized in proto3. If you read wire format that contains a -0.0, you may or may not see if from an accessor.

In other words, all NaN values form an equivalence class. Similarly -0.0 and 0.0 form an equivalence class. After serialization and deserialization it, which specific value of an equivalence class is observed is unspecified.

@fowles
Copy link
Contributor

fowles commented Apr 17, 2023

Over to @Logofile to work into our official docs

@fmeum
Copy link

fmeum commented Apr 21, 2023

There is an important case in which -0.0 and 0.0 are not treated as equivalent: When comparing proto message objects, at least in Java. As it stands, two proto message objects with float32 proto fields, one set to 0.0 and one set to -0.0, will not test equal in Java, but will do so after having been written to bytes and parsed back.

Is this still expected behavior? I would have expected equals semantics in Java to be identical to be preserved under a serialization/deserialization roundtrip.

@fowles
Copy link
Contributor

fowles commented Apr 21, 2023

I would have expected that as well. Honestly, this particular corner of our semantics is likely under covered in tests

@bilbothebaggins
Copy link

bilbothebaggins commented Dec 18, 2023

Let me add a quick quote from the specs here: https://protobuf.dev/programming-guides/encoding/

i64        := sfixed64 | fixed64 | double;
                encoded as 8-byte little-endian;
                memcpy of the equivalent C types (u?int64_t, double)

If we take this part of the spec at face value memcpy of the equivalent C types ... double) than all the behaviors that do not preserve the bit pattern are "wrong".

Edit: However, it would appear that this is orthogonal to the default-values-are-not-serialized rule.

Also, in Dec 2023 notes have been added to the spec (ln 522, 539) to clarify the +/- 0.0 behavior: https://protobuf.dev/programming-guides/proto3/#default :

... For float and double types, -0.0 and 0.0 are treated as equivalent

... Also note that if a scalar message field is set to its default, the value will not be serialized on the wire. If a float or double value is set to -0 or +0, it will not be serialized.

Note that any repeated double [packed] fields will have no "default value handling" (even in the unpacked ones I couldn't observe it), and so you can transfer a -0.0 IEEEE double iff you put it into a repeated field.

@mumbleskates
Copy link
Contributor

As of 2024-02-13, this was changed:

Also note that if a scalar message field is set to its default, the value will not be serialized on the wire. If a float or double value is set to +0 it will not be serialized, but -0 is considered distinct and will be serialized.

I cannot tell if or when this has become true in which official protobuf libraries, or if or when this has existed in any part of the conformance tests. There have been at least some unit tests since 2021. it also seems to work in c++ at present...

Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jul 21, 2024
@bilbothebaggins
Copy link

This issue should be closed as "answered"?

There is the commit protocolbuffers/protocolbuffers.github.io@025f350 from that changed the current docs to the quote already mentioned above:

... for scalar message fields, once a message is parsed there’s no way of telling whether a field was explicitly set to the default value ... If a float or double value is set to +0 it will not be serialized, but -0 is considered distinct and will be serialized.

@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jul 31, 2024
@Logofile
Copy link
Member

As mentioned, the text "For float and double types, -0.0 and 0.0 are treated as equivalent, and will round-trip" and "[i]f a
float or double value is set to -0 or +0, it will not be serialized" were added to the documentation topic proto3.md. If this doesn't fully address the documentation issue, please let me know.

@tbkka
Copy link
Author

tbkka commented Jul 31, 2024

Ummm.... so which is it?

The current docs say:

If a float or double value is set to +0 it will not be serialized, but -0 is considered distinct and will be serialized.

but

@Logofile just above claimed

... "[i]f a float or double value is set to -0 or +0, it will not be serialized"

@Logofile
Copy link
Member

Apologies: that additional text was removed in a later change. (I was looking at an earlier version of the proto3.md file when I added my last comment.) We removed the text "For float and double types, -0.0 and 0.0 are treated as equivalent, and will round-trip" based on guidance from @esrauchg.

If a float or double value is set to +0 it will not be serialized, but -0 will be serialized.

@Logofile Logofile reopened this Jul 31, 2024
@tbkka
Copy link
Author

tbkka commented Jul 31, 2024

Excellent! I think that's the behavior that most folks on this thread seem to have expected. Now that we have it documented, we can make that consistent across implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants