Improve error type

[wjones127]

The current error types suffer from a few problems:

1. It's hard to differentiate between user input issues and internal issues (for example, with schema errors.)
2. For internal errors, we often wish we had a backtrace.

Design

For classifying errors, I propose a three tier system:

1. Status code: the general type of error. This could be inspired by HTTP errors. For example, 400 for BadRequest.
2. Title: a short static description of the error. For example, "Table not found".
3. Description: a dynamic string containing details specific to the instance. For example: "Table 'foo' not found in database 'bar'."

In addition, should include:

• The version of Lance (to make this easy to see from error reports)
• An optional Backtrace or Location. We should design the constructor for internal errors to always prefer a Backtrace when available.

// To keep the size of Result<T> small, we use a Box<InnerError> to store the
// actual error. This way, the size of Result<T> is just a pointer.
pub struct Error(pub Box<InnerError>);

/// Lance's generic error.
///
/// The error aims to make it easy to determine error handling. Errors can be
/// classified at three levels:
///
/// * status_code: generic kind of error. We re-use HTTP status codes since
///   they are well known.
/// * title: a specific kind of error. For example, "Table not found."
/// * details: the details for an instance of the error.
pub struct InnerError {
    /// An HTTP status code that best describes the error.
    pub status_code: StatusCode,
    /// A short description of the error. For example, "Table not found.".
    ///
    /// This should not contain details of the instance of the error. Instead,
    /// save that for `details`. This should be the same for all instances of
    /// the same error, and thus is a static string.
    pub title: &'static str,
    /// A longer description of the error, including details specific to the
    /// instance. For example, "Table 'foo' not found in database 'bar'.".
    pub details: String,
    /// The underlying cause of the error, if any.
    pub cause: Option<BoxedError>,
    /// The backtrace or location of the error.
    pub backtrace: MaybeBacktrace,
    /// Version of the Lance library
    pub version: &'static LanceVersion
}

pub enum MaybeBacktrace {
    Captured(Backtrace),
    Location(&'static std::panic::Location<'static>),
    None,
}

Development

• Define new error type
• Provide conversion from existing error type to new error type
• Iteratively go through libraries replacing creation of old error type with new error type

[westonpace]

This is great. I'd add two more requests, if possible.

First, we often convert to/from Datafusion errors. When converting to a DF error, if the source error is present, and is a DF error, then just return the source error. When converting from a DF error, if it is a DF external error, and the source error is a lance error, then just unwrap the lance error. This will help avoid situations where we have something like LanceError(DatafusionError(LanceError))

Second, when we hit an unexpected error in the scanner or file reader, we often have a hard time knowing the exact location. For example, maybe we hit an error slicing a buffer in some decoder. That will cause the decode task to fail, which will cause the scan to fail. Typically we end up with something like "invalid slice at " which is not helpful for tracking down the original error.

To solve this problem I propose when we convert from Arrow to lance, or when we create an internal Lance error, that we capture the backtrace with std::backtrace::Backtrace::capture and attach it to the error. This will be a no-op unless RUST_BACKTRACE=1 but, if that environment variable is set, then we will have the true location of the error.

If there is any spot where we do expect an arrow error (e.g. due to bad user input) we can always map_err to manually create the arrow error. I think these spots will be rare.

[camilesing]

Can you assign it to me? I can try it. @Xuanwo @wjones127

[wjones127]

I'm thinking about this some more, and I think the original idea might not be the way to go. The proposal above radically changes the API of errors, which will make it hard to roll out incrementally. I think it also doesn't align very well with how the Rust ecosystem tends to design errors (they prefer enums rather than error codes).

I've been reading this blog post: https://sabrinajewson.org/blog/errors, which has some good ideas on how to design errors. I think we should identify some incremental improvements which move us towards better errors. We can pair this with increasing test coverage to 100% (many missing lines are just error cases), which will help nail down the API for errors.

Here are some principles we could use, based on the blog post.

Principles for error handling

Testing

Error cases should be tested in unit tests. This validates:

1. The error is returned when expected.
2. The error message is formatted as expected.

This becomes especially when APIs are later refactored, as it ensures that error handling is not accidentally removed or changed (breaking the API).

Context

Errors should give sufficient context. Example: should wrap a low-level Lance file decoder error with information about which field is being read, and wrap that with information about which file is being read.

Error: failed to read file 'data.lance'

Caused by:
    0: failed to read field 'age'
    1: bitpacked value out of range in _rowoffset 10
       backtrace:
         ...

This might be done with something like:

fn read_file(path: &str) -> Result<(), ReadFileError>;

struct ReadFileError {
    path: String,
    source: ErrorKind,
}

enum ErrorKind {
    DecodeError(FieldDecodeError),
    IoError(std::io::Error),
}

struct FieldDecodeError {
    field_name: String,
    source: DecodeError,
}

struct DecodeError {
    row_offset: usize,
    kind: DecodeErrorKind,
}

enum DecodeErrorKind {
    BitpackedValueOutOfRange,
    // ...
}

Roundtripping

When control is inverted, like when passing streams, it should be easy to extract the original error. Consider the API:

async fn write_data(data: SendableRecordBatchStream) -> Result<(), Error>;

To create a SendableRecordBatchStream, the user might wrap their own Error type into DataFusionError::External. When write_data encounters an error from the steram, there should be an easy error variant to get that back, like:

match write_data(stream).await {
    Err(Error::External(source)) => {
        if let Some(original_error) = source.downcast_ref::<MyErrorType>() {
            // handle original error
        }
    }
    Err(e) => {
        // handle other errors
    }
    Ok(_) => {}
}

Easy classification

Errors should be easy to classify. For example, if you are wrapping Lance in an HTTP service, it should be easy to write a mapping from Lance errors to HTTP status codes.

The most important cases are for things like:

• Invalid input (400)
• Not found (404)
• Already exists (409)

Remaining errors can be mapped to 500 (internal server error).

This can probably be handled just by making easy to use APIs:

fn create_dataset(uri: &str) -> Result<Dataset, OpenDatasetError>;

enum OpenDatasetError {
    InvalidUriFormat(UriParseError),
    LocationNotFound {
        uri: String,
        source: ObjectStoreError,
    },
    AlreadyExists {
        uri: String,
    },
    IO(ObjectStoreError),
    Internal(InternalError),
}

Since this high-level API dispatches to several APIs, it defines an error that represents the different options for what might happen. Errors returned by URI parsing function are wrapped in InvalidUriFormat, which can be mapped to 400.

The IO errors can be passed through to IO most of the time, but this function has enough info to differentiate certain semantically meaningful IO errors: ones that mean the location doesn't exist can be LocationNotFound (converted to 404) and ones that mean already exists can map to AlreadyExists (converted to 409).

Internal errors should have backtraces

These are for errors that shouldn't happen in normal operations, such as data corruption or logic errors. They should include backtraces to help with debugging.

[wjones127]

Here are some of the incremental TODOs I have in mind:

• Convert Error::Internal to a struct that grabs a backtrace
• Remove bad to_string() calls on errors
• Add Error::External variant to provide back external errors. Make sure we
  use this with APIs that take TryStream or Iterator of Result, such that users
  can get back original errors.
• Add ReadFileError and DecodeColumnError wrappers to provide better context
  for decoding errors
• Add WriteFileError and EncodeColumnError wrappers to provide better context
  for encoding errors
• Review use of Error::io() to find cases where that error should be a different type
• Change everything to use thiserror and remove snafu
• Make sure high-level APIs (open dataset, insert, delete, etc.) have obvious error variant for cases that need to be handled specially (e.g. file not found, permission denied, etc)

[wjones127]

@camilesing I think we want to take a different approach from what was originally described here. I started a few of the first things on the list above. Do you want to look at either of these two?

• Add ReadFileError and DecodeColumnError wrappers to provide better context for decoding errors #5602
• Review use of Error::io() to find cases where that error should be a different type #5601