You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "XuQianJin-Stars (via GitHub)" <gi...@apache.org> on 2024/08/17 07:59:31 UTC
[PR] feat: Implement ser/de for the remaining all [paimon-rust]
XuQianJin-Stars opened a new pull request, #58:
URL: https://github.com/apache/paimon-rust/pull/58
(no comment)
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement ser/de for the remaining all [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi commented on code in PR #58:
URL: https://github.com/apache/paimon-rust/pull/58#discussion_r1741003437
##########
crates/paimon/src/spec/types.rs:
##########
@@ -601,6 +765,57 @@ impl Default for LocalZonedTimestampType {
}
}
+impl FromStr for LocalZonedTimestampType {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if !s.starts_with(serde_utils::LocalZonedTimestamp::NAME) || !s.contains("WITH") {
+ return DataTypeInvalidSnafu {
+ message:
+ "Invalid LocalZonedTimestamp type. Expected string to start with 'TIMESTAMP'.",
+ }
+ .fail();
+ }
+
+ let Some(open_bracket) = s.find('(') else {
+ return DataTypeInvalidSnafu {
+ message: "Invalid LocalZonedTimestamp specification. Missing opening bracket.",
+ }
+ .fail();
+ };
+ let Some(close_bracket) = s.find(')') else {
Review Comment:
What about extract a method to extract the `(number)` format ? There are almost 8 same usage
##########
crates/paimon/src/spec/types.rs:
##########
@@ -706,6 +922,55 @@ impl Default for TimeType {
}
}
+impl FromStr for TimeType {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ if !s.starts_with(serde_utils::TIME::NAME) || s.contains("STAMP") {
Review 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 ?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement ser/de for the remaining all [paimon-rust]
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on PR #58:
URL: https://github.com/apache/paimon-rust/pull/58#issuecomment-2328764608
The comments on this pull request have been addressed.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement ser/de for the remaining all [paimon-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #58:
URL: https://github.com/apache/paimon-rust/pull/58#discussion_r1738550665
##########
crates/paimon/src/spec/types.rs:
##########
@@ -223,6 +227,44 @@ impl Default for BinaryType {
}
}
+impl FromStr for BinaryType {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ let parts: Vec<&str> = s.split_whitespace().collect();
Review 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:
```rust
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:
##########
@@ -314,6 +356,44 @@ impl Default for CharType {
}
}
+impl FromStr for CharType {
+ type Err = Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ let parts: Vec<&str> = s.split_whitespace().collect();
Review Comment:
The same.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement ser/de for the remaining all [paimon-rust]
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on PR #58:
URL: https://github.com/apache/paimon-rust/pull/58#issuecomment-2322769698
The comments on this pull request have been addressed.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Implement ser/de for the remaining all [paimon-rust]
Posted by "Aitozi (via GitHub)" <gi...@apache.org>.
Aitozi merged PR #58:
URL: https://github.com/apache/paimon-rust/pull/58
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org