-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Implement ser/de for the remaining all #58
Conversation
5c367b8
to
4b51b90
Compare
crates/paimon/src/spec/types.rs
Outdated
type Err = Error; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let parts: Vec<&str> = s.split_whitespace().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, every collect
here involves an allocation. It would be better if we could avoid it.
We can use find
instead of split
and collect
, for example:
impl FromStr for BinaryType {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
if !s.starts_with(serde_utils::BINARY::NAME) {
return DataTypeInvalidSnafu {
message: xxx
}
.fail();
}
let Some(open_bracket) = s.find('(') else {
return DataTypeInvalidSnafu {
message: xxx
}
.fail();
};
let Some(close_bracket) = s.find(')') else {
return DataTypeInvalidSnafu {
message: xxx
}
.fail();
};
if open_bracket >= close_bracket {
return DataTypeInvalidSnafu {
message: xxx
}
.fail();
}
let length_str = &s[open_bracket + 1..close_bracket];
let length = length_str.parse::<usize>().map_err(|_| DataTypeInvalidSnafu {
message: xxx
})?;
let nullable = !s[close_bracket..].contains("NOT NULL");
Ok(BinaryType {
nullable,
length,
})
}
}
crates/paimon/src/spec/types.rs
Outdated
type Err = Error; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let parts: Vec<&str> = s.split_whitespace().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
crates/paimon/src/spec/types.rs
Outdated
} | ||
.fail(); | ||
}; | ||
let Some(close_bracket) = s.find(')') else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about extract a method to extract the (number)
format ? There are almost 8 same usage
type Err = Error; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
if !s.starts_with(serde_utils::TIME::NAME) || s.contains("STAMP") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emm.. I feel here is a bit tricky.
BTW, Can highly_complex_nested_row_type.json
pass this check ? We should also include this in tests ?
The comments on this pull request have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the contribution from @XuQianJin-Stars and @Aitozi's review!
Some implementation details, like the precision_scale_str.split(',').collect::<Vec<&str>>()
, could be improved, but I don't think it's a blocker for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @XuQianJin-Stars for your updates
No description provided.