Add more variants to `ErrorKind`

[connortsui20]

Is your feature request related to a problem or challenge?

As of now, ErrorKind looks like this (doc comments removed):

pub enum ErrorKind {
    Unexpected,
    DataInvalid,
    FeatureUnsupported,
}

This is pretty bare, especially since there are whole classes of things that can go wrong with respect to the Iceberg Catalog trait. See #965 and this comment for some context for this specific repo, as well as the official REST API specification.

It would be nice if there was a dedicated error type for Catalog errors. Some simple things that could be added are variants for tables or namespaces that don't exist, or duplicate tables or namespaces. There are many other things that can go wrong. Right now #965 just uses string messages to represent those errors, which is not ideal.

Adding dedicated error types would also mean the different implementations of the Iceberg Catalog could be more easily unified: the exact return behavior of each method would be the same among every implementation (instead of having a different error message depending on which implementation you choose).

Describe the solution you'd like

I think there are many ways to do this, some better than others. I would say that the current method manually implementing error messages, backtrace, and variants is somewhat shaky. Personally, I think that adding thiserror or snafu as a dependency for this would ease development going forward, not just the error types specified above (for the catalog) but for any other error kinds that Iceberg needs / may introduce.

Willingness to contribute

• I can contribute to this feature independently
• I would be willing to contribute to this feature with guidance from the Iceberg Rust community
• I cannot contribute to this feature at this time

[liurenjie1024]

Thanks @connortsui20 for raising this. I think the motivation behind is that when we add an ErrorKind, we need to think if we could handle it, otherwise we just need to put extra information in context. cc @Xuanwo who is the expert on this.

[Xuanwo]

Adding a separate error type for the catalog doesn't make it easier for users; instead, it adds more work on their end.

At this stage, I think it would be good to have ErrorKind::TableNotFound and ErrorKind::NamespaceExists. Users can easily handle these errors and obtain detailed information from the error message or context.

[connortsui20]

Adding a separate error type for the catalog doesn't make it easier for users; instead, it adds more work on their end.

At this stage, I think it would be good to have ErrorKind::TableNotFound and ErrorKind::NamespaceExists. Users can easily handle these errors and obtain detailed information from the error message or context.

Sorry I should have been clear, I meant that there could be a dedicated Catalog error kind that could be a variant of ErrorKind. For example:

pub enum ErrorKind {
    Unexpected,
    Catalog(CatalogErrorKind),
    DataInvalid,
    FeatureUnsupported,
}

pub enum CatalogErrorKind {
    NoSuchNamespace,
    NoSuchTable,
    NamespaceAlreadyExists,
    TableAlreadyExists,
}

There's also potentially a NamespaceNotEmpty variant under CatalogErrorKind that should be there, see apache/iceberg#12502

And once table update is implemented, there should be CommitFailed and CommitStateUnknown variants.

[jonathanc-n]

I agree with @connortsui20 's idea. The current possible enums for ErrorKind is too bare, I think a NotFound error would be pretty useful (works well as a generalized error type and additional context can be given)

[liurenjie1024]

I think this is an interesting problem to discuss. iceberg-rust is now a multi crate project, so maybe it's worth rethinking the design of error handling. But I think we should not only think about catalog error here, we need an extensible approach for it.

[connortsui20]

I'll circle back to one of the ideas I brought up: using thiserror or snafu to help build better error handling. Using either would provide strictly more functionality and would not lose any of the expression (that I can see).

And as a side note, I think that the current methodology of constructing an empty Error and adding static context is not very robust. Doing a quick search through this entire repository, I don't really see such a large number of unique situations that would justify needing to dynamically specify these context fields. It makes a lot more sense to define errors over enum variants, where each variant can decide what fields it needs.

[Xuanwo]

Before continuing the discussion, I would like to first explain our current error design goals. In general, error design needs to serve the following objectives:

• The implementer does not need to do extra work.
• The caller can know what error has occurred.
• The caller can decide how to handle this error.
• The caller can understand why this error occurred.

Iceberg's error provides a clear ErrorKind, a concise summary message, and detailed context. Callers can know what happened, decide how to handle it and understand why this error occurred. It also reduce our efforts to decide while error to return while using.

This design shifts our focus from ourselves to our end users. We generally do not return errors like serde_json failure or io.EOF. Instead, we consider whether the error is meaningful to users and contributes to their error handling.

I still think it would be better if we had ErrorKind::TableNotFound, categorizing them as noun-adj. Regardless of which APIs users are calling, they would know what happened.

I propose to add:

pub enum ErrorKind {
    Unexpected,
    DataInvalid,
    FeatureUnsupported,
    
+    NamespaceNotFound,
+    NamespaceExists,
+    TableNotFound,
+    TableExists,
}

---

Sorry I should have been clear, I meant that there could be a dedicated Catalog error kind that could be a variant of ErrorKind. For example:

Hi, a dedicated catalog error kind doesn't look good to me.

Let's try write code from our users side:

let table = match ctl.load_table("test").await {
  Ok(table) => table,
  Err(err) if err.kind() == ErrorKind::TableNotExists => {
    // handle table not exists, like gerenate a user-friendly error message
  }
  Err(err) => {
    // unexpected things happend, we need to log or return bacl.
  }
} 

If we have different layers of error kind:

let table = match ctl.load_table("test").await {
  Ok(table) => table,
  Err(err) if err.kind() == ErrorKind::Catalog(CatalogErrorKind::NoSuchTable) => {
    // handle table not exists, like gerenate a user-friendly error message
  }
  Err(err) => {
    // unexpected things happend, we need to log or return bacl.
  }
} 

Users need to be aware that it's a catalog error first. However, it's not something they need to take action on.

Add more errors variants doesn't serve our error handling goal too.

[connortsui20]

• The implementer does not need to do extra work.
• The caller can know what error has occurred.
• The caller can decide how to handle this error.
• The caller can understand why this error occurred.

If these are the main goals, then I don't think the current system is ideal. When you say the caller, do you mean the developer or the code? The current design implies that the caller is really the developer. Right now the only way to check what actually went wrong is by debug printing everything out (reporting). The code cannot exhaustively match (recover) against the error that happened because 1) there aren't enough variants to choose from and 2) even if there were enough variants in ErrorKind, the caller would have to introduce logic to manually parse out that context and see what went wrong, which is not ideal.

Right now, there are only reportable errors. What I am proposing is that we introduce recoverable errors.

In the small example above, yes it may seem like it is not super useful to have another subenum inside the ErrorKinds because there isn't very many error kinds here right now. If you look at the API spec and do a search for "examples:", it will show that there are actually significantly more (I've counted at least 10 more) error cases that users need to handle.

So I think the argument here is this: Do we have implementors manually craft contexts for each of these unique error cases, and require users to extract that context out (either by logging the error or having to manually match against static strings in the context (which doesn't actually seem possible right now because the context is just used for debugging)), or do we make an effort to discriminate all of the different kinds of errors now so that users can match against any of them so that the caller can decide if they want to recover?

In my opinion, the idealized way to handle errors here would be something like this (obviously room for improvement):

#[derive(Debug)]
pub struct IcebergError {
    // Name can definitely be changed.
    err: IcebergErrorInner,
    // Could have everything else in here that the current version does.
    context: ErrorContext,
}

// I'm guessing that there is other things we would want to put in here.
#[derive(Debug)]
pub enum IcebergErrorInner {
    Unexpected,
    DataInvalid,
    FeatureUnsupported,
    CatalogError(CatalogError),
}

// Only a small subset of possible error cases.
#[derive(Debug)]
pub enum CatalogError {
    TableNotExists,
    TableAlreadyExists { existing_table: String },
    NamespaceNotExists,
    TableCommitFailed(TableCommitFailed),
}

let table = match ctl.rename_table("before", "after").await {
    Ok(table) => table,
    Err(IcebergError {
        err: IcebergErrorInner::Catalog(catalog_error),
        context,
    }) => {
        match catalog_error {
            CatalogError::TableNotExists => todo!(),
            CatalogError::NamespaceNotExists => todo!(),
            CatalogError::TableAlreadyExists { existing_table } => todo!(),
        };
        eprintln!("{context}");
    }
    // This has the benefit of being able to always reject non-catalog errors.
    Err(iceberg_error) => panic!("Something went wrong: {:?}", iceberg_error),
};

// Note that update doesn't work yet for all implementations, but when it 
// does get implemented then this will certainly be something that callers
// want to handle properly / potentially recover from.
let table = match ctl.update_table(commit).await {
    Ok(table) => table,
    Err(IcebergError {
        err: IcebergErrorInner::Catalog(catalog_error),
        context,
    }) => {
        match catalog_error {
            CatalogError::TableCommitFailed(commit_failed) => {
                todo!("There is a whole class of things that a caller might want to do here")
            }
            CatalogError::TableCommitStateUnknown(commit_state_unknown) => todo!("Same here"),
            CatalogError::TableNotExists => todo!(),
        };
        eprintln!("{context}");
    }
    Err(iceberg_error) => panic!("Something went wrong: {:?}", iceberg_error),
};

I think there are 2 key things here. The first is that code can exhaustively match error cases without having to parse a context with dynamic fields. And second is that as implementors, we can now simply raise the correct error case whenever it is needed. If we need to write a macro that helps construct the backtrace and where it came from, then so be it. But like I said above, we could just use snafu to help with that.

Note that the first example I gave above with that subenum was an attempt to get something in between the ErrorKind design and a more robust design, and you are right in that it doesn't make a lot of sense to do both things badly.

[Xuanwo]

do we make an effort to discriminate all of the different kinds of errors now so that users can match against any of them so that the caller can decide if they want to recover

Yes, that's our goal. The context is only for debugging and not designed for uesrs to extract. Similiar to #1071

[connortsui20]

In that case then I think it is reasonable to make the current ErrorKind into a more descriptive type than a couple of discriminants (i.e. add fields to some of the variants), and we would not lose any expression.

[Xuanwo]

In that case then I think it is reasonable to make the current ErrorKind into a more descriptive type than a couple of discriminants (i.e. add fields to some of the variants), and we would not lose any expression.

Hi, I'm still not in favor of an idea like this.

ErrorKind itself is simple, clear, Copy, and easier to use in bindings like python and c. Developers (including us) can easily add context, and users can check and use it effortlessly. I didn't see good reasons to make such change.

[jonathanc-n]

pub enum ErrorKind {
Unexpected,
DataInvalid,
FeatureUnsupported,

• NamespaceNotFound,
• NamespaceExists,
• TableNotFound,
• TableExists,
  }

I would like to see this added as proposed