-
Notifications
You must be signed in to change notification settings - Fork 234
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 scalar leak in RankFixer #10475
Fix scalar leak in RankFixer #10475
Conversation
Signed-off-by: Jason Lowe <[email protected]>
build |
@@ -1711,6 +1711,7 @@ class RankFixer extends BatchedRunningWindowFixer with Logging { | |||
override def close(): Unit = { | |||
previousRank.foreach(_.close()) |
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.
Should we also change?
previousRank.foreach(_.close()) | |
previousRank.foreach(_.safeClose()) |
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.
Ah, it turns out that cudf.Scalar
objects cannot be safeClose()
d:
Error: ] /home/runner/work/spark-rapids/spark-rapids/sql-plugin/src/main/scala/com/nvidia/spark/rapids/window/GpuWindowExpression.scala:1712: value safeClose is not a member of ai.rapids.cudf.Scalar
2345
Error: [ERROR] one error found
Co-authored-by: Gera Shegalov <[email protected]>
build |
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.
Yikes. Good catch. Thanks for finding this, @jlowe.
P.S. I think this should be safe to check in after we revert the safeClose
change. @gerashegalov, does that sound agreeable?
Signed-off-by: Gera Shegalov <[email protected]>
@mythrocks it just needs an RapidsPluginImplicits import |
build |
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
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.
Thanks! TIL RapidsPluginImplicits
.
Thanks for the import fix, @gerashegalov! |
Fixes #10473. Updates the RankFixer.close() method to close the BatchedRunningWindowBinaryFixer instance which is caching a scalar.