-
Notifications
You must be signed in to change notification settings - Fork 590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(optimizer): record error contexts when casting composite types #19449
Conversation
41af6ff
to
c42466f
Compare
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
c42466f
to
8c0a300
Compare
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
8c0a300
to
b2f65b7
Compare
Signed-off-by: Bugen Zhao <[email protected]>
b2f65b7
to
3f12a20
Compare
Signed-off-by: Bugen Zhao <[email protected]>
- only show "context" in the bottom level - show struct field name - do not expose internal impl of map type, instead, show "key" or "value" Signed-off-by: Bugen Zhao <[email protected]>
@@ -204,10 +275,10 @@ pub enum CastContext { | |||
Explicit, | |||
} | |||
|
|||
pub type CastMap = BTreeMap<(DataTypeName, DataTypeName), CastContext>; | |||
pub type CastTable = BTreeMap<(DataTypeName, DataTypeName), CastContext>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to be confused with cast_map
.
Signed-off-by: Bugen Zhao <[email protected]>
if ok { | ||
Ok(()) | ||
} else { | ||
bail_cast_error!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confused how it works. So this is an empty leaf CastError? Why it's not shown in the err msg stack? (Why we need it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In error report we'll disregard all empty and repeated entries, so it won't appear.
To preserve the reason for a cast failure, we must return a Result
in functions like cast_struct
. However, the default error message "cannot cast" is uniformly applied in cast
, so we simply return an empty leaf error and let the caller to fill the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like Result<(), Option<CastError>>
🤪
Signed-off-by: Bugen Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good 👍
Signed-off-by: Bugen Zhao <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Record contexts for error when recursively calling
cast
on composite types, to provide better error messages.Preview:
risingwave/src/frontend/planner_test/tests/testdata/output/cast.yaml
Lines 92 to 115 in fc42233
Checklist
./risedev check
(or alias,./risedev c
)Documentation
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.