You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "yuzhu (via GitHub)" <gi...@apache.org> on 2025/02/28 22:40:20 UTC
[PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
yuzhu opened a new pull request, #3165:
URL: https://github.com/apache/parquet-java/pull/3165
### 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
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-3060814500
cc @yuzhu
<img width="1662" height="1040" alt="image" src="https://github.com/user-attachments/assets/1728e5bf-01a3-4221-98b0-fc26ca9aa263" />
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "yuzhu (via GitHub)" <gi...@apache.org>.
yuzhu commented on code in PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#discussion_r2160639247
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -930,19 +945,70 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options) throws IOEx
this(file, options, file.newStream());
}
+ /**
+ * @param file Path to a parquet file
+ * @param options {@link ParquetReadOptions}
+ * @param f a {@link SeekableInputStream} for the parquet file
+ * @throws IOException if the file can not be opened
+ */
public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInputStream f) throws IOException {
this.converter = new ParquetMetadataConverter(options);
this.file = file;
this.f = f;
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
- } catch (Exception e) {
+ } catch (IOException e) {
Review Comment:
it was not intentional. thanks for catching it.
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#discussion_r2160634107
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -930,19 +945,70 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options) throws IOEx
this(file, options, file.newStream());
}
+ /**
+ * @param file Path to a parquet file
+ * @param options {@link ParquetReadOptions}
+ * @param f a {@link SeekableInputStream} for the parquet file
+ * @throws IOException if the file can not be opened
+ */
public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInputStream f) throws IOException {
this.converter = new ParquetMetadataConverter(options);
this.file = file;
this.f = f;
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
- } catch (Exception e) {
+ } catch (IOException e) {
Review Comment:
why change it?
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-3138412790
@yuzhu @pan3793 Any update?
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2840723512
@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.
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2841372162
https://github.com/apache/spark/pull/50765 demonstrates how this PR benefits Spark.
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#discussion_r2160605932
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -925,19 +940,70 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options) throws IOEx
this(file, options, file.newStream());
}
+ /**
+ * @param file Path to a parquet file
+ * @param options {@link ParquetReadOptions}
+ * @param f a {@link SeekableInputStream} for the parquet file
+ * @throws IOException if the file can not be opened
+ */
public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInputStream f) throws IOException {
this.converter = new ParquetMetadataConverter(options);
this.file = file;
this.f = f;
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
- } 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.
Review Comment:
```suggestion
// should be closed. Otherwise, there's no way to close this outside.
```
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2871918023
Kindly ping @yuzhu
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2692271975
BTW, could you also add a test case for it?
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "yuzhu (via GitHub)" <gi...@apache.org>.
yuzhu commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2840726430
@pan3793 glad it helped. I will wrap this up this week.
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "yuzhu (via GitHub)" <gi...@apache.org>.
yuzhu commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2994880020
is there an example unit test to model after? I didnt find any unit test for this ParquetFileReader.java file
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#discussion_r2055399328
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -932,12 +932,48 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInp
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
+ } catch (IOException e) {
+ f.close();
+ throw e;
+ }
+
+ this.fileMetaData = footer.getFileMetaData();
+ this.fileDecryptor = fileMetaData.getFileDecryptor(); // must be called before filterRowGroups!
+ if (null != fileDecryptor && fileDecryptor.plaintextFile()) {
+ this.fileDecryptor = null; // Plaintext file. No need in decryptor
+ }
+
+ try {
+ this.blocks = filterRowGroups(footer.getBlocks());
} catch (Exception e) {
- // In case that reading footer throws an exception in the constructor, the new stream
+ // In case that filterRowGroups throws an exception in the constructor, the new stream
// should be closed. Otherwise, there's no way to close this outside.
f.close();
throw e;
}
+ this.blockIndexStores = listWithNulls(this.blocks.size());
+ this.blockRowRanges = listWithNulls(this.blocks.size());
+ for (ColumnDescriptor col : footer.getFileMetaData().getSchema().getColumns()) {
+ paths.put(ColumnPath.get(col.getPath()), col);
+ }
+
+ if (options.usePageChecksumVerification()) {
+ this.crc = new CRC32();
+ this.crcAllocator = ReusingByteBufferAllocator.strict(options.getAllocator());
+ } else {
+ this.crc = null;
+ this.crcAllocator = null;
+ }
+ }
+
+ public ParquetFileReader(InputFile file, ParquetMetadata footer, ParquetReadOptions options, SeekableInputStream f)
Review Comment:
Please add the corresponding `static open` method together
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac closed pull request #3165: GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer
URL: https://github.com/apache/parquet-java/pull/3165
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2994702603
@yuzhu thanks for updating. I think you need a rebase to resolve conflicts
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#discussion_r1976433997
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -932,12 +932,48 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInp
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
+ } catch (IOException e) {
+ f.close();
Review Comment:
```suggestion
// 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
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -932,12 +932,48 @@ public ParquetFileReader(InputFile file, ParquetReadOptions options, SeekableInp
this.options = options;
try {
this.footer = readFooter(file, options, f, converter);
+ } catch (IOException e) {
+ f.close();
+ throw e;
+ }
+
+ this.fileMetaData = footer.getFileMetaData();
+ this.fileDecryptor = fileMetaData.getFileDecryptor(); // must be called before filterRowGroups!
+ if (null != fileDecryptor && fileDecryptor.plaintextFile()) {
+ this.fileDecryptor = null; // Plaintext file. No need in decryptor
+ }
+
+ try {
+ this.blocks = filterRowGroups(footer.getBlocks());
} catch (Exception e) {
- // In case that reading footer throws an exception in the constructor, the new stream
+ // In case that filterRowGroups throws an exception in the constructor, the new stream
// should be closed. Otherwise, there's no way to close this outside.
f.close();
throw e;
}
+ this.blockIndexStores = listWithNulls(this.blocks.size());
+ this.blockRowRanges = listWithNulls(this.blocks.size());
+ for (ColumnDescriptor col : footer.getFileMetaData().getSchema().getColumns()) {
+ paths.put(ColumnPath.get(col.getPath()), col);
+ }
+
+ if (options.usePageChecksumVerification()) {
+ this.crc = new CRC32();
+ this.crcAllocator = ReusingByteBufferAllocator.strict(options.getAllocator());
+ } else {
+ this.crc = null;
+ this.crcAllocator = null;
+ }
+ }
+
+ public ParquetFileReader(InputFile file, ParquetMetadata footer, ParquetReadOptions options, SeekableInputStream f)
Review Comment:
Could you add a javadoc for this new method?
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-2994784264
code change lgtm, better to have to test coverage as requested by @wgtmac
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-3142156339
I'd like to pick it up if the original author does not have time to continue this PR.
--
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
Re: [PR] GH-3141: Add constructor to ParquetFileReader to allow passing in parquet footer [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3165:
URL: https://github.com/apache/parquet-java/pull/3165#issuecomment-3067717989
```
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
--
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