nsivabalan commented on code in PR #20079:
URL: https://github.com/apache/hudi/pull/20079#discussion_r4175204344
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaEvolutionUtils.scala:
##########
@@ -80,9 +81,21 @@ class ParquetSchemaEvolutionUtils(sharedConf: Configuration,
protected var typeChangeInfos: java.util.Map[Integer, Pair[DataType,
DataType]] = null
- def getHadoopConfClone(footerFileMetaData: FileMetaData,
enableVectorizedReader: Boolean): Configuration = {
- // Clone new conf
- val hadoopAttemptConf = new Configuration(sharedConf)
+ /**
+ * Returns the configuration to read the file with: the read configuration
itself when the file needs no
+ * keys of its own, otherwise a copy with the file's requested schema. Pass
`writable` when the caller sets
+ * keys on the returned configuration. The read configuration is never
modified, since other readers may
+ * share it.
+ */
+ def getFileReadConf(footerFileMetaData: FileMetaData,
enableVectorizedReader: Boolean, writable: Boolean): Configuration = {
+ // A JobConf, so the task attempt context built on it does not copy it
again
+ var fileReadConf: Configuration = if (writable) new JobConf(readConf) else
readConf
Review Comment:
The `writable` flag is the load-bearing correctness change here, and I
checked every call site: all six per-version readers (3.3, 3.4, 3.5, 4.0, 4.1,
4.2) pass `writable = pushed.isDefined`, and in each the only post-call write
to the returned configuration is the
`ParquetInputFormat.setFilterPredicate(hadoopAttemptContext.getConfiguration,
pushed.get)` guarded by the same `pushed.isDefined`. So the flag and the actual
writes cannot drift apart today.
What makes me comfortable rather than nervous is that the aliasing is
fail-fast in the right direction: if a future caller writes without passing
`writable = true`, it mutates the shared read configuration, which is a silent
cross-file bug rather than a crash. Two cheap options to keep that from
regressing, either is fine:
- have this return a wrapper or document the contract on the method as "the
result is only writable when `writable = true`", which the scaladoc nearly does
already; or
- in the `!writable` branch, return something that makes an accidental write
loud in tests.
Not blocking, since the current call sites are all correct and
`TestParquetSchemaEvolutionUtils` covers both branches. Worth a thought given
your own note about revisiting this once the broadcasting changes in #20064
land, which is exactly when a new caller is most likely to appear.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/util/ParquetUtils.java:
##########
@@ -196,6 +198,25 @@ public void close() {
}
}
+ /**
+ * Sets the Hadoop read options of a reader built with {@code
ParquetReader.Builder(InputFile)} from the
+ * file's {@link Configuration}, as {@code ParquetReader.Builder(Path)}
followed by {@code withConf} does,
+ * without creating a new {@link Configuration}. {@code withConf} rebuilds
the read options from the
+ * configuration; the decryption properties are the only option that takes
the file path, which
+ * {@code withConf} drops, so they are resolved here with it. On parquet
1.14+ the {@code InputFile}
+ * constructor alone builds plain {@code ParquetReadOptions}, which never
consult the
+ * {@code parquet.crypto.factory.class} decryption factory.
+ */
+ public static <T> ParquetReader.Builder<T>
withHadoopReadOptions(ParquetReader.Builder<T> builder, HadoopInputFile file) {
Review Comment:
Verified this against the parquet bytecode rather than taking it on trust,
and the reasoning holds exactly. Recording what I checked so the next reader
does not have to redo it:
- `ParquetReader.Builder.withConf` calls `HadoopReadOptions.builder(conf,
this.path)` using the builder's own `path` field, which is null when the
builder came from the `InputFile` constructor. So on 1.12/1.13 the options are
rebuilt with a null path, not merely "left alone".
- `HadoopReadOptions.Builder.build()` only calls
`createDecryptionProperties(filePath, conf)` when `fileDecryptionProperties` is
still null, and that helper does `loadFactory(conf)` then
`getFileDecryptionProperties(conf, filePath)`. With a null path the factory
would receive null.
- This helper mirrors that helper exactly (`loadFactory`, null check,
`getFileDecryptionProperties` with the real path) and calls `withDecryption` so
the pre-set properties win over the null-path resolution in `build()`.
- `withDecryption` and `set` both write into `optionsBuilder`, so the
`.set(...)` calls chained after this in `HoodieAvroParquetReader` are not lost.
Ordering is fine.
- `ParquetReader.Builder(InputFile)` (protected) and the
`DecryptionPropertiesFactory` signatures are present and unchanged in 1.12.2,
1.13.1 and 1.15.2.
One suggestion: fold the first three bullets into the javadoc as the reason
this method exists. Right now the comment says `withConf` "drops" the path,
which undersells it; the path was never populated, and the null would reach the
user's decryption factory. That is the detail a future reader needs when a
parquet upgrade changes this.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/SparkParquetReaderBase.scala:
##########
@@ -64,7 +65,8 @@ abstract class SparkParquetReaderBase(enableVectorizedReader:
Boolean,
filters: Seq[Filter],
storageConf: StorageConfiguration[Configuration],
tableSchemaOpt:
util.Option[org.apache.parquet.schema.MessageType] = util.Option.empty()):
Iterator[InternalRow] = {
- val conf = storageConf.unwrapCopy()
+ // A JobConf, so the task attempt context doRead builds on it reuses it
instead of copying it again
+ val conf = new JobConf(storageConf.unwrap())
Review Comment:
Confirmed the `JobConf` trick does what the comment says. `JobContextImpl`'s
constructor is `instanceof JobConf` then `checkcast` and reuse, else `new
JobConf(conf)`, so passing a `JobConf` here genuinely removes one full
configuration copy per file rather than moving it.
Worth noting in the comment that this depends on a Hadoop implementation
detail, so if `JobContextImpl` ever stops reusing the instance the change is a
silent perf regression rather than a failure. A line naming `JobContextImpl`
specifically would point the next person at what to re-check.
--
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]
