Re: [PR] GH-3697: Reuse hadoop file status and footer in ParquetRecordReader [parquet-java]

["wgtmac (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@parquet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org