You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "ArnavBalyan (via GitHub)" <gi...@apache.org> on 2025/09/23 09:27:35 UTC
[PR] [GH-3320] Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
ArnavBalyan opened a new pull request, #3325:
URL: https://github.com/apache/parquet-java/pull/3325
### Rationale for this change
- Parquet reader fails if upstream writes wrong statistics in column index vs offset index.
- Ensure parquet java can still read the file and ignore corrupt metadata in such cases.
- Added UT to ensure existing behaviour is preserved.
- Fixes #3320
### Are these changes tested?
- Yes
### Are there any user-facing changes?
- Yes
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2376155381
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnIndexFiltering.java:
##########
@@ -650,4 +660,75 @@ public void testFilteringWithProjection() throws IOException {
false,
true));
}
+
+ @Test
+ public void testValidMetadata() throws Exception {
Review Comment:
we need the buggy parquet-go lib to create the invalid index, current check is in the column read flow, will only fail if the upstream wrote invalid data, is there a better way to test 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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2376149247
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/ColumnIndexFilter.java:
##########
@@ -220,4 +225,35 @@ public RowRanges visit(Not not) {
throw new IllegalArgumentException(
"Predicates containing a NOT must be run through LogicalInverseRewriter. " + not);
}
+
+ /**
+ * Validates that column index and offset index metadata are consistent and can be used safely.
+ *
+ * @param columnIndex the column index to validate
+ * @param offsetIndex the offset index to validate
+ * @param columnPath the column path for error reporting
+ * @return true if metadata is valid and safe to use, false if corrupt and should be ignored
+ */
+ private static boolean isValidMetadata(ColumnIndex columnIndex, OffsetIndex offsetIndex, ColumnPath columnPath) {
+ if (columnIndex == null || offsetIndex == null) {
+ return true;
+ }
Review Comment:
updated thanks
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2376152332
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/ColumnIndexFilter.java:
##########
@@ -220,4 +225,35 @@ public RowRanges visit(Not not) {
throw new IllegalArgumentException(
"Predicates containing a NOT must be run through LogicalInverseRewriter. " + not);
}
+
+ /**
+ * Validates that column index and offset index metadata are consistent and can be used safely.
+ *
+ * @param columnIndex the column index to validate
+ * @param offsetIndex the offset index to validate
+ * @param columnPath the column path for error reporting
+ * @return true if metadata is valid and safe to use, false if corrupt and should be ignored
+ */
+ private static boolean isValidMetadata(ColumnIndex columnIndex, OffsetIndex offsetIndex, ColumnPath columnPath) {
Review Comment:
updated to `isValidIndexSize`, was initially planned to be a general pre-check which can be enhanced in the future. For now restricting to index size.
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2375001325
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnIndexFiltering.java:
##########
@@ -650,4 +660,75 @@ public void testFilteringWithProjection() throws IOException {
false,
true));
}
+
+ @Test
+ public void testValidMetadata() throws Exception {
Review Comment:
Shouldn't we test invalid page index?
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/ColumnIndexFilter.java:
##########
@@ -220,4 +225,35 @@ public RowRanges visit(Not not) {
throw new IllegalArgumentException(
"Predicates containing a NOT must be run through LogicalInverseRewriter. " + not);
}
+
+ /**
+ * Validates that column index and offset index metadata are consistent and can be used safely.
+ *
+ * @param columnIndex the column index to validate
+ * @param offsetIndex the offset index to validate
+ * @param columnPath the column path for error reporting
+ * @return true if metadata is valid and safe to use, false if corrupt and should be ignored
+ */
+ private static boolean isValidMetadata(ColumnIndex columnIndex, OffsetIndex offsetIndex, ColumnPath columnPath) {
Review Comment:
It might be misleading to readers that this function does not validate everything but just `columnIndex.getMinValues().size()` vs `offsetIndex.getPageCount()`. But I'm not sure if we want to pay for the cost of validating everything.
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2378785189
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnIndexFiltering.java:
##########
@@ -650,4 +660,75 @@ public void testFilteringWithProjection() throws IOException {
false,
true));
}
+
+ @Test
+ public void testValidMetadata() throws Exception {
Review Comment:
updated thanks
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2374493098
##########
parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/ColumnIndexFilter.java:
##########
@@ -220,4 +225,35 @@ public RowRanges visit(Not not) {
throw new IllegalArgumentException(
"Predicates containing a NOT must be run through LogicalInverseRewriter. " + not);
}
+
+ /**
+ * Validates that column index and offset index metadata are consistent and can be used safely.
+ *
+ * @param columnIndex the column index to validate
+ * @param offsetIndex the offset index to validate
+ * @param columnPath the column path for error reporting
+ * @return true if metadata is valid and safe to use, false if corrupt and should be ignored
+ */
+ private static boolean isValidMetadata(ColumnIndex columnIndex, OffsetIndex offsetIndex, ColumnPath columnPath) {
+ if (columnIndex == null || offsetIndex == null) {
+ return true;
+ }
Review Comment:
Based on the code it seems they cannot be null at this stage.
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2376155381
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnIndexFiltering.java:
##########
@@ -650,4 +660,75 @@ public void testFilteringWithProjection() throws IOException {
false,
true));
}
+
+ @Test
+ public void testValidMetadata() throws Exception {
Review Comment:
we need the buggy parquet-go lib to create the invalid index, current check is in the column read flow, will only fail if the upstream wrote invalid data
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#discussion_r2377865835
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnIndexFiltering.java:
##########
@@ -650,4 +660,75 @@ public void testFilteringWithProjection() throws IOException {
false,
true));
}
+
+ @Test
+ public void testValidMetadata() throws Exception {
Review Comment:
If this test does not cover the new case, perhaps we can simply revert all changes in this file?
A better way would be uploading the corrupted data produced by parquet-go to https://github.com/apache/parquet-testing/tree/master/bad_data. But I do not think it worths the effort...
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#issuecomment-3347717783
Thanks @ArnavBalyan and @gszadovszky!
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#issuecomment-3346392124
cc @wgtmac gentle reminder thanks
--
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-3320: Ensure parquet reader does not fail due to incorrect statistics [parquet-java]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #3325:
URL: https://github.com/apache/parquet-java/pull/3325#issuecomment-3323692635
cc @gszadovszky could you pls take a look
--
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