-
Notifications
You must be signed in to change notification settings - Fork 589
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(stream): use columnar eval for temporal join non-lookup conds #15228
Conversation
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[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.
Generally LGTM
#[try_stream] | ||
async { | ||
#[allow(unreachable_code)] | ||
if false { | ||
return Err(unreachable!("type hints only") as StreamExecutorError); | ||
} |
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.
This looks really weird. 😇
Is it possible to use #[try_stream(error = StreamExecutorError)]
?
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.
They don't support that.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why non-matched rows are visible in the result? 🤔
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.
LEFT OUTER JOIN
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to extract a method for this async block? The hack here looks ugly🤣
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.
Prefer to keep the hack code in a small scope and use it only needed.
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.
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]>
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.