[SPARK-36676][SQL][BUILD] Create shaded Hive module and upgrade Guava version to 30.1.1-jre

[sunchao]

What changes were proposed in this pull request?

This PR creates a new module hive-shaded which shades & relocates various dependencies from Hive, including Guava. By this means it also upgrades the Guava version from 14.0.1 to 30.1.1-jre.

Why are the changes needed?

Spark currently use hive-exec-core which leaks a lot of dependencies to Spark, in particular Guava. As consequence, Spark is stuck with an ancient Guava version 14.0.1 which also carries CVE issues described in SPARK-32502.

By creating a shaded module, Spark is able to de-couple from those dependencies leaked by Hive and upgrade to newer versions of Guava. This also allows us to upgrade other dependencies whenever necessary, without having to wait for new Hive releases.

Does this PR introduce any user-facing change?

No. For Spark users who programmatically depending on modules such as spark-hive, the Hive dependencies will be replaced by the newly created hive-shaded module.

How was this patch tested?

Existing tests.

[dongjoon-hyun]

Thank you, @sunchao .

cc @cloud-fan , @HyukjinKwon , @gengliangwang , @wangyum

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47746/

[SparkQA]

Test build #143243 has finished for PR 33989 at commit d5a6f2c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dbtsai]

Since we use shaded guava now in both hive, hadoop, spark, can we remove this dep entirely?

[sunchao]

It actually might be possible: seems Spark now shade Guava usage everywhere within itself now. I remember seeing a few modules which are not using shaded version but now can't find them anymore. Perhaps some mistake from my side.

It's a bit strange that Spark packs the shaded Guava classes in its spark-network-common jar. This is working since it is required by spark-core and thus transitively required by other modules.

I need to fix the PR since the "inherited" tag will prevent Spark from shading jetty & guava in other modules, which is bad. Thinking about how to do proper plugin inheritance in Maven...

[pan3793]

Curator has a hard dependency on Guava, see detail at https://cwiki.apache.org/confluence/display/CURATOR/TN13

[sunchao]

Hmm you are right, I missed this. Since Spark declares Guava as provided dependency, mvn dependency:tree won't show the guava usages in Spark's transitive dependencies.

So sadly, seems we still have to include an unshaded Guava jar in Spark's distribution.

[sunchao]

So build/sbt dependencyTree provides a better result, and it looks like only curator-client and hive-exec has hard dependency on Guava at the moment. With the hive-shaded module, the latter is resolved so we are only left with the former, which makes me thinking whether we can just skip relocating the following 3 classes required by curator:

• com.google.common.base.Function
• com.google.common.base.Predicate
• com.google.common.reflect.TypeToken

It's not ideal but is better than exposing all the Guava classes.

[pan3793]

https://guava.dev/

APIs without @beta will remain binary-compatible for the indefinite future. (Previously, we sometimes removed such APIs after a deprecation period. The last release to remove non-@beta APIs was Guava 21.0.) Even @deprecated APIs will remain (again, unless they are @beta). We have no plans to start removing things again, but officially, we’re leaving our options open in case of surprises (like, say, a serious security problem).

Except hive-shaded, I think it's fine to 1)use vanilla Guava jar directly, 2) avoid using Guava @beta APIs in Spark, 3)keep Guava in the latest version.

[sunchao]

Thanks for the reference! this is good from Spark side but it won't help if downstream apps use beta API which were changed and thus causing conflicts. I agree with you that we should avoid using these beta APIs - perhaps worth adding a maven plugin to check this.

In the mean time I'll leave the guava dependency in the assembly module and we should revisit it separately. Ideally I feel we should remove it if only curator-client is blocking it but we'll need to double check on that.

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47784/

[SparkQA]

Test build #143281 has finished for PR 33989 at commit 4624325.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dbtsai]

LGTM. Thanks!

[pan3793]

If I understand correctly, the hive-shaded module creates a fat jar that contains all hive classes and the classes from transitive dependencies of hive, but only a few classes are relocated. (Correct me if I'm wrong)

That means those classes which are not relocated will cause class conflict if the user also provides the original jars, e.g. HikariCP.

For this case, maybe we can learn something from https://github.com/trinodb/trino-hive-apache

[sunchao]

@pan3793 yes that's correct, I'm not relocating all the dependencies because the others haven't caused any issue so far AFAIK (after all, Spark today already leaks these dependencies). We can definitely include them if necessary, just need to check if they are used in APIs and whether that causes any breakage.

[pan3793]

Spark today already leaks these dependencies

Yeah, but users can see what jars in $SPARK_HOME/jars and 1) align dependencies' version with Spark when build Spark application, 2) exclude jars already under $SPARK_HOME/jars when package or submit Spark job.

For hive-shaded, the user has no idea which jars should be aligned and excluded.

I think hive-shaded should relocate all classes except org.apache.hive, which makes things simple, the user just needs to exclude all hive vanilla jars.

[sunchao]

Makes sense. Let me try to relocate those too.

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47872/

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47873/

[SparkQA]

Test build #143365 has finished for PR 33989 at commit dd28c5b.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #143366 has finished for PR 33989 at commit c8e5764.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[HyukjinKwon]

cc @srowen too FYI

[SparkQA]

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47878/

[SparkQA]

Test build #143371 has finished for PR 33989 at commit aaac03a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[pan3793]

Can the property ${hive.group} be replaced with literal org.apache.hive? I also see there are <groupId>org.apache.hive</groupId> somewhere.

[sunchao]

You mean replace org.apache.hive with ${hive.group}? yes we should do it, although I just copied these verbatim from the existing hive/pom.xml :)

[pan3793]

Which value will be assigned to variable ${hive.group} except org.apache.hive? If no other options, we can remove ${hive.group} and just use literal org.apache.hive.

[sunchao]

I'm fine with either way - don't see much difference between the two.

[pan3793]

This means that the shaded jar will be different when packaging w/ or w/o -Phive-thriftserver, which one will be published to maven central?

[sunchao]

I believe we always publish binaries with -Phive-thriftserver, see here.

[pan3793]

Got it. Thanks for the explanation.

[pan3793]

javax.jdo, javax.realtime, javax.transaction, javax.xml, org.json still exist in the spark-hive-shaded jar with the original package name.

[sunchao]

Oops forgot these. I'm thinking to add these javax.* dependencies explicitly in spark-hive and exclude them from the uber jar, since they seem relatively stable. What do you think?

On the other hand, seems we should exclude org.json from Hive due to SPARK-18262.

[pan3793]

It includes some log conf or other conf files into shaded jar, like bonecp-default-config.xml, hive-exec-log4j2.properties, hive-log4j2.properties, package.jdo, parquet-logging.properties, testpool.jocl, tez-container-log4j2.properties, does spark really need them?

[sunchao]

Good point. Let me check on those.

[SparkQA]

Test build #143610 has finished for PR 33989 at commit c2ec1f8.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[JoshRosen]

A cross-reference for other reviewers:

Given that hive-exec shades Guava in Hive 2.3.8+ (apache/hive#1356), I was initially confused about why we needed to do our own shading in this PR: I originally thought that it was done to shade a broader set of dependencies beyond just Guava, further isolating us from future dependency conflicts. As @viirya points out at #29326 (comment), though, Spark uses the hive-exec-core JAR, not hive-exec, so Hive's Guava shading doesn't apply (hence the need to shade here).

[sunchao]

Hmm interesting. After I changed the isolated class loader to pick guava classes from Hive jars (which is of 14.0.1), tests started to fail because it now seems to use Spark's built-in Guava which is 30.1.1-jre. This doesn't seem to make sense.

[error] sbt.ForkMain$ForkError: java.lang.IllegalAccessError: tried to access method com.google.common.collect.Iterators.emptyIterator()Lcom/google/common/collect/UnmodifiableIterator; from class org.apache.hadoop.hive.ql.exec.FetchOperator
[error] 	at org.apache.hadoop.hive.ql.exec.FetchOperator.<init>(FetchOperator.java:108)
[error] 	at org.apache.hadoop.hive.ql.exec.FetchTask.initialize(FetchTask.java:87)
[error] 	at org.apache.hadoop.hive.ql.Driver.compile(Driver.java:541)
[error] 	at org.apache.hadoop.hive.ql.Driver.compileInternal(Driver.java:1317)
[error] 	at org.apache.hadoop.hive.ql.Driver.runInternal(Driver.java:1457)
[error] 	at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1237)
[error] 	at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1227)
[error] 	at org.apache.spark.sql.hive.client.HiveClientImpl.$anonfun$runHive$1(HiveClientImpl.scala:831)

Iterators.emptyIterator here is no longer public in the newer versions of guava.

[JoshRosen]

I think the test failures seen above are due to the tests not picking up the shaded classes: I wouldn't expect to see a reference to non-relocated Guava classnames if shaded Hive was being used by the tests.

The SBT build doesn't perform any shading (because the sbt-pom-reader plugin only captures the project dependency structure, not build plugins). To date this hasn't been a problem since Spark's existing shading was only for the benefit of downstream consumers of Spark itself and wasn't necessary for Spark's internal functioning (and official releases are published with Maven, not SBT). With this new hive-shaded, though, we'll need to find a way to wire shaded classes into the SBT build.

I think we'll also run into similar problems in the Maven build. According to Maven's build lifecycle docs:

test - test the compiled source code using a suitable unit testing framework. These tests should not require the code be packaged or deployed

The shading runs during the package phase, which is after the test phase, so the tests won't be able to use the shaded artifacts.

The hive-shaded module doesn't depend on any source code from Spark. Given this, a clean path forward might be to publish this Maven artifact separately from Spark's main build, then depend on the published artifact. This would be similar to the approach taken in https://github.com/apache/hbase-thirdparty and https://github.com/apache/hadoop-thirdparty . This would require a bunch of new infra setup, though, so if we're considering pursuing this then we should open a new thread on the dev@ mailing list to discuss.

For now, you could test out that approach via manual local publishing: that'd unblock progress on testing the IsolatedClientLoader and other things.

Why did the tests pass before? It looks like your most recent commit in c2ec1f8 included the DependencyOverrides version bump in SparkBuild.scala as well as the change to the IsolatedClientLoader. Prior to the DependencyOverrides change, the SBT build was using both unshaded Hive and old Guava, so I think the previous successful tests were passing accidentally and that the new failures aren't caused by the IsolatedClientLoader change.

[JoshRosen]

Also, I spent a bit of time re-reading the IsolatedClientLoader code and think I now have a better understanding of how it works and have a better explanation for why I think we should exclude Guava from sharedClasses:

---

First, let's consider the case where the metastore and execution Hive versions are the same:

In the else if (!isSharedClass(name)) branch we'll try to load from the isolated classloader itself via a super.loadClass call. This will first attempt to load classes from rootClassLoader and then will fall back on loading classes from the jars specified in allJars. When the built-in execution Hive is used, it looks like allJars gets populated from the context/Spark classloader's JARs (which is effectively the classpath used by Spark user code). In this case we'd expect Guava 30.1.1-jre to be used.

In the else branch (taken when isSharedClass(name) == true), we first try to load from baseClassLoader, which is the context/Spark classloader. Failing that, we then try try the steps described in the previous branch.

This makes sense to me: if the metastore and execution Hive versions are the same then there's no separate classpath of downloaded or user-provided metastore JARs to use, so we'll wind up loading classes using Spark's own classpath. In this case I think the only difference in these two branches is whether we'll use already-loaded classes from Spark's classloader or whether we'll load them again in the IsolatedClientLoader, but in both cases the classpath should be the same. If Spark's Guava version is the same as the version used by Hive then it's preferable to load from the shared classloader so that we avoid unnecessary duplicate classloading.

---

What happens when the metastore Hive version is not the same as the execution Hive version?

In that case, non-shared classes will be loaded from allJars, which should be a different classpath from Spark itself. As a result, I think we should exclude Guava from the shared classes.

If Spark resolves the metastore JARs from Maven, this allJars path should include a downloaded copy of Guava 14.0.1.

A common pattern that I've seen is that users will use Spark's existing Maven-based metastore resolution once to download the JARs, then will copy the result to a durable location to avoid re-downloading in the future. In that case they'll already have the required old version of Guava and things should continue to work while upgrading.

There's a tricky corner-case if a user has manually built a metastore classpath which includes only the dependencies not already provided by Spark: in that case, an upgrade might cause the user to run into dependency errors if Spark upgrades its Guava and the user's metastore JARs path doesn't include the old version. This is a purely hypothetical scenario and I imagine it would be rare, but it's potentially worth flagging in the docs' upgrade guide.

[JoshRosen]

One more consideration: what about Hadoop 2.7 builds?

My understanding is that Hadoop used unshaded Guava until HADOOP-16924 in Hadoop 3.3.1.

Since Hadoop 2.7.4 still uses unshaded Guava, what are our options? We haven't decided to drop Hadoop 2.7 support yet. Should we continue to use unshaded old Guava under the Hadoop 2.7 profile? Is that potentially confusing to users?

(To be clear, I'm highly in favor of upgrading Guava; I just want to make sure we've thought through the implications and interactions for all of the different build and deployment environments).

[sunchao]

Thanks @JoshRosen ! These are some great analysis!

I think we'll also run into similar problems in the Maven build. According to Maven's build lifecycle docs:

I've completely missed this 🤦 . Yes adding the hive-shaded module in Spark will not be a good idea given the above reasons on SBT and Maven test lifecycle, and now I understand why other projects put the shaded library in a different repo :)

Let me spend more time to revisit the following two paths:

1. shade all the dependencies in Hive (e.g., via hive-exec fat jar) and make a new release, so Spark can start using that.
2. create a ASF repo such as spark-thirdparty following the examples from HBase & Hadoop. This needs community discussion as you mentioned, and I'm not sure how much more burden it will add to Spark's maintenance procedure.

There's a tricky corner-case if a user has manually built a metastore classpath which includes only the dependencies not already provided by Spark

Thanks for the detailed explanation on how the IsolatedClientLoader works, and I agree this is a minor issue we should be aware of. We can either put something on the release notes, or perhaps exclude unshaded Guava jar completely from the Spark distribution (for hadoop-3.2). Currently this appears to be blocked by the curator-client dependency as discussed earlier in the PR, and perhaps there is still a way to ship only shaded Guava (from network-common) with those few classes required by curator-client excluded from relocation.

One more consideration: what about Hadoop 2.7 builds?

Another good question :) You are right that Hadoop 2.7 still uses unshaded Guava, while Hadoop 3.3.1 has switched to use shaded Guava via HADOOP-17288. In addition Spark is using shaded Hadoop client from HADOOP-11804 which further relocates other Hadoop dependencies so they won't pollute Spark's classpath.

I think one approach is to keep Guava 14.0.1 for hadoop-2.7 profile so everything still stay the same there. This at least will unblock us from upgrading Guava for the default hadoop-3.2 profile, and make sure all the published Spark artifacts will get the newer version of Guava. Also the aforementioned idea of excluding unshaded Guava from Spark distribution will only apply for the latter.

A crazier idea is to shade Hadoop 2.7 also if we are going with the spark-thirdparty approach but I'm not sure whether it's worth it given we are going to deprecate hadoop-2.7 eventually.

[pan3793]

1. shade all the dependencies in Hive (e.g., via hive-exec fat jar) and make a new release, so Spark can start using that.

I love this way. Lots of downstream projects will benefit if Hive provides a shaded hive-exec jar that relocates anything(maybe we can give an exception to slf4j or Hadoop classes) but hive classes.

[mridulm]

+CC @xkrogen, @shardulm94

[sunchao]

I love this way. Lots of downstream projects will benefit if Hive provides a shaded hive-exec jar that relocates anything(maybe we can give an exception to slf4j or Hadoop classes) but hive classes.

Yes that would be ideal. There is some recent discussion on this in the Hive community as well, but IMO it could be very difficult to implement it because:

1. it's hard to fully shade and relocate all Hive dependencies, since they could appear in public APIs crossing module & project boundaries. See this PR for some previous effort on this.
2. Currently Hive only provide a fat hive-exec jar but not for other modules like hive-metastore, hive-beeline etc.
3. Spark is using a very old Hive version 2.3.x: it could be a bit tricky to push the changes from master to this branch.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!