You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "gohalo (via GitHub)" <gi...@apache.org> on 2024/08/31 02:57:25 UTC
[PR] [wip] refact: define Hudi error types across hudi-core [hudi-rs]
gohalo opened a new pull request, #124:
URL: https://github.com/apache/hudi-rs/pull/124
## Description
Use `thiserror` instead of `anyhow` to define the error for hudi-core crate.
Currently, work in progress.
#72
<!--- Describe your changes in detail -->
<!--- If it fixes an open issue, please link to the issue here. -->
<!--- Please link any related issues and PRs as well. -->
## How are the changes test-covered
- [x] N/A
- [ ] Automated tests (unit and/or integration tests)
- [ ] Manual tests
- [ ] Details are described below
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan merged PR #124:
URL: https://github.com/apache/hudi-rs/pull/124
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refact: define Hudi error types across hudi-core [hudi-rs]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2342650504
## [Codecov](https://app.codecov.io/gh/apache/hudi-rs/pull/124?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `71.30435%` with `33 lines` in your changes missing coverage. Please review.
> Project coverage is 86.80%. Comparing base [(`01ef2fc`)](https://app.codecov.io/gh/apache/hudi-rs/commit/01ef2fc196b77a4a55a8d2f6b6f74fd011938968?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`fb6ff8e`)](https://app.codecov.io/gh/apache/hudi-rs/commit/fb6ff8efa378567687657b57bcef37bec7754c71?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi-rs/pull/124?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [crates/core/src/table/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL21vZC5ycw==) | 33.33% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/storage/utils.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fstorage%2Futils.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3N0b3JhZ2UvdXRpbHMucnM=) | 56.25% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/storage/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fstorage%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3N0b3JhZ2UvbW9kLnJz) | 78.94% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [python/src/internal.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=python%2Fsrc%2Finternal.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHl0aG9uL3NyYy9pbnRlcm5hbC5ycw==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/config/internal.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fconfig%2Finternal.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL2NvbmZpZy9pbnRlcm5hbC5ycw==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/config/table.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fconfig%2Ftable.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL2NvbmZpZy90YWJsZS5ycw==) | 93.54% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/file\_group/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ffile_group%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL2ZpbGVfZ3JvdXAvbW9kLnJz) | 71.42% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/table/fs\_view.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Ffs_view.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL2ZzX3ZpZXcucnM=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/table/timeline.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Ftimeline.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL3RpbWVsaW5lLnJz) | 91.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #124 +/- ##
==========================================
- Coverage 87.77% 86.80% -0.98%
==========================================
Files 14 15 +1
Lines 728 773 +45
==========================================
+ Hits 639 671 +32
- Misses 89 102 +13
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/hudi-rs/pull/124?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "gohalo (via GitHub)" <gi...@apache.org>.
gohalo commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2510366661
> @gohalo do you have time to resume work on this? in case you're occupied, maybe @TheR1sing3un could take this over? as this is more of a foundational work, we want to move forward sooner, ideally land this within the next 7 days. thanks!
working on that, still have some conflicts to fix now
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "gohalo (via GitHub)" <gi...@apache.org>.
gohalo commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2489916812
OK, will fix that next week
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2488734013
@gohalo any plan to resume the work here by resolving conflicts first? we've cut release branch so master won't have much changes for some time.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#discussion_r1870481178
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
+ #[error("Config '{0}' not found")]
+ ConfNotFound(String),
+
+ #[error("Invalid config item '{item}', {source:?}")]
+ InvalidConf {
+ item: &'static str,
+ source: Box<dyn std::error::Error + Sync + Send + 'static>,
+ },
+
+ #[error("Parse url '{url}' failed, {source}")]
+ UrlParse {
+ url: String,
+ source: url::ParseError,
+ },
+
+ #[error("Invalid file path '{name}', {detail}")]
+ InvalidPath { name: String, detail: String },
+
+ #[error("{0}")]
+ Unsupported(String),
+
+ #[error("{0}")]
Review Comment:
this will be overwritten in a follow up.
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
+ #[error("Config '{0}' not found")]
+ ConfNotFound(String),
+
+ #[error("Invalid config item '{item}', {source:?}")]
+ InvalidConf {
+ item: &'static str,
+ source: Box<dyn std::error::Error + Sync + Send + 'static>,
+ },
+
+ #[error("Parse url '{url}' failed, {source}")]
+ UrlParse {
+ url: String,
+ source: url::ParseError,
+ },
+
+ #[error("Invalid file path '{name}', {detail}")]
+ InvalidPath { name: String, detail: String },
+
+ #[error("{0}")]
Review Comment:
this will be overwritten in a follow up.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#discussion_r1868692161
##########
crates/core/src/table/partition.rs:
##########
@@ -196,21 +198,17 @@ pub struct PartitionFilter {
}
impl TryFrom<((&str, &str, &str), &Schema)> for PartitionFilter {
- type Error = anyhow::Error;
+ type Error = crate::Error;
fn try_from((filter, partition_schema): ((&str, &str, &str), &Schema)) -> Result<Self> {
let (field_name, operator_str, value_str) = filter;
- let field: &Field = partition_schema
- .field_with_name(field_name)
- .with_context(|| format!("Field '{}' not found in partition schema", field_name))?;
+ let field: &Field = partition_schema.field_with_name(field_name)?;
- let operator = Operator::from_str(operator_str)
- .with_context(|| format!("Unsupported operator: {}", operator_str))?;
+ let operator = Operator::from_str(operator_str)?;
Review Comment:
I think we should implement a cast error
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#discussion_r1870472362
##########
crates/core/src/storage/mod.rs:
##########
@@ -293,10 +299,6 @@ mod tests {
result.is_err(),
"Should return error when no base path is invalid."
);
- assert!(result
- .unwrap_err()
- .to_string()
- .contains("Failed to create storage"));
Review Comment:
should check error type instead of removing the test case
##########
crates/core/src/config/read.rs:
##########
@@ -74,11 +79,16 @@ impl ConfigParser for HudiReadConfig {
let get_result = configs
.get(self.as_ref())
.map(|v| v.as_str())
- .ok_or(anyhow!("Config '{}' not found", self.as_ref()));
+ .ok_or(ConfNotFound(self.as_ref().to_string()));
Review Comment:
```suggestion
.ok_or(ConfigNotFound(self.as_ref().to_string()));
```
##########
crates/core/src/config/read.rs:
##########
@@ -103,7 +112,7 @@ mod tests {
fn parse_invalid_config_value() {
let options = HashMap::from([(InputPartitions.as_ref().to_string(), "foo".to_string())]);
let value = InputPartitions.parse_value(&options);
- assert!(value.err().unwrap().is::<ParseIntError>());
+ assert!(value.is_err());
Review Comment:
we should not relax the test case, esp. when we have custom error type
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2518881355
I'll work on a follow up PR to get the coverage level back. merging it now.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refact: define Hudi error types across hudi-core [hudi-rs]
Posted by "gohalo (via GitHub)" <gi...@apache.org>.
gohalo commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2340632727
@xushiyan @codope
Hope this codes could be reviewed as soon as possible, because it refact the core crate, and almost every new merged code should refact again. 😊
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2515350118
> @xushiyan the conflicts have fixed, i will check the coverage recently, you could review the code first.
Thanks for the fix! will go through this today.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on code in PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#discussion_r1870476347
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
Review Comment:
enum is the right way to maintain custom error types using thiserror https://docs.rs/thiserror/latest/thiserror/#example
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "gohalo (via GitHub)" <gi...@apache.org>.
gohalo commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2511421421
@xushiyan just blocked by the following code [crates/core/src/table/fs_view.rs#L134](https://github.com/gohalo/hudi-rs/blob/8413a3a0f9e3cf1e7f174be7172321ac22b36d7a/crates/core/src/table/fs_view.rs#L134), do you have any ideas how to fix this one.
The following code just works fine.
``` rust
use futures::stream::{StreamExt, TryStreamExt};
use std::collections::HashSet;
use std::time::Duration;
use tokio::time::sleep;
#[derive(Debug)]
pub enum Error {
Internal(String),
}
type Result<T, E = Error> = std::result::Result<T, E>;
async fn hello(idx: usize) -> Result<Vec<usize>> {
println!("Hello idx={idx}");
sleep(Duration::from_secs(1)).await;
Ok((idx..idx + 3).collect())
}
#[tokio::main]
async fn main() {
let value: HashSet<usize> = vec![1usize, 2, 3].into_iter().collect();
//let data = futures::stream::iter(1..5)
let data = futures::stream::iter(value)
.map(|i| async move {
let data = hello(i).await?;
Ok::<_, Error>(data)
})
.buffer_unordered(10) // .buffered(10)
.try_for_each(|c| async move {
println!("{:?}", c);
Ok(())
})
.await;
println!("{:?}", data);
}
```
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "gohalo (via GitHub)" <gi...@apache.org>.
gohalo commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2513731574
@xushiyan the conflicts have fixed, i will check the coverage recently, you could review the code first.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#discussion_r1868690611
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
+ #[error("Config '{0}' not found")]
+ ConfNotFound(String),
+
+ #[error("Invalid config item '{item}', {source:?}")]
+ InvalidConf {
+ item: &'static str,
+ source: Box<dyn std::error::Error + Sync + Send + 'static>,
+ },
+
+ #[error("Parse url '{url}' failed, {source}")]
+ UrlParse {
+ url: String,
+ source: url::ParseError,
+ },
+
+ #[error("Invalid file path '{name}', {detail}")]
+ InvalidPath { name: String, detail: String },
+
+ #[error("{0}")]
+ Unsupported(String),
+
+ #[error("{0}")]
Review Comment:
```suggestion
#[error("Internal Error. {0}")]
```
Consistent context
##########
crates/core/src/table/partition.rs:
##########
@@ -196,21 +198,17 @@ pub struct PartitionFilter {
}
impl TryFrom<((&str, &str, &str), &Schema)> for PartitionFilter {
- type Error = anyhow::Error;
+ type Error = crate::Error;
fn try_from((filter, partition_schema): ((&str, &str, &str), &Schema)) -> Result<Self> {
let (field_name, operator_str, value_str) = filter;
- let field: &Field = partition_schema
- .field_with_name(field_name)
- .with_context(|| format!("Field '{}' not found in partition schema", field_name))?;
+ let field: &Field = partition_schema.field_with_name(field_name)?;
- let operator = Operator::from_str(operator_str)
- .with_context(|| format!("Unsupported operator: {}", operator_str))?;
+ let operator = Operator::from_str(operator_str)?;
Review Comment:
I think we should implement a cast error
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
Review Comment:
Should we keep it like this long term or define this as a struct with an enum for the error type, impl display, etc? @xushiyan @gohalo
##########
crates/core/src/lib.rs:
##########
@@ -48,3 +48,49 @@ pub mod file_group;
pub mod storage;
pub mod table;
pub mod util;
+
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {
+ #[error("Config '{0}' not found")]
+ ConfNotFound(String),
+
+ #[error("Invalid config item '{item}', {source:?}")]
+ InvalidConf {
+ item: &'static str,
+ source: Box<dyn std::error::Error + Sync + Send + 'static>,
+ },
+
+ #[error("Parse url '{url}' failed, {source}")]
+ UrlParse {
+ url: String,
+ source: url::ParseError,
+ },
+
+ #[error("Invalid file path '{name}', {detail}")]
+ InvalidPath { name: String, detail: String },
+
+ #[error("{0}")]
Review Comment:
```suggestion
#[error("Operation is not supported. {0}")]
```
Add more context
##########
crates/core/src/config/table.rs:
##########
@@ -199,13 +236,13 @@ pub enum TableTypeValue {
}
impl FromStr for TableTypeValue {
- type Err = anyhow::Error;
+ type Err = Error;
fn from_str(s: &str) -> Result<Self> {
match s.to_ascii_lowercase().as_str() {
"copy_on_write" | "copy-on-write" | "cow" => Ok(Self::CopyOnWrite),
"merge_on_read" | "merge-on-read" | "mor" => Ok(Self::MergeOnRead),
- _ => Err(anyhow!("Unsupported table type: {}", s)),
+ v => Err(Unsupported(format!("unsupported table type {}", v))),
Review Comment:
```suggestion
v => Err(Unsupported(format!("Unsupported table type {}", v))),
```
Small nit
##########
crates/core/src/config/table.rs:
##########
@@ -218,12 +255,12 @@ pub enum BaseFileFormatValue {
}
impl FromStr for BaseFileFormatValue {
- type Err = anyhow::Error;
+ type Err = Error;
fn from_str(s: &str) -> Result<Self> {
match s.to_ascii_lowercase().as_str() {
"parquet" => Ok(Self::Parquet),
- _ => Err(anyhow!("Unsupported base file format: {}", s)),
+ v => Err(Unsupported(format!("unsupported base file format {}", v))),
Review Comment:
```suggestion
v => Err(Unsupported(format!("Unsupported base file format {}", v))),
```
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: define Hudi error types across hudi-core [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan commented on PR #124:
URL: https://github.com/apache/hudi-rs/pull/124#issuecomment-2509327550
@gohalo do you have time to resume work on this? in case you're occupied, maybe @TheR1sing3un could take this over? as this is more of a foundational work, we want to move forward sooner. thanks!
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org