-
Notifications
You must be signed in to change notification settings - Fork 564
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
feat(expr): support capturing context in expression #12747
Conversation
Signed-off-by: TennyZhuang <zty0826@gmail.com>
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.
license-eye has totally checked 4258 files.
Valid | Invalid | Ignored | Fixed |
---|---|---|---|
1889 | 1 | 2368 | 0 |
Click to see the invalid file list
- src/expr/macro/src/context.rs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
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.
LGTM. Next, we can consider how to integrate context into the #[function]
macro more gracefully. 🤔
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@xiangjinwu PTAL |
Signed-off-by: TennyZhuang <zty0826@gmail.com>
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.
LGTM. Btw we would need to update this to detect such functions for a better experience.
risingwave/src/frontend/src/handler/query.rs
Lines 244 to 246 in f27182f
fn must_run_in_local_mode(batch_plan: PlanRef) -> bool { | |
SysTableVisitor::has_sys_table(batch_plan) | |
} |
where | ||
F: FnOnce(&#ty) -> R | ||
{ | ||
LOCAL_KEY.try_with(f).map_err(|_| risingwave_expr::ContextUnavailable::new(stringify!(#name))).map_err(Into::into) |
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.
We may need the function name in error message as well:
dev=> set query_mode to distributed;
SET_VARIABLE
dev=> select ''::regclass;
ERROR: QueryError: Expr error: Context DB_NAME not found
Or maybe it can be part of any ExprError
?
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 should be a part of any ExprError
https://buildkite.com/risingwavelabs/pull-request/builds/33335
Unknown failure, seems unrelated to the PR. |
I know how to modify it. Just merge the PR first to make the future work |
Codecov Report
@@ Coverage Diff @@
## main #12747 +/- ##
==========================================
- Coverage 69.19% 69.19% -0.01%
==========================================
Files 1489 1492 +3
Lines 245795 246004 +209
==========================================
+ Hits 170088 170216 +128
- Misses 75707 75788 +81
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
#11522 #11752
Support context capture during expression evaluation.
And support casting to regclass with all valid inputs.
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.