-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Fix JSON orient='table' issues with numeric column names #25488
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
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e6f7969
Tests to be fixed
c88affc
Fix JSON orient='table' issues for numeric column names (#19129) (#22…
e7abdb1
Fix tests
11930e5
Merge branch 'master' into fix-19129
953c128
Merge branch 'master' into fix-19129
82d486e
Change JSON schema version number
e183094
Fix typo table='orient' to orient='table'
3332fec
Parse table schema according to the version
36d9350
Set values as default orient for JSONTableWriter
3123dc6
Fix minor typos
3345119
Test read_json for table generated by version 0.20.0
1465e39
Merge branch 'master' into fix-19129
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't know that we want to do this. Is it valid JSON in the table spec to have column names that are non-string?
Understood you have gotten this to round trip but if it violates the Table spec for JSON then I'd rather raise as commented previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd as I already commented in #19129, after having tried to start a discussion before doing the PR:
Could you please tell me where the JSON table spec claims that column names MUST be strings?
I am going to make a longer comment to further justify my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to the table spec as much as just JSON itself. See the description of an object here:
https://json.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd you are talking about different things:
And my question is: where the JSON table spec claims that COLUMN NAMES (not JSON object keys) must be strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwalsh can you comment on this? Is the
name
field in a Field Descriptor expected to be a string?