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

serde rc feature might not be needed? #989

Closed
carols10cents opened this issue Dec 1, 2021 · 1 comment · Fixed by #990
Closed

serde rc feature might not be needed? #989

carols10cents opened this issue Dec 1, 2021 · 1 comment · Fixed by #990
Labels

Comments

@carols10cents
Copy link
Contributor

This is low priority, and I'm happy to send in a PR for this if it's determined we can remove the rc feature.

I'm analyzing dependencies and features in IOx, which is also leading me to analyze dependencies and features in IOx's dependencies.

I noticed arrow enables serde's rc feature:

serde = { version = "1.0", features = ["rc"] }

serde's docs say:

--features rc

Opt into impls for Rc and Arc. Serializing and deserializing these types does not preserve identity and may result in multiple copies of the same data. Be sure that this is what you want before enabling this feature.

Serializing a data structure containing reference-counted pointers will serialize a copy of the inner value of the pointer each time a pointer is referenced within the data structure. Serialization will not attempt to deduplicate these repeated data.

Deserializing a data structure containing reference-counted pointers will not attempt to deduplicate references to the same data. Every deserialized pointer will end up with a strong count of 1.

The feature was added in this PR made 3 years ago. It adds derive(Serialize, Deserialize) to DataType, Field, and Schema, none of which contain any Rc<T> or Arc<T> as far as I can tell. It only adds a test for a DataType::Struct that contains Fields, but Schema contains Fields too, so I think that coverage is enough.

I tried taking the rc feature out and arrow's tests still pass.

@andygrove I don't suppose you remember why you enabled the rc feature of serde in a PR made 3 years ago? 😂

@houqp
Copy link
Member

houqp commented Dec 2, 2021

if it builds without rc and passes all tests, then I don't see a reason to keep it :D

carols10cents added a commit to integer32llc/arrow-rs that referenced this issue Dec 2, 2021
Fixes apache#989.

This feature opts into impls for `Rc` and `Arc`, but none of the data
structures that use Serialize/Deserialize actually contain `Rc` or
`Arc`s.

See:

- [Serde docs](https://serde.rs/feature-flags.html#-features-rc)
- [PR adding this](apache/arrow#3016)
carols10cents added a commit to integer32llc/arrow-rs that referenced this issue Dec 3, 2021
Fixes apache#989.

This feature opts into impls for `Rc` and `Arc`, but none of the data
structures that use Serialize/Deserialize actually contain `Rc` or
`Arc`s.

See:

- [Serde docs](https://serde.rs/feature-flags.html#-features-rc)
- [PR adding this](apache/arrow#3016)
@alamb alamb closed this as completed in #990 Dec 3, 2021
alamb pushed a commit that referenced this issue Dec 3, 2021
Fixes #989.

This feature opts into impls for `Rc` and `Arc`, but none of the data
structures that use Serialize/Deserialize actually contain `Rc` or
`Arc`s.

See:

- [Serde docs](https://serde.rs/feature-flags.html#-features-rc)
- [PR adding this](apache/arrow#3016)
alamb pushed a commit that referenced this issue Dec 9, 2021
Fixes #989.

This feature opts into impls for `Rc` and `Arc`, but none of the data
structures that use Serialize/Deserialize actually contain `Rc` or
`Arc`s.

See:

- [Serde docs](https://serde.rs/feature-flags.html#-features-rc)
- [PR adding this](apache/arrow#3016)
alamb added a commit that referenced this issue Dec 9, 2021
Fixes #989.

This feature opts into impls for `Rc` and `Arc`, but none of the data
structures that use Serialize/Deserialize actually contain `Rc` or
`Arc`s.

See:

- [Serde docs](https://serde.rs/feature-flags.html#-features-rc)
- [PR adding this](apache/arrow#3016)

Co-authored-by: Carol (Nichols || Goulding) <193874+carols10cents@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants