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

Super-fy the Super JSON format doc #5386

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Super-fy the Super JSON format doc #5386

merged 2 commits into from
Oct 30, 2024

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Oct 29, 2024

In the same spirit as #5368.

@philrz philrz self-assigned this Oct 29, 2024
@@ -149,7 +149,7 @@ conforming to any floating point representation that cannot be
interpreted as an integer, e.g., `1.` or `1.0` instead of
`1` or `1e3` instead of `1000`. Unlike JSON, a floating point number can
also be one of:
`Inf`, `+Inf`, `-Inf`, or `Nan`.
`Inf`, `+Inf`, `-Inf`, or `NaN`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out case is important here.

$ super -version
Version: v1.18.0-69-g5ab9615c

$ echo '1 NaN 2' | super -z -c 'yield typeof(this)' -
<int64>
<float64>
<int64>

The following causes a syntax error, which makes sense to me:

$ echo '1 Nan 2' | super -z -c 'yield typeof(this)' -
stdio:stdin: Super JSON syntax error

Not sure how to explain this though. A bug?

$ echo 'Nan' | super -z -c 'yield typeof(this)' -
[no output]

Also related, it looks like Inf requires the explicit + or - at the moment.

$ echo '1 +Inf -Inf 2' | super -z -c 'yield typeof(this)' -
<int64>
<float64>
<float64>
<int64>

$ echo '1 Inf 2' | super -z -c 'yield typeof(this)' -
stdio:stdin: Super JSON syntax error

$ echo 'Inf' | super -z -c 'yield typeof(this)' -
[no output]

Whereas NaN seemed like an easy docs fix since that's the way it's shown in Excel/etc., it's not clear to me if we'd rather add support for plain Inf to the grammar or drop it from the spec.

Copy link
Collaborator

@mccanne mccanne Oct 30, 2024

Choose a reason for hiding this comment

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

This is a bug in the jsup parser for any input that is a single identifier then EOF. It shouldn't be too hard to fix.

Copy link
Collaborator

@mccanne mccanne Oct 30, 2024

Choose a reason for hiding this comment

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

Regarding Inf, in IEEE floating point, there's a +Inf and a -Inf. I think we should have one way to represent +Inf in Super JSON and we could change +Inf to Inf. I think we went with +Inf because the golang fmt library prints it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mccanne. I've opened #5391 to track the JSUP parser bug and #5392 to track possibly changing +Inf to just Inf.

@philrz philrz marked this pull request as ready for review October 29, 2024 21:06
@philrz philrz requested a review from a team October 29, 2024 21:06
@philrz philrz merged commit cb5fa71 into main Oct 30, 2024
4 checks passed
@philrz philrz deleted the docs-super-json branch October 30, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants