refactor(error): migrate function errors to anyhow

[wangrunji0408]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

SQL functions now return anyhow error. Making it easy for implementors to construct errors using anyhow!, bail! and .context().

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added test labels as necessary. See details.
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
• All checks passed in ./risedev check (or alias, ./risedev c)
• My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

• My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

• My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

[wangrunji0408]

caa5b65 adds function name to the error context, which mostly resolves #13394. cc @TennyZhuang

dev=> select 2147483647 + 1;
ERROR:  Failed to run the query

Caused by these errors (recent errors listed first):
  1: Expr error
  2: in function "add"
  3: numeric out of range

[TennyZhuang]

I haven't completely thought it through, but I feel like anyhow might be a bit too heavy for Expr, especially with backtrace. Errors in Expr often occur in succession, and frequent backtracing might cause system crashes.

[TennyZhuang]

Anyway, I like the PR so much. Just some concern about performance.

[wangrunji0408]

Any ideas on how to disable backtrace inside the expr_impl crate? 🥹

[TennyZhuang]

Any ideas on how to disable backtrace inside the expr_impl crate? 🥹

@BugenZhao Can you give some insight?

A simple solution is Box<dyn Error> with some syntax sugur (e.g. context).

[xxchan]

qq: So the most important change is here right? Could you teach me why anyhow is necessary for this?

[wangrunji0408]

I don't think this is the most important part. It is just a side-effect that anyhow makes it easy to add context around the error. 🤡

[xxchan]

Let me rephrase my points: I mean the most important change is the ability to .context, right? But does this require anyhow? 🤔 (I mean anyhow is the implementation detail, instead of the goal) I roughly remember bugen's thiserror_ext also have context.

BTW, the most important user-facing change looks indeed like the function name. 🤣 Can you show me some other exciting examples?

[xxchan]

I roughly remember bugen's thiserror_ext also have context.

Well, it seems to be ContextInto, instead of ContextInfo 🤡

[wangrunji0408]

From developer's view, they can freely use anyhow error and no longer need to consider which ExprError to use. 🤡

[xxchan]

Hmmm, I'm thinking about sth like this might suffice

struct ContextError {
    contexts: Vec<String>,
}

impl ContextError {
    fn context(self, context: impl Display) -> Self {
        let mut contexts = self.contexts;
        contexts.push(context.to_string());
        Self { contexts }
    }
}

// This trait refers to anyhow
trait Context<T> {
    /// Wrap the error value with additional context.
    fn context<C>(self, context: C) -> Result<T, ContextError>
    where
        C: Display + Send + Sync + 'static;

    /// Wrap the error value with additional context that is evaluated lazily
    /// only once an error does occur.
    fn with_context<C, F>(self, f: F) -> Result<T, ContextError>
    where
        C: Display + Send + Sync + 'static,
        F: FnOnce() -> C;
}

impl<T> Context<T> for Result<T, ContextError> {
    fn context<C>(self, context: C) -> Result<T, ContextError>
    where
        C: Display + Send + Sync + 'static,
    {
        // Not using map_err to save 2 useless frames off the captured backtrace
        // in ext_context.
        match self {
            Ok(ok) => Ok(ok),
            Err(error) => Err(error.context(context)),
        }
    }

    fn with_context<C, F>(self, context: F) -> Result<T, ContextError>
    where
        C: Display + Send + Sync + 'static,
        F: FnOnce() -> C,
    {
        match self {
            Ok(ok) => Ok(ok),
            Err(error) => Err(error.context(context())),
        }
    }
}

fn foo() -> Result<(), ContextError> {
    Err(ContextError {
        contexts: vec!["foo".to_string()],
    })
}

fn bar() -> Result<(), ContextError> {
    foo().context("wtf").context("woo")
}

[xxchan]

Oh, this is just similar to tygg's comment above

A simple solution is Box with some syntax sugur (e.g. context).

[BugenZhao]

FYI, the implementation of anyhow::Context::context uses an anonymous error type to provide the context and put the original error as its source, in which way the source chain is correctly maintained.

https://docs.rs/anyhow/latest/src/anyhow/error.rs.html#305-308

[xxchan]

Actually I view these expr errors as leaf errors. So just feel some string stuff is enough.

[BugenZhao]

Any ideas on how to disable backtrace inside the expr_impl crate? 🥹

@BugenZhao Can you give some insight?

I guess there's no way to disable that. The backtrace will always be captured in nightly channel.

https://docs.rs/anyhow/latest/src/anyhow/lib.rs.html#131-132

[xxchan]

We can use a forked anyhow without backtraces if really necessary. 🤪 I think no much effort is needed.

[xxchan]

about the backtrace's overhead: dtolnay/anyhow#41 (comment)

and some numbers I tried previously #6205 (comment)

I think the most important question is whether this sentence holds true 🤔

Errors in Expr often occur in succession

[TennyZhuang]

I have a rough idea (partially inspired by @yuhao-su).

If using Rust ≥ 1.65, a backtrace is captured and printed with the error if the underlying error type does not already provide its own. In order to see backtraces, they must be enabled through the environment variables described in std::backtrace:

According to the doc of anyhow, it seems that they will uses error_generic_member_access feature to acquire the backtrace from its underlying Error. Which means that we can provide a fake Backtrace to misguide anyhow, and then anyhow may giveup to capture a backtrace by themselves.

/// Underlying, real error in the implementation
struct ParseIntError;

struct DisableBacktrace<E: Error>(E);

impl Error for DisabeBacktrace {
    fn provide<'a>(&'a self, req: &mut Demand<'a>) {
        // Pseudo code, need more investigation
        req.provide(Backtrace::disabled);
    }
}

anyhow::Error::new(DisableBacktrace(ParseIntError));

[wangrunji0408]

After discussion with @TennyZhuang, we decided to encourage function implementors to return custom error types rather than anyhow, so that functions can be reused outside of expression framework. The #[function] macro will accept any error type and wrap them in an anyhow error with necessary context.

[BugenZhao]

Which means that we can provide a fake Backtrace to misguide anyhow, and then anyhow may giveup to capture a backtrace by themselves.

If we really want a backtrace in the upper layer, it's worth noting that we might be misguided as well. A simple workaround could be not marking #[backtrace] to the ExprError type in thiserror derivation.

[BugenZhao]

It is just a side-effect that anyhow makes it easy to add context around the error. 🤡

If this is the main motivation, I guess we can achieve similar effects with thiserror_ext (inspired by discussion w/ @xiangjinwu). 😄

Check this example: https://github.com/BugenZhao/thiserror-ext/blob/8e8d7c65f3ddbc1f9097e9200a83f8d3bf9d2a70/examples/context.rs

---

UPDATE: Lazily evaluate the context: https://github.com/risingwavelabs/thiserror-ext/blob/f28a7b9188460d14aa7f12ccaa79cbeb7a6e2e25/examples/context.rs#L31-L33

[xxchan]

Check this example: BugenZhao/thiserror-ext@8e8d7c6/examples/context.rs

Previously I was thinking adding context to the outer struct. 🤣 This solution is quite simple (reminds me of linked list)