peter-toth commented on code in PR #58615:
URL: https://github.com/apache/spark/pull/58615#discussion_r3995611322
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala:
##########
@@ -200,6 +200,26 @@ private[spark] object HiveUtils extends Logging {
.booleanConf
.createWithDefault(false)
+ val INITIALIZE_METASTORE_FORMAT_CLASSES =
+ buildConf("spark.sql.hive.initializeMetastoreFormatClasses")
+ .doc("When true, an InputFormat/OutputFormat class name stored in the
Hive metastore is " +
+ "resolved with its static initializer run at resolution time. When
false, the class is " +
+ "still loaded, so a missing class still fails here, but its static
initializer is not " +
+ "run. Only the class name is needed when converting metastore
metadata, so setting " +
+ "this to false avoids running a format class's static initializer
during a metadata " +
+ "operation. Note that the conversion also runs when planning a scan or
write and when " +
+ "inferring the schema of a Hive serde table, so with false a format
class whose static " +
+ "initializer fails no longer fails fast at resolution time. It fails
when the format " +
+ "is instantiated instead (as NoClassDefFoundError: Could not
initialize class ...), " +
Review Comment:
**Finding 7.** This parenthetical is mine from round 2 and it is wrong.
`false` changes where the failure lands, not what it is. Class
initialization fails the same way in both modes:
- the first attempt to initialize the class throws
`ExceptionInInitializerError`
- every later attempt throws `NoClassDefFoundError: Could not initialize
class ...`
With `true` that first error comes out of `Utils.classForName` here in
`toHiveTable`. With `false` it comes out of the first instantiation instead,
e.g. `ReflectionUtils.newInstance` at
`core/src/main/scala/org/apache/spark/rdd/HadoopRDD.scala:218`, where Hadoop
wraps it in a `RuntimeException`. The `catch` just below at `:220` relies on
that wrapping. So what an operator actually sees on a scan is
`RuntimeException: ExceptionInInitializerError`, and `NoClassDefFoundError`
only from the second query on.
Checked on JDK 17 with a class whose static block throws:
```
loaded uninitialized: p.Boom
attempt 0 -> java.lang.ExceptionInInitializerError: null
attempt 1 -> java.lang.NoClassDefFoundError: Could not initialize class
p.Boom
```
Simplest fix is to drop the parenthetical. The sentence is about location,
and the rest of it already gives driver versus executor.
```suggestion
"is instantiated instead, " +
```
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientImplSuite.scala:
##########
@@ -20,9 +20,46 @@ package org.apache.spark.sql.hive.client
import org.apache.hadoop.hive.metastore.api.FieldSchema
import org.apache.spark.{SparkFunSuite, SparkUnsupportedOperationException}
+import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat,
CatalogTable, CatalogTableType}
+import org.apache.spark.sql.hive.{HiveUtils, StaticInitFlags,
StaticInitInputFormat, StaticInitOutputFormat}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StructType
class HiveClientImplSuite extends SparkFunSuite {
+ test("SPARK-59330: toHiveTable skips the format class static initializer
when " +
+ "spark.sql.hive.initializeMetastoreFormatClasses is false") {
+ // Both call sites are exercised: toHiveTable resolves the input format
via toInputFormat and
+ // the output format via toOutputFormat, so each format class has its own
flag.
+ val table = CatalogTable(
+ identifier = TableIdentifier("t", Some("default")),
+ tableType = CatalogTableType.MANAGED,
+ storage = CatalogStorageFormat.empty.copy(
+ inputFormat = Some(classOf[StaticInitInputFormat].getName),
+ outputFormat = Some(classOf[StaticInitOutputFormat].getName)),
+ schema = new StructType().add("a", "int"))
+
+ def toHiveTableWith(initialize: Boolean): Unit = {
+ val conf = new SQLConf()
+ conf.setConf(HiveUtils.INITIALIZE_METASTORE_FORMAT_CLASSES, initialize)
+ SQLConf.withExistingConf(conf) {
+ HiveClientImpl.toHiveTable(table)
+ }
+ }
+
+ // The false half must run first: class initialization is one-way per JVM.
Resolving the
+ // format class names without initializing them must not run their static
initializers.
+ toHiveTableWith(initialize = false)
+ assert(!StaticInitFlags.inputFormatInitialized)
+ assert(!StaticInitFlags.outputFormatInitialized)
Review Comment:
**Finding 8.** The doc now says `false` still loads the class, so a missing
class still fails here. That is the reason for `initialize = false` over
dropping the resolution entirely, and nothing pins it. A refactor that stopped
resolving and set the SD's format string directly, the way `toHivePartition`
already does at
`sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:1276`,
would leave this suite green while silently changing that contract.
A separate test keeps it clear of the one-way-init ordering this one depends
on:
```scala
test("SPARK-59330: toHiveTable still resolves the format class when " +
"spark.sql.hive.initializeMetastoreFormatClasses is false") {
val table = CatalogTable(
identifier = TableIdentifier("t", Some("default")),
tableType = CatalogTableType.MANAGED,
storage = CatalogStorageFormat.empty.copy(
inputFormat =
Some("org.apache.spark.sql.hive.DoesNotExistInputFormat")),
schema = new StructType().add("a", "int"))
val conf = new SQLConf()
conf.setConf(HiveUtils.INITIALIZE_METASTORE_FORMAT_CLASSES, false)
intercept[ClassNotFoundException] {
SQLConf.withExistingConf(conf) {
HiveClientImpl.toHiveTable(table)
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
