Core: Refactor ContentStats and FieldStats#17159
Conversation
| Types.StructType type(); | ||
|
|
||
| /** Returns a copy of these {@link ContentStats}, deep-copying the contained field stats. */ | ||
| ContentStats copy(); |
There was a problem hiding this comment.
Needed to implement copy in TrackedFile correctly. We also need to follow up with methods to project by field ID.
| * <p>Note: This type may be a projection of the stats stored in manifest files. | ||
| */ | ||
| Type type(); | ||
| Types.StructType type(); |
There was a problem hiding this comment.
This changed to the actual schema. The lower and upper bounds types do not always match (for geo types) so this is the best way to expose types. It is also more consistent with existing conventions.
|
|
||
| /** The total NaN value count */ | ||
| Long nanValueCount(); | ||
| long nanValueCount(); |
There was a problem hiding this comment.
These changes are conservative. We may want to widen later, but I want to make a conscious choice.
|
|
||
| private Object getOffset(int offset) { | ||
| return switch (offset) { | ||
| case StatsUtil.LOWER_BOUND_OFFSET -> lowerBound(); |
There was a problem hiding this comment.
Uses offsets to avoid a switch by position immediately followed by a switch by enum symbol.
|
|
||
| private static final Types.StructType CONTENT_STATS_TYPE = | ||
| StatsUtil.contentStatsFor(TABLE_SCHEMA).type().asStructType(); | ||
| StatsUtil.statsReadSchema(TABLE_SCHEMA, ImmutableList.of(1, 2)); |
There was a problem hiding this comment.
The read schema method is easiest to call for testing since it creates full stats structs for each field ID. The write schema method will omit lower and upper bounds for MetricsMode.Counts.
| 10L, | ||
| 3L, | ||
| null); | ||
| private static final ContentStatsStruct CONTENT_STATS = |
There was a problem hiding this comment.
I left this in to continue testing the MetricsUtil conversion, but I suspect that those conversions will be done lazily.
Those are the only place where fieldId is needed so I think we can avoid needing to recover field ID from base ID if we rewrite them.
| assertThat(dataFile.equalityFieldIds()).isNull(); | ||
| assertThat(dataFile.columnSizes()).isNull(); | ||
| assertThat(dataFile.valueCounts()).containsOnly(Map.entry(1, 100L), Map.entry(2, 200L)); | ||
| assertThat(dataFile.valueCounts()).containsOnly(Map.entry(1, 100L), Map.entry(2, 100L)); |
There was a problem hiding this comment.
Updated to align with what stats would actually look like.
rdblue
force-pushed
the
fix-content-stats
branch
from
b35b148 to
f996392
Compare
1 participant
This updates the content stats API and implementations used for columnar stats in v4.
API changes:
StructLikeused for serialization from the interfacescopymethods to support copyingTrackedFilewith statsIterable, expose full struct type astype, etc.)StatsUtilchanges:FieldStatisticenum; not all offsets were represented asFieldStatisticand it also embedded util methods. All utils are now inStatsUtil.switchstatement to resolve position to enum and enum to field inFieldStatsStructgetandsetMetricsConfigContentStatsimplementation changes:ContentStatsStructto align with other v4 typesStructLikemethods without copying data to allow object reuse, do not rebuild field statssetCustomTypeinstead of copying via builder insetFieldStatsimplementation changes:FieldStatsStructto align with other v4 typesSupportsIndexProjectionthat is bypassedgetandsetprojection directly based on offset, do not rely on mapping passed to constructorjavaClass.castequalsandhashCodeTest changes:
StatsUtiltest values from production constants