-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(rust, python): Clear window cache after evaluate predication expr #10505
Conversation
@@ -35,6 +35,9 @@ impl Executor for FilterExec { | |||
state.insert_has_window_function_flag() | |||
} | |||
let s = self.predicate.evaluate(&df, state)?; | |||
if state.cache_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.
Might replace this by if has_window
to reduce the overhead of atomic load? 🤔
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 an you put the cache_window
and the clear_window_expr
behind a if self.has_windows
?
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.
Do you mean to replace it with
if self.has_window() && state.cache_window() {
state.clear_window_expr_cache();
}
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 is what we do during projection.
if has_windows {
state.clear_window_expr_cache();
}
I believe the && state.cache_window()
is redundant as it should always be set if has_windows
is true.
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 see, removed 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.
Nice @reswqa. Great analysis.
I have left some comments.
@@ -35,6 +35,9 @@ impl Executor for FilterExec { | |||
state.insert_has_window_function_flag() | |||
} | |||
let s = self.predicate.evaluate(&df, state)?; | |||
if state.cache_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.
Yes an you put the cache_window
and the clear_window_expr
behind a if self.has_windows
?
Thanks @ritchie46 for the review. I have updated this, but I'm not sure if I totally understand all comments, so feel free to append new comments. |
The redundant |
Alright! 🙌 |
This fixes #10499.