-
Notifications
You must be signed in to change notification settings - Fork 595
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
fix(over window): fix over window predicate pushdown #13662
Conversation
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
.collect(); | ||
over_window_related_cols.grow(out_schema_len); | ||
|
||
let (window_pred, other_pred) = predicate.split_disjoint(&over_window_related_cols); |
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.
Considering this case, even if we use a id
which should be not over_window_related_cols
column, we should not push this filter through the over window.
select * from (select id, rule, at, row_number() over (partition by cat order by at) as rank from t) x where id = 2;
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.
Then it seems that we cannot push any predicate through over window.
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.
However, partition only related filter could be pushed down. For example
select * from (select id, cat, rule, at, count(*) over (partition by cat order by at) as cnt from t) x where cat = 'foo';
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.
Indeed.
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[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.
LGTM, thanks!
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13662 +/- ##
==========================================
+ Coverage 68.28% 68.30% +0.01%
==========================================
Files 1523 1523
Lines 261607 261619 +12
==========================================
+ Hits 178631 178688 +57
+ Misses 82976 82931 -45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Just need to merge main one more time ...
Signed-off-by: Richard Chien <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Given the following queries:
By their semantics, Q1 requires filtering rows read from
t
, while Q2 requires filtering rows after window function calculation. Previously, for Q2, our optimizer incorrectly pushed the predicate down before over window, which diverged from the expected semantics.Thanks to @xiangjinwu's #13653 for revealing such a long-existing bug. This PR fixes #13653.
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.