Core: guard MetricsUtil.nullValueCounts for columns without null_value_count - #17306
Core: guard MetricsUtil.nullValueCounts for columns without null_value_count#17306stevenzwu wants to merge 1 commit into
Conversation
| if (fs != null) { | ||
| // null_value_count is only stored for optional (nullable) columns; skip columns that do not | ||
| // track it so the primitive nullValueCount() accessor is never called on an absent value. | ||
| if (fs != null |
There was a problem hiding this comment.
this guard is the main fix of this PR
…e_count MetricsUtil.nullValueCounts(ContentStats) was added in apache#16100, when the FieldStats count accessors returned boxed Long, so a column without a null_value_count simply produced a null map entry. apache#17159 changed those accessors to primitive long. In v4 content stats, null_value_count is only stored for optional (nullable) columns: StatsUtil.fieldStatsStruct omits the field for required columns, so a deserialized FieldStatsStruct for a required column leaves it null. nullValueCounts now throws a NullPointerException unboxing that null. Add StatsUtil.tracksStat(fieldStatsType, fieldId, statOffset) and guard nullValueCounts with it, skipping columns whose stats struct does not track null_value_count (reported as unknown) instead of throwing, as nanValueCounts already filters by whether the metric applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b4869c4 to
17a34f5
Compare
| import org.apache.iceberg.types.Types; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class TestMetricsUtil { |
There was a problem hiding this comment.
while adding test coverage for the MetricsUtil.nullValueCounts(stats) util method, I thought it is good to let AI generate other test coverage code too.
| * Returns whether the field stats struct for the given field ID tracks the metric at the offset. | ||
| */ | ||
| static boolean tracksStat(Types.StructType fieldStatsType, int fieldId, int statOffset) { | ||
| return fieldStatsType.field(toBaseId(fieldId) + statOffset) != null; |
There was a problem hiding this comment.
I thought field id is the more canonical identifier and hence went with this route.
Another option is to use field name to get the field.
static boolean tracksStat(Types.StructType fieldStatsType, String statFieldName) {
return fieldStatsType.field(statFieldName) != null;
}
| /** | ||
| * Returns whether the field stats struct for the given field ID tracks the metric at the offset. | ||
| */ | ||
| static boolean tracksStat(Types.StructType fieldStatsType, int fieldId, int statOffset) { |
There was a problem hiding this comment.
I really don't think this is the right approach.
We need to signal whether a stat is present or not and I'm not confident that the right way to do that is to return it as null. That's an open question, but I think the range of possibilities is either to go back to Long for null count, or to add a specific hasNullCount() accessor, not to have a util method that does the null check.
This is going to be part of the PR I'm working on with an evaluator.
| // null_value_count is only stored for optional (nullable) columns; skip columns that do not | ||
| // track it so the primitive nullValueCount() accessor is never called on an absent value. | ||
| if (fs != null | ||
| && StatsUtil.tracksStat(fs.type(), fs.fieldId(), StatsUtil.NULL_VALUE_COUNT_OFFSET)) { |
There was a problem hiding this comment.
I don't think that we should be "fixing" this. I probably should have objected more strongly to adding these conversion methods because I don't think it makes sense to convert to maps. Either we should expose everything as ContentStats and not translate at all, or we should make Map implementations that lazily convert from ContentStats in get.
|
discussed with Ryan offline. we will go with a diff direction |
2 participants
What
MetricsUtil.nullValueCounts(ContentStats)was added in #16100, when theFieldStatscount accessors returned boxedLong— a column without anull_value_countsimply produced a null map entry. #17159 (Refactor ContentStats and FieldStats) changed those accessors to primitivelong.In v4 content stats,
null_value_countis only stored for optional (nullable) columns —StatsUtil.fieldStatsStructomits the field forrequiredcolumns — so a deserializedFieldStatsStructfor a required column leaves it null.nullValueCountsnow throws aNullPointerExceptionunboxing that null.Fix
Add
StatsUtil.tracksStat(fieldStatsType, fieldId, statOffset)and guardnullValueCountswith it, so columns whose stats struct does not tracknull_value_countare skipped (reported as unknown) instead of throwing. This mirrors hownanValueCountsalready filters by whether the metric applies.Tests
TestStatsUtil#testTracksStat— required column → false, optional → true;value_count→ true for both.TestMetricsUtil(new):value_count/null_value_count/nan_value_countconverters plus the null-input contract, including the required-column case that previously threw.🤖 Generated with Claude Code