Dynamically load hive and avro using reflection to avoid potential class not found exception [databricks] #5723
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
res-life
changed the title
|
build |
…ass not found exception Signed-off-by: Chong Gao <res_life@163.com>
res-life
force-pushed
the
fix-compile-time-reference
branch
from
d73e53e
to
c89b407
Compare
|
build |
|
Conflict with #5716 |
|
I have verified this fixes the bug reported by our QA |
razajafri
reviewed
Jun 2, 2022
| object ExternalSource { | ||
| val providerClassName = "org.apache.spark.sql.rapids.AvroSourceProvider" | ||
|
|
||
| lazy val hasSparkAvroJar = { | ||
| val loader = Utils.getContextOrSparkClassLoader |
There was a problem hiding this comment.
We should do the same thing here as we are doing in GpuHiveOverrides. i.e. use the ShimLoader
gerashegalov
requested changes
Jun 2, 2022
Comment on lines
49
to
50
| val className = "org.apache.spark.sql.hive.rapids.HiveSourceProvider" | ||
| ShimLoader.newInstanceOf[HiveProvider](className).getExprs |
There was a problem hiding this comment.
nit:
Our current pattern is to create dedicated methods in ShimLoader to reduce spreading of the class-for-name sort of code in the code base.
Consider creating in ShimLoader.scala methods:
def newHiveProvider(): HiveProvider= {
newInstanceOf[HiveProvider]("org.apache.spark.sql.hive.rapids.HiveSourceProvider")
}Another nit about naming. Usual pattern is that when the interface/trait called HiveProvider then the implementing class is HiveProviderImpl
Naming is hard. Given this called from GpuHiveOverrides, should the trait be GpuHiveOverridesProvider and the class GpuHiveOverridesImpl?
Similar considerations for Avro
| case _: AvroFileFormat => true | ||
| case _ => false | ||
| } | ||
| ShimLoader.newInstanceOf[AvroProvider](providerClassName).isSupportedFormat(format) |
There was a problem hiding this comment.
This can be called more than once, and will lead to creating garbage instances of AvroProvider. We should memoize AvroProvider as a singleton in a lazy val similar to hasSparkAvroJar
Let us create a ShimLoader method def newAvroProvider(): AvroProvider . See the comment about Hive
I will file a dedicated issues for this. It does not look like this PR fixes it yet. To reproduce, invoke: $SPARK_HOME/bin/pyspark \
--driver-class-path dist/target/rapids-4-spark_2.12-22.08.0-SNAPSHOT-cuda11.jar \
--packages org.apache.spark:spark-avro_2.12:3.2.1 \
--conf spark.plugins=com.nvidia.spark.SQLPlugin \
--conf spark.rapids.sql.enabled=true \
--conf spark.rapids.sql.explain=ALLIf we substitute --jars for --driver-class-path it works |
added 2 commits
June 7, 2022 13:24
|
I verified the following test works. |
|
build |
|
@gerashegalov Help review. |
|
build |
1 similar comment
|
build |
gerashegalov
requested changes
Jun 20, 2022
| /** | ||
| * Singleton Avro Provider | ||
| */ | ||
| lazy val avroProvider: AvroProvider = ShimLoader.newInstanceOf[AvroProvider]( |
There was a problem hiding this comment.
I'd rather have only methods in these trait, and not introduce any constraints, and let the caller decide how to use it. Can we change this to:
def newAvroProvider(): AvroProvider = {
...
}
for consistency.
| case _: AvroFileFormat => true | ||
| case _ => false | ||
| } | ||
| ShimLoader.avroProvider.isSupportedFormat(format) |
There was a problem hiding this comment.
Add a lazy val to object ExternalSource
lazy val avroProvider = ShimLoader.newAvroProvider()and here and elsewhere simply call methods on the lazy val
|
build |
|
build |
|
this failed in https://github.com/NVIDIA/spark-rapids/blob/branch-22.08/jenkins/spark-premerge-build.sh#L81-L84 |
|
build |
|
I posted the above 2 commits. The second is to fix the comments. |
gerashegalov
previously approved these changes
Jun 22, 2022
Comment on lines
4
to
5
| com/nvidia/spark/rapids/HiveProvider.class | ||
| com/nvidia/spark/rapids/AvroProvider.class |
There was a problem hiding this comment.
Let us keep the file sorted
|
build |
gerashegalov
approved these changes
Jun 23, 2022
|
build |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5648
Potential class not found exception problem:
source code of ExternalSource
See the above code.
ExternalSourcestatically referenceAvroScanevenAvroScandoes not exist at runtime.ExternalSource bytecode has the compile reference.
Resolve
Use the method mentioned in the issue #5648
Remove all the references to
Avroclasses in theExternalSourcefile.Provides a
providertrait and animplementsubclass.Put all the compile-references into the
implementsubclass.ExternalSourcedynamically loadsproviderusing reflection to avoid potential class not found exception.If has no
Avrojar detected by theloader.loadClass,ExternalSourcewill not try to load theimplementsubclass.Also apply to
HiveThe following avro and hive classes should move to subclasses of the providers.
avro.{AvroFileFormat, AvroOptions}
avro.AvroScan
hive.{HiveGenericUDF, HiveSimpleUDF}
Signed-off-by: Chong Gao res_life@163.com