Clarify num-nulls handling in Statistics and ColumnIndex #449
+18
−2
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -257,7 +257,14 @@ struct Statistics { | |
| */ | ||
| 1: optional binary max; | ||
| 2: optional binary min; | ||
| /** | ||
| * Count of null values in the column. | ||
| * | ||
| * Writers SHOULD always write this field even if it is zero (i.e. no null value) | ||
| * or the column is not nullable. | ||
| * Readers MUST distinguish between null_count not being present and null_count == 0. | ||
| * If null_count is not present, readers MUST NOT assume null_count == 0. | ||
|
There was a problem hiding this comment. I agree with this statement (even though there is an issue with older parquet files written with parquet-rs) There was a problem hiding this comment. What did parquet-rs do? Not write the null count in case it was null? If so, then readers will just not optimize out null handling in this case, which seems fine. There was a problem hiding this comment. @JFinis apache/arrow-rs#6257 would support write 0 here |
||
| */ | ||
| 3: optional i64 null_count; | ||
| /** count of distinct values occurring */ | ||
| 4: optional i64 distinct_count; | ||
|
|
@@ -1084,7 +1091,16 @@ struct ColumnIndex { | |
| */ | ||
| 4: required BoundaryOrder boundary_order | ||
|
|
||
| /** | ||
| * A list containing the number of null values for each page | ||
| * | ||
| * Writers SHOULD always write this field even if no null values | ||
| * are present or the column is not nullable. | ||
| * Readers MUST distinguish between null_counts not being present | ||
| * and null_count being 0. | ||
|
Comment on lines
+1099
to
+1100
|
||
| * If null_counts are not present, readers MUST NOT assume all | ||
| * null counts are 0. | ||
| */ | ||
| 5: optional list<i64> null_counts | ||
|
|
||
| /** | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should writers write this field if the column isn't nullable? That feels strictly redundant and therefore a waste of memory. If a reader assumes that their could be nulls even though the type is required in the schema, then this reader has a bug. There wouldn't even be a way to write nulls into a required column, as they cannot be represented in this case.
(Also, if you disregard this comment, at least change the wording from
not nullabletonot required, as that's the correct term w.rt. to the schema (there is nonullablefield in the schema, just a repetition type).)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's not costless in this section, however most impl uses this. It's not a standard but it's widely used.
You're right, I'll rechecking this