yihua commented on code in PR #20079:
URL: https://github.com/apache/hudi/pull/20079#discussion_r4170739120
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaEvolutionUtils.scala:
##########
@@ -80,9 +80,13 @@ 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)
+ /**
+ * Sets the file's requested read schema on the read configuration and
returns it for the task
+ * attempt context. The configuration belongs to this read only (see
[[SparkParquetReaderBase.read]]),
+ * so it is updated in place.
+ */
+ def getHadoopAttemptConf(footerFileMetaData: FileMetaData,
enableVectorizedReader: Boolean): Configuration = {
+ val hadoopAttemptConf = readConf
Review Comment:
Addressed: `getFileReadConf` now copies the read configuration only when the
file needs its own requested schema or the reader pushes a filter into it, so
it never modifies a configuration other readers may share, as in #20102.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaEvolutionUtils.scala:
##########
@@ -80,9 +80,13 @@ 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)
+ /**
+ * Sets the file's requested read schema on the read configuration and
returns it for the task
+ * attempt context. The configuration belongs to this read only (see
[[SparkParquetReaderBase.read]]),
+ * so it is updated in place.
+ */
+ def getHadoopAttemptConf(footerFileMetaData: FileMetaData,
enableVectorizedReader: Boolean): Configuration = {
Review Comment:
Renamed to `getFileReadConf`: it returns the configuration to read the file
with, either the read configuration itself or a per-file copy.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/util/ParquetUtils.java:
##########
@@ -196,6 +198,23 @@ 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}. On parquet 1.15+ that
constructor builds plain
+ * {@code ParquetReadOptions}, which never consult the {@code
parquet.crypto.factory.class} decryption
+ * factory, and {@code withConf} drops the file path, so the decryption
properties are resolved here with it.
+ */
+ public static <T> ParquetReader.Builder<T>
withHadoopReadOptions(ParquetReader.Builder<T> builder, HadoopInputFile file) {
+ Configuration conf = file.getConfiguration();
+ builder.withConf(conf);
+ DecryptionPropertiesFactory decryptionFactory =
DecryptionPropertiesFactory.loadFactory(conf);
+ if (decryptionFactory != null) {
+
builder.withDecryption(decryptionFactory.getFileDecryptionProperties(conf,
file.getPath()));
+ }
Review Comment:
Addressed. Decryption is the only path-dependent option, since `withConf`
rebuilds every other read option from the configuration;
`TestParquetReadOptionsParity` now compares the reader's options field by field
with `HadoopReadOptions.builder(conf, path)` and its rows with the path-based
builder on parquet 1.12.2 through 1.17.0, so a new path-dependent option in a
future parquet fails CI.
--
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]
