feat: Implement ser/de for the remaining all#58
Conversation
XuQianJin-Stars
force-pushed
the
deserialize-type
branch
from
5c367b8 to
4b51b90
Compare
| 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.
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,
})
}
}| type Err = Error; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let parts: Vec<&str> = s.split_whitespace().collect(); |
| } | ||
| .fail(); | ||
| }; | ||
| let Some(close_bracket) = s.find(')') else { |
There was a problem hiding this comment.
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.
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. |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
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.
Aitozi
left a comment
Aitozi
left a comment
There was a problem hiding this comment.
LGTM, thanks @XuQianJin-Stars for your updates
3 participants
No description provided.