Spec: Clarify null_value_count applies to nullable fields - #17609
Conversation
null_value_count was documented as only used for optional fields, but a required field nested within an optional struct is null whenever an ancestor value is null. Define nullability by the full path from root, scope the required-field contract to a present enclosing struct, and require readers to rely on null_value_count == 0 rather than the required flag.
github-actions
Bot
added
the
Specification
|
Linking the discussion thread: https://lists.apache.org/thread/93c86nsm4k4zz3yv86mxrjnzw1blogb0 |
| | _optional_ | 3 | `tight_bounds` | `boolean` | all primitives except for `geometry` and `geography` | When true, `lower_bound` and `upper_bound` must be equal to the min and max values | | ||
| | _optional_ | 4 | `value_count` | `long` | all | Number of values in the column (including null and NaN values) | | ||
| | _optional_ | 5 | `null_value_count` | `long` | optional fields | Number of null values in the column | | ||
| | _optional_ | 5 | `null_value_count` | `long` | nullable fields | Number of null values in the column | |
There was a problem hiding this comment.
This is a breaking change from the current behavior of our statistics.
Right now, our null count matches what Parquet produces, which is a null count that is up to the number of values for the field. If a parent is null, it doesn't show up in the field-level count. It shows up in that the field has a different value count than the parent or the number of records in the data file.
There was a problem hiding this comment.
our null count matches what Parquet produces, which is a null count that is up to the number of values for the field. If a parent is null, it doesn't show up in the field-level count.
I thought this was a bug, and even created a PR for that (#17560). We do see cases where planner was misleaded and produced wrong results.
I will take a look at #17413 and discuss there.
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
-1 on this change.
This is a breaking change that would invalidate existing statistics. It also introduces a new confusing but specific term, nullable, that is not needed.
I think we can work on clarifying how to detect nested struct fields, but this is an incorrect way to fix the problem.
|
@xndai, @CTTY, let's try to keep discussion in one place. Right now, this is being discussed and addressed primarily in #17413. That's not a great place to finish the work because this is a separate change, but we can at least keep the discussion there until we identify the changes we need to fix this issue. |
4 participants
null_value_count was documented as only used for optional fields, but a required field nested within an optional struct is null whenever an ancestor value is null. Add definition of nullability by the full path from root, scope the required-field contract to a present enclosing struct, and require readers to rely on null_value_count == 0 rather than the required flag.