peter-toth commented on code in PR #58615:
URL: https://github.com/apache/spark/pull/58615#discussion_r3978501345
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1163,11 +1163,20 @@ private[hive] object HiveClientImpl extends Logging {
Option(hc.getComment).map(field.withComment).getOrElse(field)
}
+ // Only the class name is needed when converting metastore metadata (Hive
stores it via
+ // getName), so when spark.sql.hive.initializeMetastoreFormatClasses is
false the class is
+ // resolved without running its static initializer here; the initializer
then runs when the
+ // format is actually instantiated for a scan/write. Defaults to true (the
previous behavior).
+ private def initializeFormatClasses: Boolean =
+ SQLConf.get.getConf(HiveUtils.INITIALIZE_METASTORE_FORMAT_CLASSES)
+
private def toInputFormat(name: String) =
- Utils.classForName[org.apache.hadoop.mapred.InputFormat[_, _]](name)
+ Utils.classForName[org.apache.hadoop.mapred.InputFormat[_, _]](
+ name, initialize = initializeFormatClasses)
Review Comment:
**Finding 1.** The `true` branch is the pre-PR path, so the existing suites
cover it by accident. Nothing exercises `false`, which is everything the PR
adds. A later refactor that drops the `initialize` argument, or one that
reintroduces an eager resolution before `setInputFormatClass`, would go
unnoticed.
Sketch below, untested. Scala classes have no static initializer, so it
needs two small Java files under
`sql/hive/src/test/java/org/apache/spark/sql/hive/`:
```java
public final class StaticInitFlags {
public static volatile boolean inputFormatInitialized = false;
}
public class StaticInitInputFormat implements
org.apache.hadoop.mapred.InputFormat<Void, Void> {
static { StaticInitFlags.inputFormatInitialized = true; }
// getSplits / getRecordReader throw UnsupportedOperationException
}
```
and one test:
```scala
val t = table.copy(storage = table.storage.copy(
inputFormat = Some(classOf[StaticInitInputFormat].getName)))
withSQLConf(HiveUtils.INITIALIZE_METASTORE_FORMAT_CLASSES.key -> "false") {
HiveClientImpl.toHiveTable(t)
assert(!StaticInitFlags.inputFormatInitialized)
}
withSQLConf(HiveUtils.INITIALIZE_METASTORE_FORMAT_CLASSES.key -> "true") {
HiveClientImpl.toHiveTable(t)
assert(StaticInitFlags.inputFormatInitialized)
}
```
Two constraints. The flag has to live on a separate class, because reading
it off the format class would initialize the very class under test.
Initialization is one-way per JVM, so the `false` half has to run first.
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala:
##########
@@ -200,6 +200,19 @@ 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 " +
+ "resolved without running its static initializer, which then runs when
the format is " +
Review Comment:
**Finding 2.** `toHiveTable` is not reached only from metastore metadata
operations. It also runs on:
-
`sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveTableScanExec.scala:92`,
Hive serde scan planning
-
`sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala:254`
and
`sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/V1WritesHiveUtils.scala:54`,
write planning
-
`sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveDirCommand.scala:77`
- `HiveUtils.inferSchema` at line 544 of this file, which `HiveStrategies`
calls during analysis
So `false` also skips initialization for a plain `SELECT` off a Hive serde
table. That is fine functionally, because the format is instantiated through
`ReflectionUtils` in `HadoopTableReader`. It does move the failure for a class
whose static initializer throws from the driver to an executor task, where it
reads as `NoClassDefFoundError: Could not initialize class ...`. That is the
part an operator flipping this needs to know, so it is worth a sentence in the
doc.
Same for the comment at
`sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:1166`.
"Only the class name is needed" holds for `setInputFormatClass`, but
`HiveTableScanExec` uses the `Class` object itself, comparing it to
`classOf[SymlinkTextInputFormat]` and putting it in the `TableDesc`. An
uninitialized class serves both, so the conclusion stands, the stated reason
just does not cover every use.
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1163,11 +1163,20 @@ private[hive] object HiveClientImpl extends Logging {
Option(hc.getComment).map(field.withComment).getOrElse(field)
}
+ // Only the class name is needed when converting metastore metadata (Hive
stores it via
+ // getName), so when spark.sql.hive.initializeMetastoreFormatClasses is
false the class is
+ // resolved without running its static initializer here; the initializer
then runs when the
+ // format is actually instantiated for a scan/write. Defaults to true (the
previous behavior).
+ private def initializeFormatClasses: Boolean =
Review Comment:
**Finding 3.** I went looking for a consumer that needs the class
initialized and did not find one:
- `Table.setInputFormatClass` / `setOutputFormatClass` store the `Class` and
call `getName()`.
- `HiveTableScanExec.getInputFormat` compares by identity
(`sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveTableScanExec.scala:237`).
- `HadoopTableReader.compatibleWithNewHadoopRDD` uses `isAssignableFrom`
(`sql/hive/src/main/scala/org/apache/spark/sql/hive/TableReader.scala:272`).
- `HadoopRDD` and `NewHadoopRDD` reach it through
`ReflectionUtils.newInstance`, which initializes it there.
The same method already sets the serde by name only (line 1225), and
`toHivePartition` sets both formats by name only (line 1273). So `initialize =
false` matches what this file does everywhere else.
The one thing `true` buys is failing early, at CREATE TABLE or analysis
rather than in a task. If that is the reason for keeping a switch, the doc
should say it. Otherwise dropping the config and passing `initialize = false`
unconditionally leaves one less permanent public config to carry.
--
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]
