laskoviymishka commented on code in PR #2051:
URL: https://github.com/apache/iceberg-go/pull/2051#discussion_r4153116106
##########
table/internal/parquet_files.go:
##########
@@ -1602,12 +1602,13 @@ func (p parquetFormat) DataFileStatsFromMeta(meta
Metadata, statsCols map[int]St
panic(err)
}
+ // These metrics come from column chunk metadata, so
keep totaling them even when stats are invalid.
+ colSizes[fieldID] += colChunk.TotalCompressedSize()
+ valueCounts[fieldID] += colChunk.NumValues()
Review Comment:
We now retain `value_counts` for a column whose stats got invalidated
mid-file, where Java drops the column entirely (its `bounds()`/`counts()` bail
to null on the first empty row group). I think ours is actually better, since
`NumValues()` is physical chunk metadata rather than a stat, but it's a real
divergence an "align with Java" pass could revert straight back into the
original bug. A one-line comment marking the retention as deliberate would lock
it in.
##########
table/internal/parquet_files_test.go:
##########
@@ -737,6 +737,27 @@ func
TestDataFileStatsFromMetaWithMalformedFixedLenDecimalStats(t *testing.T) {
assert.NotContains(t, dataFile.UpperBoundValues(), 15)
}
+func TestDataFileStatsFromMetaPreservesCountsWhenStatsAreMissing(t *testing.T)
{
+ format := internal.GetFileFormat(iceberg.ParquetFile)
+
+ meta, tblMeta := constructTestTablePrimitiveTypes(t)
+ secondMeta, _ := constructTestTablePrimitiveTypes(t)
+ meta.RowGroups = append(meta.RowGroups, secondMeta.RowGroups[0])
Review Comment:
This appends the live `*RowGroup` from `secondMeta`, so `meta.RowGroups[1]`
and `secondMeta.RowGroups[0]` end up the same object, and we then feed
`secondMeta` in as the baseline. It's harmless today since the call doesn't
mutate chunk metadata, but the sibling
`TestDataFileStatsFromMetaDoesNotSkipInvalidatedColumnMetadata` deep-copies the
row group exactly to avoid this. I'd match that:
```go
secondRowGroup := *secondMeta.RowGroups[0]
secondRowGroup.Columns = append(secondRowGroup.Columns[:0:0],
secondRowGroup.Columns...)
meta.RowGroups = append(meta.RowGroups, &secondRowGroup)
```
##########
table/internal/parquet_files_test.go:
##########
@@ -737,6 +737,27 @@ func
TestDataFileStatsFromMetaWithMalformedFixedLenDecimalStats(t *testing.T) {
assert.NotContains(t, dataFile.UpperBoundValues(), 15)
}
+func TestDataFileStatsFromMetaPreservesCountsWhenStatsAreMissing(t *testing.T)
{
+ format := internal.GetFileFormat(iceberg.ParquetFile)
+
+ meta, tblMeta := constructTestTablePrimitiveTypes(t)
+ secondMeta, _ := constructTestTablePrimitiveTypes(t)
+ meta.RowGroups = append(meta.RowGroups, secondMeta.RowGroups[0])
+
+ const columnPos = 1 // field id 2: ints
+ meta.RowGroups[0].Columns[columnPos].MetaData.Statistics = nil
+
+ mapping, err := format.PathToIDMapping(tblMeta.CurrentSchema())
+ require.NoError(t, err)
+ stats := format.DataFileStatsFromMeta(internal.Metadata(meta),
getCollector(), mapping, nil, nil)
+ secondGroupStats :=
format.DataFileStatsFromMeta(internal.Metadata(secondMeta), getCollector(),
mapping, nil, nil)
+
+ assert.Equal(t, 2*secondGroupStats.ColSizes[2], stats.ColSizes[2])
Review Comment:
Both this line and the `ValueCounts[2]` assert compare map lookups that
return 0 for an absent key, so a regression that stopped populating field 2
entirely would leave both sides at 0 and still pass. The test would greenlight
the exact bug it's here to catch. I'd guard the baseline first:
```go
require.Greater(t, secondGroupStats.ColSizes[2], int64(0))
require.Greater(t, secondGroupStats.ValueCounts[2], int64(0))
assert.Equal(t, 2*secondGroupStats.ColSizes[2], stats.ColSizes[2])
assert.Equal(t, 2*secondGroupStats.ValueCounts[2], stats.ValueCounts[2])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
