[SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client

What changes were proposed in this pull request?
When using the 'builtin' Hive version for the Hive metadata client, do not create a separate classloader, and rather continue to use the overall user/application classloader (regardless of Java version). This standardizes the behavior for all Java versions with that of Java 9+. See SPARK-42539 for more details on why this approach was chosen.
Why are the changes needed?
Please see a much more detailed description in SPARK-42539. The tl;dr is that user-provided JARs (such as hive-exec-2.3.8.jar) take precedence over Spark/system JARs when constructing the classloader used by IsolatedClientLoader on Java 8 in 'builtin' mode, which can cause unexpected behavior and/or breakages. This violates the expectation that, unless user-first classloader mode is used, Spark JARs should be prioritized over user JARs. It also seems that this separate classloader was unnecessary from the start, since the intent of 'builtin' mode is to use the JARs already existing on the regular classloader (as alluded to here). The isolated clientloader was originally added in #5876 in 2015. This bit in the PR description is the only mention of the behavior for "builtin":

attempt to discover the jars that were used to load Spark SQL and use those. This option is only valid when using the execution version of Hive.

I can't follow the logic here; the user classloader clearly has all of the necessary Hive JARs, since that's where we're getting the JAR URLs from, so we could just use that directly instead of grabbing the URLs. When this was initially added, it only used the JARs from the user classloader, not any of its parents, which I suspect was the motivating factor (to try to avoid more Spark classes being duplicated inside of the isolated classloader, I guess). But that was changed a month later anyway in #6435 / #6459, so I think this may have basically been deadcode from the start. It has also caused at least one issue over the years, e.g. SPARK-21428, which disables the new-classloader behavior in the case of running inside of a CLI session.
Does this PR introduce any user-facing change?
No, except to protect Spark itself from potentially being broken by bad user JARs.
How was this patch tested?
This includes a new unit test in HiveUtilsSuite which demonstrates the issue and shows that this approach resolves it. It has also been tested on a live cluster running Java 8 and Hive communication functionality continues to work as expected.

cc @sunchao @AngersZhuuuu since you've worked on somewhat-related changes in #34690, #32887, etc.
cc @srowen @squito since you were involved in #24057 for the Java 9+ changes
cc @HyukjinKwon @dongjoon-hyun for any general interest

Also, cc @mridulm , @cloud-fan , @rednaxelafx, @zsxwing, @kiszk , @maropu .

It makes sense to use the builtin classloader when using builtin Hive. To clarification: we still have the class loading issue if people specifies a certain hive version (not builtin), right?

Great question @cloud-fan , and actually no, we don't. For all of the other values of spark.sql.hive.metastore.jars besides 'builtin', the user JARs are not included at all (refer to this section of HiveUtils). In all of those cases, the JAR list is constructed purely from the dependencies for the specified Hive version. Whether that behavior is correct is another question -- @shardulm94 informed me that user JARs are required to support custom serdes inside of the Hive client -- but in any case, 'builtin' is the only mode that is susceptible to this issue.

lgtm if all tests pass

Merged to master/branch-3.4. Thanks @xkrogen !

Thanks @sunchao and @cloud-fan !

Seems like the tests didn't pass .. I am reverting this as it causes a lot of test failures. e.g.)

https://github.com/apache/spark/runs/11644563592
https://github.com/apache/spark/actions/runs/4288729207/jobs/7471072831

Thank you for recovering master branch by reverting, @HyukjinKwon ! The reverting unblocks other PRs.

Shall we revert it from 3.4 as well?

Yes, it's reverted here, 26009d47c1.

Hmm interesting. Somehow the tests were shown all passing for me when I merged this. Sorry for the trouble.

I will take a look at the test failures, thanks @HyukjinKwon for addressing the revert!

If you don't mind, shall we consider this for Apache Spark 3.5 only? ClassLoader issue has been tricky always in the community. We need enough time to stabilize in master branch to give a chance to be verified in several cases by different organizations.

+1. I agree with @dongjoon-hyun .

I agree as well. Posted a new PR at #40224.