Core: Add content stats evaluator - #17413
Conversation
The content stats schema omitted null_value_count whenever a field was required, but a required field nested in an optional struct is null on every row where its parent is null, and Parquet reports that count from the leaf column's definition levels. Include the count for any field that may be null, either because it or one of its ancestors is optional.
Content stats omit null_value_count for a field that cannot be null, but InclusiveStatsEvaluator read the missing count as unknown and stopped pruning files for notEqual, notIn, and notStartsWith. The v3 metrics maps carry an explicit zero from Parquet, so the same filters pruned there. Detect the fields that cannot be null the same way expression binding does, by requiring the field and all of its ancestors to be required, and report no nulls for them.
github-actions
Bot
added
API
core
Specification
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.iceberg.expressions; |
There was a problem hiding this comment.
I think this forced promotion of package private v4 class into public and result in the revapi breakage. I am wondering if we shall consider the alternative below ?
- increase the visibility of InclusiveEvalVisitor into public
- move
InclusiveStatsEvaluatorto org.apache.iceberg package with package private class.
This avoid the unnecessary change to ContentStats, FieldStats and TrackedFile and revapi. We can eventually move InclusiveStatsEvaluator to org.apache.iceberg.expressions package once it's v4 ready.
This also require expose VariantExpressionUtil.castTo() as a protected method in InclusiveEvalVisitor.
|
|
||
| Field-level structs in `content_stats` are based on the corresponding table field's type, requirement, and ID (`field-id`). | ||
|
|
||
| Stats are tracked for primitive and `variant` fields. Stats are not tracked for `struct`, `list`, and `map` fields, or for any field contained in a `list` or `map` because it may be repeated. |
There was a problem hiding this comment.
maybe consider move the spec to a separate PR?
| | _optional_ | 3 | `tight_bounds` | `boolean` | all except `geometry`, `geography`, `variant` | 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` | fields that may be null | Number of null values in the column | |
There was a problem hiding this comment.
looks like we now omit the fields which is definitely required, null count is guaranteed to be 0. Curious, is this intentional depart from v1-v3 null_count?
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
I think this method is very similar to InclusiveStatsEvaluator::allAncestorFieldsAreRequired, since both are purely schema based and have nothing to do with stats, maybe consider move to the schema.isNullable(int fieldId) instead as it already have the fieldId of all schema cached lazily?
In reader, it can use referencedIds.stream().filter(id -> !schema.isNullable(id)) and in writer it can be fieldStatsStruct(tableSchema.isNullable(id), ...).
| import org.apache.iceberg.types.Types; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** Runs the inclusive evaluation tests against {@link ContentStats} tracked by a file. */ |
There was a problem hiding this comment.
nit: probably don't need this
2 participants
No description provided.