refactor: remove macro define_opaque_error

[waynexia]

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

define_opaque_error provides a convenient way to define opaque error type. It's useful for util shim type like BoxedError, but it also gets confused when applied to normal errors. It creates a way to bypass pre-defined error enumerations, and place an arbitrary source error into one type.

This might be useful when trying to expose an error type in the trait method, but it can also be done by defining a generic error type that accepts BoxedError, like QueryExecution.

Checklist

• I have written the necessary rustdoc comments.
• I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

[codecov]

Codecov Report

Merging #812 (56a568e) into develop (4d56d89) will decrease coverage by 0.16%.
The diff coverage is 81.06%.

@@             Coverage Diff             @@
##           develop     #812      +/-   ##
===========================================
- Coverage    85.38%   85.22%   -0.17%     
===========================================
  Files          417      416       -1     
  Lines        54380    54364      -16     
===========================================
- Hits         46435    46333     -102     
- Misses        7945     8031      +86     

Flag	Coverage Δ
rust	85.22% <81.06%> (-0.17%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/catalog/src/error.rs	74.68% <0.00%> (-7.93%)	⬇️
src/promql/src/error.rs	0.00% <0.00%> (ø)
src/query/src/datafusion/catalog_adapter.rs	48.71% <0.00%> (-1.82%)	⬇️
src/query/src/datafusion/error.rs	89.47% <ø> (-2.53%)	⬇️
src/frontend/src/table.rs	89.89% <34.78%> (-1.12%)	⬇️
src/query/src/datafusion/planner.rs	65.45% <50.00%> (-1.22%)	⬇️
src/table/src/error.rs	90.62% <70.00%> (+2.21%)	⬆️
src/query/src/datafusion.rs	95.30% <92.15%> (-1.54%)	⬇️
src/mito/src/engine.rs	98.87% <96.00%> (-0.10%)	⬇️
src/common/error/src/ext.rs	95.71% <100.00%> (ø)
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[v0y4g3r]

LGTM

[v0y4g3r]

IIRC there has long been a mixed usage of define_opaque_error and BoxedError, can @evenyag explain the original necessity of introducing define_opaque_error?

[evenyag]

the original necessity of introducing define_opaque_error

The main purpose is to allow crates to create their own opaque error type for traits, thus they can also implement some methods for their error types. Another consideration is that each implementation might have different variants for its error. It might be not easy to unify these variants into a few categories.

But in practice, it looks a little verbose and inconvenient to use such an opaque error.

[waynexia]

each implementation might have different variants for its error. It might be not easy to unify these variants into a few categories.

I don't think this is the reason to keep this macro. As I mentioned in the very beginning, it can be achieved by

defining a generic error type that accepts BoxedError, like QueryExecution.

If it is to provide a generic error interface for trait methods, I don't see any functionality differences between bare dynamic error and named dynamic error.

[evenyag]

it can also be done by defining a generic error type that accepts BoxedError, like QueryExecution.

The most inconvenient thing is that you need to write lots of .map_err(BoxedError::new).context(QueryExecutionSnafu)?;. 🤣 All of these errors tell you the same thing: Failure during query execution.

[evenyag]

Anyway, I'm fine to remove the usage of define_opaque_error, so I'm going to approve this.

[waynexia]

The most inconvenient thing is that you need to write lots of .map_err(BoxedError::new).context(QueryExecutionSnafu)?;. 🤣 All of these errors tell you the same thing: Failure during query execution.

This deal is done when we decided to use snafu, right? It's really confusing if you write it in some place, but aren't willing to write it here.