You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AdamGS (via GitHub)" <gi...@apache.org> on 2025/10/06 10:41:49 UTC
[PR] Replace serde with `serde_core` when possible [arrow-rs]
AdamGS opened a new pull request, #8558:
URL: https://github.com/apache/arrow-rs/pull/8558
# Which issue does this PR close?
- Closes #8451.
With this change, its possible to compile the core crate without pulling `serde_derive`, which is will be only required for `arrow-avro` and `arrow-schema/serde`.
# Rationale for this change
Improve compile time and reduce number of dependencies and binary size in some cases.
# What changes are included in this PR?
1. Use `serde_core` when possible
2. Manually implement `Serialize/Deserialize` for canonical extension type metadata.
# Are these changes tested?
Covered by existing tests
# Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace serde with `serde_core` when possible [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8558:
URL: https://github.com/apache/arrow-rs/pull/8558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace serde with `serde_core` when possible [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #8558:
URL: https://github.com/apache/arrow-rs/pull/8558#discussion_r2405693299
##########
arrow-schema/src/extension/canonical/fixed_shape_tensor.rs:
##########
@@ -141,6 +144,144 @@ pub struct FixedShapeTensorMetadata {
permutations: Option<Vec<usize>>,
}
+impl Serialize for FixedShapeTensorMetadata {
+ fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+ where
+ S: Serializer,
+ {
+ let mut state = serializer.serialize_struct("FixedShapeTensorMetadata", 3)?;
+ state.serialize_field("shape", &self.shape)?;
+ state.serialize_field("dim_names", &self.dim_names)?;
+ state.serialize_field("permutations", &self.permutations)?;
+ state.end()
+ }
+}
+
+impl<'de> Deserialize<'de> for FixedShapeTensorMetadata {
+ fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+ where
+ D: Deserializer<'de>,
+ {
+ #[derive(Debug)]
Review Comment:
my guess is that this is auto-generated code. I personally would prefer if we wouldn't nest structs/enums/impls/etc. into methods. It makes the code rather hard to read (for auto-generated code there are usually good reasons why this is done this way, e.g. due to shadowing).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace serde with `serde_core` when possible [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #8558:
URL: https://github.com/apache/arrow-rs/pull/8558#issuecomment-3376366349
This currently is only semi-effective until https://github.com/BurntSushi/rust-csv/pull/403 is merged and released. But if we try this upstream change by adding the following to `Cargo.toml`:
```toml
[patch.crates-io]
csv = {git = "https://github.com/paolobarbolini/rust-csv.git", branch = "switch-to-serde-core"}
```
and then we get all crates for a full-featured `arrow` (but without dev dependencies because `criterion` uses derive-based serialization):
```console
$ cargo tree -p arrow --all-features -e=no-dev
```
then there's no `serde_derive` left and no `syn` and no `proc-macro2`. So I would call this a win.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace serde with `serde_core` when possible [arrow-rs]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #8558:
URL: https://github.com/apache/arrow-rs/pull/8558#discussion_r2405697896
##########
arrow-schema/src/extension/canonical/fixed_shape_tensor.rs:
##########
@@ -141,6 +144,144 @@ pub struct FixedShapeTensorMetadata {
permutations: Option<Vec<usize>>,
}
+impl Serialize for FixedShapeTensorMetadata {
+ fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+ where
+ S: Serializer,
+ {
+ let mut state = serializer.serialize_struct("FixedShapeTensorMetadata", 3)?;
+ state.serialize_field("shape", &self.shape)?;
+ state.serialize_field("dim_names", &self.dim_names)?;
+ state.serialize_field("permutations", &self.permutations)?;
+ state.end()
+ }
+}
+
+impl<'de> Deserialize<'de> for FixedShapeTensorMetadata {
+ fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+ where
+ D: Deserializer<'de>,
+ {
+ #[derive(Debug)]
Review Comment:
Definitely autogenerated-inspired, I'll pull all the inner definition outside.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace serde with `serde_core` when possible [arrow-rs]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #8558:
URL: https://github.com/apache/arrow-rs/pull/8558#discussion_r2407865283
##########
arrow-schema/src/extension/canonical/fixed_shape_tensor.rs:
##########
@@ -141,6 +144,144 @@ pub struct FixedShapeTensorMetadata {
permutations: Option<Vec<usize>>,
}
+impl Serialize for FixedShapeTensorMetadata {
+ fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+ where
+ S: Serializer,
+ {
+ let mut state = serializer.serialize_struct("FixedShapeTensorMetadata", 3)?;
+ state.serialize_field("shape", &self.shape)?;
+ state.serialize_field("dim_names", &self.dim_names)?;
+ state.serialize_field("permutations", &self.permutations)?;
+ state.end()
+ }
+}
+
+impl<'de> Deserialize<'de> for FixedShapeTensorMetadata {
+ fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+ where
+ D: Deserializer<'de>,
+ {
+ #[derive(Debug)]
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org