GH-3574: Keep null_count when min/max stats exceed the size limit - #3737
Closed
arimu1 wants to merge 1 commit into
Closed
GH-3574: Keep null_count when min/max stats exceed the size limit#3737arimu1 wants to merge 1 commit into
arimu1 wants to merge 1 commit into
Conversation
Omit oversized min/max statistics, but still write null_count and nan_count. parquet-format recommends always writing null_count even when it is zero.
Author
|
Closing: this re-lands #3575, which was reverted in #3688 because Iceberg Sorry for the noise — #3574 remains valid once that consumer contract is evolved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
When column statistics exceed the max size for truncation,
toParquetStatisticscurrently omits the entireStatisticspayload, includingnull_count. Dropping min/max is reasonable (there is no way to mark a truncated bound as a lower/upper bound rather than a true min/max).null_countandnan_countare tiny counters and should still be written.Missing
null_countcauses downstream readers to treat the column as if it might contain nulls. Snowflake reports:parquet-format recommends always writing
null_count, even when it is zero.This re-lands the parquet-java side of #3575 after it was reverted in #3688 to unblock a release. The format contract is that min/max and null counts are independent:
Statistics.hasNonNullValue()vsStatistics.isNumNullsSet(). Consumers that assumed "all stats or no stats" should treat a missing min/max independently of a presentnull_count. Iceberg already observesnum_nulls: N, min/max not definedon older files.What changes are included in this PR?
In
ParquetMetadataConverter.toParquetStatistics, writenull_count(andnan_countwhen set) whenever stats are non-empty, even if min/max are omitted because they exceedMAX_STATS_SIZE. Min/max are still not written in that case (not truncated to an unmarked bound).Are these changes tested?
Yes.
TestParquetMetadataConverter.testBinaryStatsV1/testBinaryStatsV2now assert that oversized binary min/max are omitted whilenull_countis still written and round-trips.TestParquetMetadataConverter: 72 tests, 0 failures (Temurin 21, macOS aarch64).On current
masterwithout this change, the same assertions fail (null_countexpected 3004, actual 0).Are there any user-facing changes?
Yes: files with oversized column min/max now still carry
null_count(andnan_countwhen set) in column-chunk statistics. Readers that previously assumed the statistics object was all-or-nothing should usehasNonNullValue()/isSetMin_value()for bounds andisNumNullsSet()/isSetNull_count()for nulls.Closes #3574