refactor(expr): introduce ExprError #3081

TennyZhuang · 2022-06-08T14:03:08Z

Signed-off-by: TennyZhuang [email protected]

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

As title

Checklist

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

Refer to a related PR or issue link (optional)

close #3082

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang · 2022-06-09T02:48:40Z

src/common/src/error.rs

@@ -96,6 +96,12 @@ pub enum ErrorCode {
        #[source]
        BoxedError,
    ),
+    #[error("Expr error: {0:?}")]
+    ExprError(


I'm not sure should we introduce ExprError(ExprError) here instead of convert to other RwError.

pub type BoxedError = Box<dyn Error + Send + Sync>;

So there the error is boxed into a trait object. 🤔

ExprError(ExprError) looks better to me.

I think the expression error will always be capsulated by some other error like StreamError when exposed to the user, so there might be no need to make it a variant of RwError eventually.
We may keep it for now due to compatibility.

pub type BoxedError = Box<dyn Error + Send + Sync>;

So there the error is boxed into a trait object. 🤔

ExprError(ExprError) looks better to me.

It seems we can't refer ExprError here due to crate dependencies?

pub type BoxedError = Box<dyn Error + Send + Sync>;

So there the error is boxed into a trait object. 🤔
ExprError(ExprError) looks better to me.

It seems we can't refer ExprError here due to crate dependencies?

Yes, same as StorageError

Signed-off-by: TennyZhuang <[email protected]>

BowenXiao1999

LGTM!

Looking forward to it!

codecov · 2022-06-09T03:02:40Z

Codecov Report

Merging #3081 (d9ad267) into main (77e07f0) will increase coverage by 0.05%.
The diff coverage is 83.54%.

@@            Coverage Diff             @@
##             main    #3081      +/-   ##
==========================================
+ Coverage   73.40%   73.46%   +0.05%     
==========================================
  Files         734      735       +1     
  Lines       99959    99930      -29     
==========================================
+ Hits        73376    73413      +37     
+ Misses      26583    26517      -66

Flag	Coverage Δ
rust	`73.46% <83.54%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
src/batch/src/executor/project.rs	`72.47% <0.00%> (ø)`
src/batch/src/executor/update.rs	`82.27% <0.00%> (ø)`
src/batch/src/executor/values.rs	`97.67% <0.00%> (+2.21%)`	⬆️
src/common/src/error.rs	`67.68% <0.00%> (-2.94%)`	⬇️
src/expr/src/expr/expr_unary.rs	`74.64% <0.00%> (+0.21%)`	⬆️
src/expr/src/lib.rs	`100.00% <ø> (ø)`
src/expr/src/vector_op/conjunction.rs	`100.00% <ø> (ø)`
src/expr/src/vector_op/like.rs	`95.12% <ø> (ø)`
src/expr/src/vector_op/round.rs	`100.00% <ø> (ø)`
src/expr/src/vector_op/to_char.rs	`0.00% <0.00%> (ø)`
... and 46 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

fuyufjh

LGTM

fuyufjh · 2022-06-09T03:01:34Z

src/common/src/error.rs

@@ -96,6 +96,12 @@ pub enum ErrorCode {
        #[source]
        BoxedError,
    ),
+    #[error("Expr error: {0:?}")]
+    ExprError(


pub type BoxedError = Box<dyn Error + Send + Sync>;

So there the error is boxed into a trait object. 🤔

ExprError(ExprError) looks better to me.

fuyufjh · 2022-06-09T03:02:44Z

src/expr/src/error.rs

+    Array(
+        #[backtrace]
+        #[source]
+        RwError,


Will we refactor this later?

fuyufjh · 2022-06-09T03:03:52Z

src/expr/src/vector_op/agg/general_agg.rs

@@ -256,7 +256,7 @@ mod tests {
                Arc::new(input.into()),
                &agg_type,
                return_type,
-                ArrayBuilderImpl::Int32(I32ArrayBuilder::new(0)?),
+                ArrayBuilderImpl::Int32(I32ArrayBuilder::new(0).unwrap()),


Why ArrayBuilder::new returns Result...

Allocation may failed 😃

Anyway, here it's unwraped.

Here is tests

BugenZhao

Cool!

BugenZhao · 2022-06-09T03:01:49Z

src/common/src/error.rs

@@ -96,6 +96,12 @@ pub enum ErrorCode {
        #[source]
        BoxedError,
    ),
+    #[error("Expr error: {0:?}")]
+    ExprError(


I think the expression error will always be capsulated by some other error like StreamError when exposed to the user, so there might be no need to make it a variant of RwError eventually.
We may keep it for now due to compatibility.

BugenZhao · 2022-06-09T03:04:39Z

src/expr/src/error.rs

+        #[backtrace]
+        #[source]
+        RwError,


May review this in the future. 🤣

We need ArrayError :(

Signed-off-by: TennyZhuang <[email protected]>

refactor(expr): introduce ExprError

67d35b2

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang requested review from skyzh, neverchanje, BugenZhao, st1page and nanderstabel June 8, 2022 14:03

github-actions bot added the type/refactor label Jun 8, 2022

fix test

6802305

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang requested a review from BowenXiao1999 June 9, 2022 02:37

Merge branch 'main' into refactor/introduce-expr-error

efcb013

TennyZhuang mentioned this pull request Jun 9, 2022

Tracking Issue for Iterator::try_collect rust-lang/rust#94047

Open

6 tasks

TennyZhuang commented Jun 9, 2022

View reviewed changes

fix clippy

5e14a67

Signed-off-by: TennyZhuang <[email protected]>

BowenXiao1999 approved these changes Jun 9, 2022

View reviewed changes

fuyufjh reviewed Jun 9, 2022

View reviewed changes

BugenZhao approved these changes Jun 9, 2022

View reviewed changes

prefer to use try_collect

d9ad267

Signed-off-by: TennyZhuang <[email protected]>

TennyZhuang enabled auto-merge (squash) June 9, 2022 03:21

TennyZhuang merged commit b0a38c4 into main Jun 9, 2022

TennyZhuang deleted the refactor/introduce-expr-error branch June 9, 2022 03:30

BugenZhao mentioned this pull request Jun 10, 2022

refactor(array): introduce ArrayError & unify ensure! by anyhow! #3130

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(expr): introduce ExprError #3081

refactor(expr): introduce ExprError #3081

TennyZhuang commented Jun 8, 2022 •

edited

Loading

TennyZhuang Jun 9, 2022

fuyufjh Jun 9, 2022

BugenZhao Jun 9, 2022

BugenZhao Jun 9, 2022

TennyZhuang Jun 9, 2022

BowenXiao1999 left a comment

codecov bot commented Jun 9, 2022 •

edited

Loading

fuyufjh left a comment

fuyufjh Jun 9, 2022

fuyufjh Jun 9, 2022

TennyZhuang Jun 9, 2022

fuyufjh Jun 9, 2022

TennyZhuang Jun 9, 2022

fuyufjh Jun 9, 2022

TennyZhuang Jun 9, 2022

BugenZhao left a comment

BugenZhao Jun 9, 2022

BugenZhao Jun 9, 2022

TennyZhuang Jun 9, 2022

refactor(expr): introduce ExprError #3081

refactor(expr): introduce ExprError #3081

Conversation

TennyZhuang commented Jun 8, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BowenXiao1999 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2022 • edited Loading

Codecov Report

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TennyZhuang commented Jun 8, 2022 •

edited

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading