-
Notifications
You must be signed in to change notification settings - Fork 245
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
Add Apache Spark 3.3.0-SNAPSHOT Shims #4012
Add Apache Spark 3.3.0-SNAPSHOT Shims #4012
Conversation
Signed-off-by: Gera Shegalov <[email protected]>
Signed-off-by: Gera Shegalov <[email protected]>
Signed-off-by: Gera Shegalov <[email protected]>
Signed-off-by: Gera Shegalov <[email protected]>
build |
Signed-off-by: Gera Shegalov <[email protected]>
.../src/main/301until330-all/scala/com/nvidia/spark/rapids/shims/v2/Spark30Xuntil32XShims.scala
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
|
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.
nit extra newline
.../src/main/301until330-all/scala/com/nvidia/spark/rapids/shims/v2/Spark30Xuntil32XShims.scala
Outdated
Show resolved
Hide resolved
.../src/main/301until330-all/scala/com/nvidia/spark/rapids/shims/v2/Spark30Xuntil32XShims.scala
Outdated
Show resolved
Hide resolved
@@ -63,7 +63,7 @@ case class GpuOrcScan( | |||
super.description() + ", PushedFilters: " + seqToString(pushedFilters) | |||
} | |||
|
|||
override def withFilters( | |||
def withFilters( |
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.
so I think this is bad, meaning Spark changed this in SPARK-36351 and isn't calling withFilters anymore in 3.3.0. This implies we might not be getting the filters applied in Spark 3.3.0 because we don't implement the new api.
We need to at least file a follow on for this and point to both SPARK-36351 and this change to investigate to make sure we don't need to use those new API.
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/metrics/source/MockTaskContext.scala
Show resolved
Hide resolved
Signed-off-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.
changes look fine pending filing the follow on issue for SPARK-36351
cc @pxLi |
thanks, I will add a nightly pipeline for spark33x~ |
Closes #3863
Closes #3954
Signed-off-by: Gera Shegalov [email protected]