-
Notifications
You must be signed in to change notification settings - Fork 242
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 AnsiCastOpSuite failures with Spark 3.2 #3377
Fix AnsiCastOpSuite failures with Spark 3.2 #3377
Conversation
Signed-off-by: Andy Grove <[email protected]>
shims/spark320/src/main/scala/com/nvidia/spark/rapids/shims/spark320/Spark320Shims.scala
Show resolved
Hide resolved
Signed-off-by: Andy Grove <[email protected]>
Signed-off-by: Andy Grove <[email protected]>
Signed-off-by: Andy Grove <[email protected]>
Signed-off-by: Andy Grove <[email protected]>
sql-plugin/src/main/scala/com/nvidia/spark/rapids/SparkShims.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/spark30+all/scala/com/nvidia/spark/rapids/shims/v2/Spark30XShims.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Andy Grove <[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.
Looks good to me, but I want to be sure that @gerashegalov is also okay with it because he reviewed this too.
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, but preferably merge after #3381
recurse(plan, predicate, new ListBuffer[SparkPlan]()) | ||
} | ||
|
||
override def shouldAssertIsOnTheGpu(plan: SparkPlan): Boolean = plan match { |
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 am tempted to suggest trading off type safety for such small auxiliary functions and just do string comparison in the shared code instead of instanceof checks in the future.
I am looking into the test failures |
build |
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.
I don't know the new build system well enough to approve this right now. I see shim changes for spark320 and 301+-nondb, but what about databricks?
This PR does not build on Databricks. I will have a fix up later today. |
I pushed a fix and this now builds on Databricks 7.3 |
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.
Just FYI the build has been having some issues because of a mirror of public maven repos.
build |
This PR updates an assertion in AnsiCastOpSuite and fixes all 21 test failures when running against Spark 3.2
This partially addresses #3376