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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
