refactor(stream): use columnar eval for temporal join non-lookup conds #15228
Conversation
Signed-off-by: TennyZhuang <[email protected]>
TennyZhuang
requested review from
stdrc,
chenzl25,
wangrunji0408,
BugenZhao and
yuhao-su
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
|
Note: Check "Hide whitespace" can significantly improve your review experience. |
Signed-off-by: TennyZhuang <[email protected]>
TennyZhuang
changed the title
| #[try_stream] | ||
| async { | ||
| #[allow(unreachable_code)] | ||
| if false { | ||
| return Err(unreachable!("type hints only") as StreamExecutorError); | ||
| } |
There was a problem hiding this comment.
This looks really weird. 😇
Is it possible to use #[try_stream(error = StreamExecutorError)]?
There was a problem hiding this comment.
They don't support that.
There was a problem hiding this comment.
It's non-trivial to declare a variable as impl Trait type. TAIT must be enabled for that, and there are many corner cases. So I guess the macro author don't want to do that.
There was a problem hiding this comment.
I'll create a helper macro to generate the code.
| let ArrayImpl::Bool(bool_array) = &*filter else { | ||
| panic!("unmatched type: filter expr returns a non-null array"); | ||
| }; | ||
| let new_vis = bool_array.to_bitmap() | (!row_matched); |
There was a problem hiding this comment.
Why non-matched rows are visible in the result? 🤔
There was a problem hiding this comment.
LEFT OUTER JOIN
There was a problem hiding this comment.
Non-match rows will only be appended to the chunk when it's a LEFT OUTER join, so they should be visible here.
Signed-off-by: TennyZhuang <[email protected]>
|
Need to handle |
Signed-off-by: TennyZhuang <[email protected]>
|
@chenzl25 Is it possible the LHS be non append only in the future? If yes doing columnar eval can be more complicated then |
Signed-off-by: TennyZhuang <[email protected]>
|
If LHS matches a row in RHS and in |
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Recorded in #15257 The PR doesn't make the behavior worse, I guess we can merge it first. |
| #[try_stream] | ||
| async { | ||
| #[allow(unreachable_code)] | ||
| #[allow(clippy::diverging_sub_expression)] | ||
| if false { | ||
| return unreachable!("type hints only") as StreamExecutorResult<_>; | ||
| } |
There was a problem hiding this comment.
Is it possible to extract a method for this async block? The hack here looks ugly🤣
There was a problem hiding this comment.
Prefer to keep the hack code in a small scope and use it only needed.
There was a problem hiding this comment.
I think this is very acceptable. I like the simple and stupid way
Signed-off-by: TennyZhuang <[email protected]>
|
|
#15228) Signed-off-by: TennyZhuang <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Will do a benchmark later
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.