refactor(error): simplify leaf error instantiation #13627

BugenZhao · 2023-11-23T18:18:56Z

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

What's changed and what's your intention?

In this PR, we take the NotImplemented error as an example.

Instead of writing something like...

return Err(ErrorCode::NotImplemented(
    format!(
        "window frame in `{}` mode is not supported yet",
        frame.units
    ),
    9124.into(),
)
.into());

...simply write...

bail_not_implemented!(
    issue = 9124,
    "window frame in `{}` mode is not supported yet",
    frame.units
);

to construct a NotImplemented error variant then convert and bail out.

If the issue is not applicable, omitting the field also works. This implies <Option<TrackingIssue> as Default>::default().

[bail_]not_implemented!("message {}", here);

Why do we need to generate macros, instead of methods?

Because we want tons of sugars! With macro, we can achieve...

Optional argument specification.
Inline format and implicit conversion (even for Box<str> messages).
Bail out the current function, no need for repeating Err(...).into() everywhere.

Actually the philosophy is picked from anyhow. To construct a leaf error with anyhow, we use anyhow!(<format_args>) and bail out with bail!(..).

The interesting part is that the macro [bail_]not_implemented itself is generated by the proc-macro thiserror_ext::Macro!

risingwave/src/common/src/error.rs

Lines 73 to 80 in 5a79c16

    
           #[derive(Error, Debug, Macro)] 
        
           #[error("Feature is not yet implemented: {feature}. {issue}")] 
        
           #[thiserror_ext(macro(path = "crate::error"))] 
        
           pub struct NotImplemented { 
        
               #[message] 
        
               pub feature: String, 
        
               pub issue: TrackingIssue, 
        
           }

The #[message] attribute here indicates which field should be formatted into. By the way, the macro is also applicable on enum variants.

This means that the convenience for constructing a new error can be scaled out easily in the future.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

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

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao · 2023-11-24T08:58:42Z

e2e_test/error_ui/simple/main.slt

+statement error
+set rw_implicit_flush to maybe;
+----
+db error: ERROR: Failed to run the query
+
+Caused by these errors (recent errors listed first):
+  1: Failed to get/set session config
+  2: Invalid value `maybe` for `rw_implicit_flush`
+  3: provided string was not `true` or `false`


Forget to add tests in last PR (#13588).

BugenZhao · 2023-11-24T09:01:16Z

src/common/src/error.rs

@@ -70,6 +71,15 @@ impl Display for TrackingIssue {
    }
 }

+#[derive(Error, Debug, Macro)]
+#[error("Feature is not yet implemented: {feature}. {issue}")]
+#[thiserror_ext(macro(path = "crate::error"))]


This is because we want the qualified name when calling from other crates to avoid importing the structure itself. So we generate the path $crate::error:: before the type name.

If the structure is private, then path is not necessary.

BTW, is there any way to automatically derive this?

Does module_path!() work?

I didn't find a way. 😕 Calling module_path in the proc macro body won't work as we will get the module path of the proc macro crate.

chenzl25

LGTM!

xxchan

Why do we need to generate macros, instead of methods?

xxchan · 2023-11-26T15:12:39Z

src/common/src/error.rs

@@ -70,6 +71,15 @@ impl Display for TrackingIssue {
    }
 }

+#[derive(Error, Debug, Macro)]


Macro isn't a very cool name 😕

Do you have any proposals? 😢 It's not stable, so feel free to change.

BugenZhao · 2023-11-27T08:09:00Z

Why do we need to generate macros, instead of methods?

Because we want tons of sugars! With macro, we can achieve...

Optional argument specification.
Inline format and implicit conversion (even for Box<str> messages).
Bail out the current function, no need for repeating Err(...).into() everywhere.

Actually the philosophy is picked from anyhow. To construct a leaf error with anyhow, we use anyhow!(<format_args>) and bail out with bail!(..).

Signed-off-by: Bugen Zhao <[email protected]>

xxchan · 2023-11-27T08:38:44Z

Because we want tons of sugars!

But I feel it might be too sweet. Different from anyhow, we will have many macros for each error, users might also feel confused what to use and how to use. One thing to note is that we can't expand to see what macros are available...

I'm wondering would it be enough to have sth like methods accepting Into and ToString parameters.

Anyway, it's a matter of taste. I do not even hold strong opinions to block this PR. Feel free to proceed in any way you like!

Signed-off-by: Bugen Zhao <[email protected]>

stdrc

Do we really need bail_not_implemented? It does simplify our code a lot, but I'm afraid the code can finally become hard to read if number of such macros gradually increase. The key problem is that it involves implicit control flow, although when there is only one bail, it looks clear.

codecov · 2023-11-27T09:10:14Z

Codecov Report

Attention: 103 lines in your changes are missing coverage. Please review.

Comparison is base (45d2df5) 68.24% compared to head (f16fd49) 68.23%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/frontend/src/binder/expr/function.rs	33.33%	22 Missing ⚠️
src/frontend/src/binder/expr/mod.rs	10.00%	9 Missing ⚠️
src/frontend/src/binder/relation/table_function.rs	0.00%	8 Missing ⚠️
...end/src/optimizer/plan_node/logical_over_window.rs	66.66%	7 Missing ⚠️
...rc/frontend/src/binder/relation/table_or_source.rs	0.00%	6 Missing ⚠️
src/frontend/src/handler/mod.rs	0.00%	6 Missing ⚠️
src/frontend/src/handler/create_table.rs	0.00%	5 Missing ⚠️
src/common/src/types/to_binary.rs	0.00%	4 Missing ⚠️
src/frontend/src/expr/window_function.rs	50.00%	3 Missing ⚠️
src/frontend/src/handler/extended_handle.rs	0.00%	3 Missing ⚠️
... and 21 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13627      +/-   ##
==========================================
- Coverage   68.24%   68.23%   -0.02%     
==========================================
  Files        1521     1521              
  Lines      261722   261488     -234     
==========================================
- Hits       178620   178417     -203     
+ Misses      83102    83071      -31

Flag	Coverage Δ
rust	`68.23% <35.62%> (-0.02%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BugenZhao · 2023-11-27T09:43:51Z

Do we really need bail_not_implemented? It does simplify our code a lot, but I'm afraid the code can finally become hard to read if number of such macros gradually increase. The key problem is that it involves implicit control flow, although when there is only one bail, it looks clear.

Totally agree. However, we already have hundreds of occurrences of anyhow::bail. I guess we can expect developers to be familiar with this. 🥵

BugenZhao · 2023-11-27T09:47:46Z

One thing to note is that we can't expand to see what macros are available...

It's still possible. 🥺

Expand the Macro proc-macro

Expand the bail_not_implemented calls

github-actions bot added the type/refactor label Nov 23, 2023

Base automatically changed from bz/error-rw-common to main November 24, 2023 07:14

BugenZhao added 4 commits November 24, 2023 16:13

refactor(error): simplify not implemented error Instantiation

64aad6c

Signed-off-by: Bugen Zhao <[email protected]>

use derived ctor macro and bail macros

bb34b5e

Signed-off-by: Bugen Zhao <[email protected]>

minor fixes

2b32ba6

Signed-off-by: Bugen Zhao <[email protected]>

bump thiserror-ext to 0.0.8

f62b71b

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao force-pushed the bz/error-not-implemented branch from 5a79c16 to f62b71b Compare November 24, 2023 08:13

BugenZhao added 2 commits November 24, 2023 16:27

use not implemented for isolation level

ab4e797

Signed-off-by: Bugen Zhao <[email protected]>

add tests for session config

1ec2c89

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao changed the title ~~refactor(error): simplify not implemented error Instantiation~~ refactor(error): simplify error instantiation Nov 24, 2023

BugenZhao changed the title ~~refactor(error): simplify error instantiation~~ refactor(error): simplify leaf error instantiation Nov 24, 2023

BugenZhao marked this pull request as ready for review November 24, 2023 08:56

BugenZhao requested a review from a team as a code owner November 24, 2023 08:56

BugenZhao requested review from chenzl25, xxchan, stdrc and wangrunji0408 November 24, 2023 08:57

BugenZhao commented Nov 24, 2023

View reviewed changes

chenzl25 approved these changes Nov 24, 2023

View reviewed changes

TennyZhuang approved these changes Nov 24, 2023

View reviewed changes

xxchan reviewed Nov 26, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into bz/error-not-implemented

3c370f8

keep using \n to make error message untouched

fa9eae6

Signed-off-by: Bugen Zhao <[email protected]>

fix slt ui test

f16fd49

Signed-off-by: Bugen Zhao <[email protected]>

stdrc reviewed Nov 27, 2023

View reviewed changes

BugenZhao added this pull request to the merge queue Nov 28, 2023

Merged via the queue into main with commit d5850cd Nov 28, 2023
27 of 28 checks passed

BugenZhao deleted the bz/error-not-implemented branch November 28, 2023 04:39

This was referenced Nov 29, 2023

refactor(error): eliminate most RwError usages in common crate #13588

Merged

refactor: reinvestigate error handling and reporting in the system #11443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(error): simplify leaf error instantiation #13627

refactor(error): simplify leaf error instantiation #13627

BugenZhao commented Nov 23, 2023 •

edited

Loading

BugenZhao Nov 24, 2023

BugenZhao Nov 24, 2023

xxchan Nov 26, 2023 •

edited

Loading

BugenZhao Nov 27, 2023

chenzl25 left a comment

xxchan left a comment

xxchan Nov 26, 2023

BugenZhao Nov 27, 2023

BugenZhao commented Nov 27, 2023

xxchan commented Nov 27, 2023

stdrc left a comment

codecov bot commented Nov 27, 2023 •

edited

Loading

BugenZhao commented Nov 27, 2023 •

edited

Loading

BugenZhao commented Nov 27, 2023

	#[derive(Error, Debug, Macro)]
	#[error("Feature is not yet implemented: {feature}. {issue}")]
	#[thiserror_ext(macro(path = "crate::error"))]
	pub struct NotImplemented {
	#[message]
	pub feature: String,
	pub issue: TrackingIssue,
	}

refactor(error): simplify leaf error instantiation #13627

refactor(error): simplify leaf error instantiation #13627

Conversation

BugenZhao commented Nov 23, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

BugenZhao Nov 24, 2023

Choose a reason for hiding this comment

BugenZhao Nov 24, 2023

Choose a reason for hiding this comment

xxchan Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

BugenZhao Nov 27, 2023

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

xxchan Nov 26, 2023

Choose a reason for hiding this comment

BugenZhao Nov 27, 2023

Choose a reason for hiding this comment

BugenZhao commented Nov 27, 2023

xxchan commented Nov 27, 2023

stdrc left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

BugenZhao commented Nov 27, 2023 • edited Loading

BugenZhao commented Nov 27, 2023

BugenZhao commented Nov 23, 2023 •

edited

Loading

xxchan Nov 26, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

BugenZhao commented Nov 27, 2023 •

edited

Loading