Skip to content

GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer - #3165

Closed
yuzhu wants to merge 6 commits into
apache:masterfrom
yuzhu:refactorConstructor
Closed

GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer#3165
yuzhu wants to merge 6 commits into
apache:masterfrom
yuzhu:refactorConstructor

Conversation

@yuzhu

@yuzhuyuzhu commented Feb 28, 2025•
edited by wgtmac
Loading

Copy link
Copy Markdown

Rationale for this change

HadoopFile is getting deprecated, it is useful to be able to pass in a parsed footer instead of reading the footer every time

What changes are included in this PR?

a new constructor

Are these changes tested?

Yes

Are there any user-facing changes?

No

Some code duplication is there, because we cant call another constructor and catch the IOException to close the stream. Let me know if you would like this handled in another way. Thanks.

Closes#3141

wgtmac
wgtmac approved these changes Mar 1, 2025
View reviewed changes

@wgtmacwgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have left some minor comments.

Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
try {
this.footer = readFooter(file, options, f, converter);
} catch (IOException e) {
f.close();

@wgtmacwgtmacMar 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f.close();
// In case that reading footer throws an exception in the constructor, the new stream
// should be closed. Otherwise, there's no way to close this outside.
f.close();

Please keep its original comment

Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
}
}

public ParquetFileReader(InputFile file, ParquetMetadata footer, ParquetReadOptions options, SeekableInputStream f)

@wgtmacwgtmacMar 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a javadoc for this new method?

@pan3793pan3793Apr 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the corresponding static open method together

@wgtmac

wgtmac commented Mar 1, 2025

Copy link
Copy Markdown
Member

BTW, could you also add a test case for it?

@pan3793

pan3793 commented Apr 30, 2025

Copy link
Copy Markdown
Member

@yuzhu do you have time to address the above comments? I ported this change into our internal branches, with some modifications on the Spark, vectorized reading could dramatically reduce 3/4 of the namenode RPC.

@yuzhu

yuzhu commented Apr 30, 2025

Copy link
Copy Markdown
Author

@pan3793 glad it helped. I will wrap this up this week.

@pan3793

pan3793 commented Apr 30, 2025

Copy link
Copy Markdown
Member

apache/spark#50765 demonstrates how this PR benefits Spark.

@pan3793

pan3793 commented May 12, 2025

Copy link
Copy Markdown
Member

Kindly ping @yuzhu

@yuzhu
yuzhu requested a review from pan3793June 21, 2025 06:19
@pan3793

pan3793 commented Jun 23, 2025

Copy link
Copy Markdown
Member

@yuzhu thanks for updating. I think you need a rebase to resolve conflicts

pan3793
pan3793 reviewed Jun 23, 2025
View reviewed changes
Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java Outdated
} catch (Exception e) {
} catch (IOException e) {
// In case that reading footer throws an exception in the constructor, the new stream
// should be closed. Otherwise, there's no way to close this outside.

@pan3793pan3793Jun 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// should be closed. Otherwise, there's no way to close this outside.
// should be closed. Otherwise, there's no way to close this outside.

@yuzhu
yuzhuforce-pushed the refactorConstructor branch from e7f187a to cce9f1fCompareJune 23, 2025 02:39
@yuzhu
yuzhu requested a review from pan3793June 23, 2025 02:43
pan3793
pan3793 reviewed Jun 23, 2025
View reviewed changes
Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java Outdated
try {
this.footer = readFooter(file, options, f, converter);
} catch (Exception e) {
} catch (IOException e) {

@pan3793pan3793Jun 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change it?

@yuzhuyuzhuJun 23, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was not intentional. thanks for catching it.

@pan3793

pan3793 commented Jun 23, 2025

Copy link
Copy Markdown
Member

code change lgtm, better to have to test coverage as requested by @wgtmac

@yuzhu

yuzhu commented Jun 23, 2025

Copy link
Copy Markdown
Author

is there an example unit test to model after? I didnt find any unit test for this ParquetFileReader.java file

@wangyum

wangyum commented Jul 11, 2025

Copy link
Copy Markdown
Member

cc @yuzhu
image

@wgtmac

wgtmac commented Jul 14, 2025

Copy link
Copy Markdown
Member
Error: Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.30.0:check (default) on project parquet-hadoop: The following files had format violations:
Error: src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
Error: @@ -739,7 +739,8 @@
Error: ···*·@return·an·open·ParquetFileReader
Error: ···*·@throws·IOException·if·there·is·an·error·while·opening·the·file
Error: ···*/
Error: -··public·static·ParquetFileReader·open(InputFile·file,·ParquetMetadata·footer,·ParquetReadOptions·options,·SeekableInputStream·f)
Error: +··public·static·ParquetFileReader·open(
Error: +······InputFile·file,·ParquetMetadata·footer,·ParquetReadOptions·options,·SeekableInputStream·f)
Error: ······throws·IOException·{
Error: ····return·new·ParquetFileReader(file,·footer,·options,·f);
Error: ··}
Error: Run 'mvn spotless:apply' to fix these violations.
Error: -> [Help 1]
Error: Error: To see the full stack trace of the errors, re-run Maven with the -e switch.
Error: Re-run Maven using the -X switch to enable full debug logging.
Error: Error: For more information about the errors and possible solutions, please read the following articles:
Error: [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
Error: Error: After correcting the problems, you can resume the build with the command
Error: mvn <args> -rf :parquet-hadoop
Error: Process completed with exit code 1.

Please fix the style issue. @yuzhu

@wangyum

wangyum commented Jul 31, 2025

Copy link
Copy Markdown
Member

@yuzhu@pan3793 Any update?

@pan3793

pan3793 commented Aug 1, 2025

Copy link
Copy Markdown
Member

I'd like to pick it up if the original author does not have time to continue this PR.

@pan3793pan3793 mentioned this pull request Aug 6, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmacwgtmacwgtmac approved these changes
+1 more reviewer
@pan3793pan3793pan3793 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add more constructors to ParquetFileReader

4 participants

@yuzhu@wgtmac@pan3793@wangyum