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

Don't Derive Serialize/Deserialize Serde Implementations for Schema Types #2723

Closed
wants to merge 1 commit into from

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Related to #2300
Part of #2594
Related to #2711

Rationale for this change

Serde derive is a non-trivial proc macro dependency, and the nature of the orphan rule forces the Serialize/Deserialize implementations to be defined in the same crate. This in turn forces json to leak into an arrow-schema crate, see #2711.

It turns out we aren't actually using the generated code, although downstreams potentially are.

What changes are included in this PR?

Removes the serde derives, ensuring we only provide a single way to serialize a schema to JSON, and potentially allowing this to live in a separate arrow-json crate (#2594)

Are there any user-facing changes?

Yes, theoretically users may have been using these serialize implementations. I'm not sure this duplication was intentional.

@tustvold tustvold added the api-change Changes to the arrow API label Sep 13, 2022
@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 13, 2022
@tustvold tustvold changed the title Remove use of serde derive Don't Derive Serialize/Deserialize Serde Implementations for Schema Types Sep 13, 2022
@tustvold
Copy link
Contributor Author

tustvold commented Sep 13, 2022

Ok I'm rather confused by this, DataType::from doesn't match the format of DataType::to_json for nested types. Perhaps we ought to remove the to_json, etc... methods instead 🤔

Edit: It would appear these relate to the integration test plumbing - https://github.com/apache/arrow/blob/0b67673d045bf6e3523277f6802be126661e3001/docs/source/format/Integration.rst#json-test-data-format

@tustvold
Copy link
Contributor Author

Closing in favor of #2724

@tustvold tustvold closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant