Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Fix JSON export of array dtype #181

Merged
merged 1 commit into from
Jan 16, 2018
Merged

Fix JSON export of array dtype #181

merged 1 commit into from
Jan 16, 2018

Conversation

znation
Copy link
Contributor

@znation znation commented Jan 14, 2018

This fixes the formatting of the flex_vec (array) dtype in CSV export
from SFrame.

Still quite a few bugs lurking in the JSON export -- I also tested
inf/nan handling and it's broken. Leaving that commented out in the test
for now. I suspect other things are broken too (namely datetime or Image
types - not sure how to reasonably JSON export those).

We might want to consider extensions._json for JSON export.

This fixes the formatting of the `flex_vec` (array) dtype in CSV export
from SFrame.

Still quite a few bugs lurking in the JSON export -- I also tested
inf/nan handling and it's broken. Leaving that commented out in the test
for now. I suspect other things are broken too (namely datetime or Image
types - not sure how to reasonably JSON export those).

We might want to consider extensions._json for JSON export.
Copy link
Collaborator

@hoytak hoytak left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I still wonder why the to method of flexible type doesn't just output valid json -- I see no reason why doing that is not the expected behavior. What's the previous difference?

@znation
Copy link
Contributor Author

znation commented Jan 15, 2018

@hoytak I'm not sure what you mean by previous difference. If you think changing the flex_vec::to<std::string> behavior is the better fix I'm happy to do that instead; I figured just changing the CSV writer was the more scoped fix. (Also: most of the types already have their own CSV writer output behavior, alongside the change I just made here; they don't use the to method as is. Should they?)

@ylow
Copy link
Collaborator

ylow commented Jan 16, 2018

Yes. The csv localized fix is better. The flexible_type has never promised JSON compatibility and it is not about to sign up for that.

@znation znation merged commit a577970 into apple:master Jan 16, 2018
@znation znation deleted the json_export_fix branch January 16, 2018 18:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants