wgtmac commented on code in PR #3698:
URL: https://github.com/apache/parquet-java/pull/3698#discussion_r3699472107
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetInputSplit.java:
##########
@@ -222,6 +231,24 @@ public long[] getRowGroupOffsets() {
return rowGroupOffsets;
}
+ /**
+ * @return the footer of the file, if it was passed in by whoever built the
split, else null.
+ */
+ public ParquetMetadata getFooter() {
+ return footer;
+ }
+
+ /**
+ * Pass in a footer already read from the file, so that a reader created
from this split does not have to read it
+ * again. As the footer is not serialized with the split, this only has an
effect on readers created in the same
+ * JVM.
+ *
+ * @param footer footer of the file this split refers to
+ */
+ public void setFooter(ParquetMetadata footer) {
+ this.footer = footer;
Review Comment:
`footer` is omitted from `write()`, but `readFields()` never clears it.
Hadoop may reuse Writable instances, so a split can retain the footer from
another file and `ParquetRecordReader` may open the wrong `InputFile`. Please
clear `footer` in `readFields()` and cover object reuse in the round-trip test.
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetRecordReader.java:
##########
@@ -154,9 +156,16 @@ private void initializeInternalReader(ParquetInputSplit
split, Configuration con
optionsBuilder.withRange(split.getStart(), split.getEnd());
}
- // open a reader with the metadata filter
- ParquetFileReader reader =
- ParquetFileReader.open(HadoopInputFile.fromPath(path, configuration),
optionsBuilder.build());
+ // open a reader with the metadata filter, reusing the footer of the split
and the file it was read from
+ // when the split was built by a caller which had already read them
+ ParquetMetadata footer = split.getFooter();
+ InputFile inputFile = footer != null && footer.getInputFile() != null
+ ? footer.getInputFile()
+ : HadoopInputFile.fromPath(path, configuration);
+ ParquetReadOptions options = optionsBuilder.build();
+ ParquetFileReader reader = footer != null
+ ? ParquetFileReader.open(inputFile, footer, options,
inputFile.newStream())
Review Comment:
Readers opened from the same encrypted footer share its mutable
`InternalFileDecryptor` and JCE `Cipher` state, so concurrent splits in one JVM
can race. Please use reader-local decryption state or skip footer reuse for
encrypted files, and add a concurrent encrypted test.
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetInputSplit.java:
##########
@@ -55,6 +57,13 @@ public class ParquetInputSplit extends FileSplit implements
Writable {
private long end;
private long[] rowGroupOffsets;
+ /**
+ * Footer of the file, if the split was built by a caller which had already
read it.
+ * Not written by {@link #write(DataOutput)}, so it is only visible within
the JVM which set it.
+ */
+ @JsonIgnore
Review Comment:
This `JsonIgnore` is relocated in the published jar and is invisible to an
external `ObjectMapper`, as the new CLI mix-in notes. The current test runs
before shading. Please use a serialization-neutral exclusion and test the
shaded artifact; `ParquetMetadata.inputFile` has the same issue.
--
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]
