Skip to content

Register format decimal128 #3209

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

Merged
merged 3 commits into from
Mar 30, 2023
Merged

Register format decimal128 #3209

merged 3 commits into from
Mar 30, 2023

Conversation

ralfhandl
Copy link
Contributor

@ralfhandl ralfhandl commented Mar 22, 2023

Format for 128-bit decimal floating-point numbers as proposed in proposal in #3190 (comment).

Fixes #603 with proposal for representing NaN, -INF, and INF.

@ralfhandl
Copy link
Contributor Author

@baywet @handrews please review

owner:
issue:
description: A decimal floating-point number with 34 significant decimal digits
base_type: string number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we only have string here? or at least warn about a loss of accuracy in case of JSON serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a well-known problem for some serializers and parsers, and already called out in https://www.rfc-editor.org/rfc/rfc8259#section-6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralfhandl is this string, number as in you can use either, or do you mean string number as in "a string containing a number"? If the latter, I would just say string. If the former, I suggest the comma for clarity 🧐

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good callout for clarity. If we change this, we should also change it in the infamous decimal format

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralfhandl Are you referring to this line:

implementations will approximate JSON numbers within the expected precision

I don't think that RFC 8259 wording is particularly clear about potential loss of precision when storing decimals as numbers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darrelmiller @ralfhandl we would probably want to reference https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-00#section-4.2.1 which includes:

Whitespace and formatting concerns, including different lexical
representations of numbers that are equal within the data model, are
thus outside the scope of JSON Schema. JSON Schema vocabularies
(Section 8.1) that wish to work with such differences in lexical
representations SHOULD define keywords to precisely interpret
formatted strings within the data model rather than relying on having
the original JSON representation Unicode characters available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@handrews It's "type": ["string", "number"], we use content negotiation to allow clients select whether the server will send JSON numbers or wrap the numbers as JSON strings.

Copy link
Contributor

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the type for decimal as well please for consistency?

@ralfhandl
Copy link
Contributor Author

@baywet Done in separate PR #3210

@darrelmiller
Copy link
Member

  1. No objection to adding support for both string and number
  2. We should add a warning about the risks of using number and point to related spec
  3. For decimal serialized as strings, call out how things such as inf and Nan are supported.

@ralfhandl ralfhandl requested review from darrelmiller, handrews and baywet and removed request for darrelmiller, handrews and baywet March 23, 2023 17:23
@ralfhandl
Copy link
Contributor Author

ralfhandl commented Mar 23, 2023

Strange: I can only re-request a review from a single reviewer, trying to re-request for a second reviewer removes the re-request for the first reviewer 🤔

@darrelmiller darrelmiller merged commit dace7d0 into OAI:gh-pages Mar 30, 2023
@ralfhandl ralfhandl deleted the format/decimal128 branch April 5, 2023 13:20
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.

4 participants