GH-3697: Reuse hadoop file status and footer in ParquetRecordReader - #3698
GH-3697: Reuse hadoop file status and footer in ParquetRecordReader#3698steveloughran wants to merge 3 commits into
Conversation
…ader - Ignore InputFile field in ShowFooterCommand - Create ParquetFileReader with InputFile instead of HadoopInputFile Was PARQUET-2415 Contains contributions by Claude.ai (rebasing; junit5/assertj migration) Contributed by wankunde
verify that the metadata isn't included in json or writable marshalling.
|
AFAK, this changed has been ported into several cloud products. |
There was a problem hiding this comment.
These review comments are generated by Codex. I've just checked them and all of these seem non-blocking to me. Let me know what you think @steveloughran @wankunde.
| * @param footer footer of the file this split refers to | ||
| */ | ||
| public void setFooter(ParquetMetadata footer) { | ||
| this.footer = footer; |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good one. Needs fix.
| : HadoopInputFile.fromPath(path, configuration); | ||
| ParquetReadOptions options = optionsBuilder.build(); | ||
| ParquetFileReader reader = footer != null | ||
| ? ParquetFileReader.open(inputFile, footer, options, inputFile.newStream()) |
There was a problem hiding this 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.
| * 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 |
There was a problem hiding this 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.
|
I will try to fix but can't set a timeline. If you haven't heard I left cloudera on Friday, I do want to wrap up the recent PRs but I need to set up my mac mini as a dev box first, with the pi5 rebuilt as the docker server. That's where the hadoop 3.4.3 jars on maven central came from, FWIW |
3 participants
Contains contributions by Claude.ai (rebasing; junit5/assertj migration)
Contributed by @wankunde; made current by claude.
Rationale for this change
HTTP HEAD is expensive, slow and comes out of your quota of a few thousand reads/second
What changes are included in this PR?
The InputFile used when reading a file footer is cached in the input split, so all splits read in the same process only will use that and any cached FileStatus when opening the file. This saves a HEAD against s3, azure and gcs on every split read within that process.
Are these changes tested?
new test, otherwise all internal.
Are there any user-facing changes?
no
Closes #3697