You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "keuin (via GitHub)" <gi...@apache.org> on 2026/05/25 06:18:31 UTC
[PR] GH-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
keuin opened a new pull request, #3586:
URL: https://github.com/apache/parquet-java/pull/3586
<!--
Thanks for opening a pull request!
If you're new to Parquet-Java, information on how to contribute can be found here: https://parquet.apache.org/docs/contribution-guidelines/contributing
Please open a GitHub issue for this pull request: https://github.com/apache/parquet-java/issues/new/choose
and format pull request title as below:
GH-${GITHUB_ISSUE_ID}: ${SUMMARY}
or simply use the title below if it is a minor issue:
MINOR: ${SUMMARY}
-->
### Rationale for this change
Fix the num_nulls field in DataPageHeaderV2, which will be unexpectedly set to -1 when statistics is disabled.
### What changes are included in this PR?
Add a counter tracker in ColumnValueCollector, which is always enabled, to track required field num_nulls.
### Are these changes tested?
Yes, tested. The test will fail before this fix, and will succeed with this fix.
### Are there any user-facing changes?
No user-facing or API changes.
<!-- Please uncomment the line below and replace ${GITHUB_ISSUE_ID} with the actual Github issue id. -->
Closes #3585
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "keuin (via GitHub)" <gi...@apache.org>.
keuin commented on code in PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#discussion_r3320258614
##########
parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnValueCollector.java:
##########
@@ -45,6 +45,10 @@ class ColumnValueCollector {
private SizeStatistics.Builder sizeStatisticsBuilder;
private GeospatialStatistics.Builder geospatialStatisticsBuilder;
+ // track the required `num_nulls` field in DataPageHeaderV2
+ // https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift
+ private int nullCount;
Review Comment:
Yes, placing it inside ColumnWriterBase is the correct way. Fixed.
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#discussion_r3315110625
##########
parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnValueCollector.java:
##########
@@ -45,6 +45,10 @@ class ColumnValueCollector {
private SizeStatistics.Builder sizeStatisticsBuilder;
private GeospatialStatistics.Builder geospatialStatisticsBuilder;
+ // track the required `num_nulls` field in DataPageHeaderV2
+ // https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift
+ private int nullCount;
Review Comment:
Could we keep this counter in ColumnWriterBase instead? nullCount is page-level metadata, like valueCount and pageRowCount, while ColumnValueCollector is otherwise about optional statistics and bloom filters. Keeping it with the other page counters would make the ownership clearer.
##########
parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnWriterBase.java:
##########
@@ -403,6 +404,7 @@ void writePage() {
abstract void writePage(
int rowCount,
int valueCount,
+ int nullCount,
Review Comment:
This adds a V2-only value to the shared writePage signature, so ColumnWriterV1 has to accept an unused parameter. Consider keeping nullCount on the V2 path only, either through a V2-specific helper or a protected getter from the base class.
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetWriter.java:
##########
@@ -858,4 +863,68 @@ public void testNoFlushAfterException() throws Exception {
FileSystem fs = file.getFileSystem(conf);
assertTrue(!fs.exists(file) || fs.getFileStatus(file).getLen() == 0);
}
+
+ @Test
+ public void testV2PageNullCountWithStatisticsDisabled() throws Exception {
+ // Regression test: when using PARQUET_2_0 with statistics disabled on a nullable column,
+ // DataPageHeaderV2.num_nulls must still contain the correct null count (not -1).
+ MessageType schema = Types.buildMessage()
+ .required(INT32)
+ .named("id")
+ .optional(BINARY)
+ .as(stringType())
+ .named("value")
+ .named("test_schema");
+
+ File file = temp.newFile();
+ temp.delete();
Review Comment:
This should delete only the placeholder file, not the TemporaryFolder root. file.delete() is enough to free the output path and avoids changing the test fixture lifecycle.
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "keuin (via GitHub)" <gi...@apache.org>.
keuin commented on code in PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#discussion_r3298406999
##########
parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnValueCollector.java:
##########
@@ -54,6 +58,7 @@ class ColumnValueCollector {
}
void resetPageStatistics() {
+ this.nullCount = 0;
this.statistics = statisticsEnabled
? Statistics.createStats(path.getPrimitiveType())
: Statistics.noopStats(path.getPrimitiveType());
Review Comment:
It's possible, but putting it outside `Statistics` is a design choice. By reading code I think the `Statistics` object is a 1:1 mapping to parquet header's `statistics` object, which is optional and may be disabled by user.
Putting counter of `num_nulls` outside `statistics` object makes the code clearer, because the code maps clearly to parquet header structure.
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "keuin (via GitHub)" <gi...@apache.org>.
keuin commented on code in PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#discussion_r3320267055
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetWriter.java:
##########
@@ -858,4 +863,68 @@ public void testNoFlushAfterException() throws Exception {
FileSystem fs = file.getFileSystem(conf);
assertTrue(!fs.exists(file) || fs.getFileStatus(file).getLen() == 0);
}
+
+ @Test
+ public void testV2PageNullCountWithStatisticsDisabled() throws Exception {
+ // Regression test: when using PARQUET_2_0 with statistics disabled on a nullable column,
+ // DataPageHeaderV2.num_nulls must still contain the correct null count (not -1).
+ MessageType schema = Types.buildMessage()
+ .required(INT32)
+ .named("id")
+ .optional(BINARY)
+ .as(stringType())
+ .named("value")
+ .named("test_schema");
+
+ File file = temp.newFile();
+ temp.delete();
Review Comment:
Fixed.
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#discussion_r3297094828
##########
parquet-column/src/main/java/org/apache/parquet/column/impl/ColumnValueCollector.java:
##########
@@ -54,6 +58,7 @@ class ColumnValueCollector {
}
void resetPageStatistics() {
+ this.nullCount = 0;
this.statistics = statisticsEnabled
? Statistics.createStats(path.getPrimitiveType())
: Statistics.noopStats(path.getPrimitiveType());
Review Comment:
Is it possible to change `Statistics.noopStats` to make it possible to ignore min/max stats but still collects null count?
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586
--
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-3585: Fix DataPageHeaderV2.num_nulls=-1 when column statistics are disabled [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3586:
URL: https://github.com/apache/parquet-java/pull/3586#issuecomment-4570052853
Thank you, @keuin!
--
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