Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: Align data type with other implementation. #21

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

liurenjie1024
Copy link
Collaborator

This refactor introduces two changes:

  1. Use NestedField in composite types such as struct, map, list to align with java/python implementation.
  2. Simplify type ser/de.

@liurenjie1024
Copy link
Collaborator Author

cc @Fokko @Xuanwo @JanKaul @nastra PTAL

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

This is great! 👍🏻

crates/iceberg/src/spec/datatypes.rs Show resolved Hide resolved
Copy link
Collaborator

@JanKaul JanKaul left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

Comment on lines +95 to +114
impl Serialize for Type {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let type_serde = _serde::SerdeType::from(self);
type_serde.serialize(serializer)
}
}

impl<'de> Deserialize<'de> for Type {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let type_serde = _serde::SerdeType::deserialize(deserializer)?;
Ok(Type::from(type_serde))
}
}

Copy link
Collaborator

@JanKaul JanKaul Aug 4, 2023

Choose a reason for hiding this comment

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

Couldn't we simplify this with:

#[serde(from = "_serde::SerdeType", into = "_serde::SerdeType")]
pub enum Type {
...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, but it seems that the generated code can't contains lifetime parameter. The code duplication is not much, so I prefer to keep current approach.

@Fokko Fokko merged commit 7b29dff into apache:main Aug 4, 2023
7 checks passed
@liurenjie1024 liurenjie1024 deleted the renjie/refactor_nested_field branch August 4, 2023 13:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants