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 int_value in google.protobuf.struct Value #10255

Closed
ebiebix opened this issue Jul 15, 2022 · 4 comments
Closed

add int_value in google.protobuf.struct Value #10255

ebiebix opened this issue Jul 15, 2022 · 4 comments

Comments

@ebiebix
Copy link

ebiebix commented Jul 15, 2022

In the Message of Value, there is a number_value that defines a double type value.
Is it possible to add something like int_value to distinguish between double and int types?

Currently
double number_value = 2;
at here

could be changed to

double double_value = 2;
int64 int_value = 3;

I would change it to
If possible, we will issue a PR here.

If it is not possible to change it, could you please tell us why you are using double as number_value among the various types of numbers?

@ebiebix ebiebix added the untriaged auto added to all issues by default when created. label Jul 15, 2022
@ebiebix ebiebix changed the title Could you Could you add int_value in Value? Jul 15, 2022
@ebiebix ebiebix changed the title Could you add int_value in Value? add int_value in Value? Jul 15, 2022
@ebiebix ebiebix changed the title add int_value in Value? add int_value in google.protobuf.struct Value Jul 15, 2022
@fowles fowles added enhancement release notes: yes and removed untriaged auto added to all issues by default when created. labels Jul 15, 2022
@fowles
Copy link
Member

fowles commented Jul 15, 2022

On the surface this seems like a pretty reasonable request. Let me dig around a bit internally to see if we had some reason for not including this from the start.

@fowles
Copy link
Member

fowles commented Jul 15, 2022

One awkward thing is that this would change the behavior for all existing users, which is kinda unfortunate.

@mcy
Copy link
Contributor

mcy commented Jul 15, 2022

So let me start by saying that in a vacuum I am 100% in favor of this feature. But...

One key design feature of the JSON wire format is that we do not need to provide the actual parser: rather, we can use an existing parser that produces a map of strings to whatever and turn that into a proto.

JSON made the decision to only have floats as it's data type. Although JSON does not specify this, every implementation ever interprets "float" as "IEEE754 binary64", aka double. Unfortunately, the representable subset of the rationals of binary64 is not a superset of the representable integers of int64. Integers larger than 2^53 will overflow the mantissa, losing sigfigs.

The upshot of all of this is that we cannot parse very large integers as JSON numbers: they must be quoted. This the behavior for integer-types fields today: if they are a 64-bit type, we quote them.

However, we can't do this for Value, because when parsing a Value, all strings are unambiguously strings. We do not try to interpret them (in particular, "NaN" is always a string when parsing a Value, not a double).

These facts are all at odds with each other: the existing serialization rules are too overdetermined to add an int64 field to Value such that:

  • It is is unambiguous to parse.
  • Roundtrips in the face of arbitrary JSON parsers.

As a result, I think we need to close this, since there's no way we can make this work (even breaking changes aside).

NB: Some parsers, like the one in C++, do not reject too-large unquoted integers. This is a bug.

@erinboyle
Copy link

erinboyle commented Jun 17, 2023

What if, instead of migrating Struct and Value, you created new objects that prioritized encoding ints, to the exclusion of some of your other constraints? Developers could choose between Struct and StructWithInt. I'd love to use this for dicts in python. I'm doing a sad json.dumps / json.loads of my dicts into/out of proto to keep their integers.

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

5 participants