laskoviymishka commented on code in PR #1575:
URL: https://github.com/apache/iceberg-go/pull/1575#discussion_r3703956722
##########
manifest.go:
##########
@@ -2201,7 +2202,45 @@ func (d *dataFile) FileFormat() FileFormat {
return d.Format }
func (d *dataFile) Partition() map[int]any {
d.initPartitionData()
- return d.fieldIDToPartitionData
+ return clonePartitionMap(d.fieldIDToPartitionData)
Review Comment:
The benchmark answered my round-1 doubt about whether the stats fast path is
worth it, thanks for adding it. The half that's still open is the consistency
call: `Partition()` is on the same per-file scan path and now clones on every
call, and `ColumnSizes`/`DistinctValueCounts` sit outside the borrowed view too.
I'm not asking to widen the escape hatch, just to make the scope deliberate.
Either extend the borrowed view (or add a `Partition` benchmark showing the
copy is cheap), or drop a line on why the partition map and those two stat maps
are intentionally left on the copying path. wdyt?
##########
manifest_test.go:
##########
@@ -2969,6 +2969,82 @@ func (m *ManifestTestSuite)
TestManifestEntryPresentEmptyListSurvivesRewrite() {
"present-empty column_sizes must survive a decode -> re-encode
rewrite as a present array")
}
+func (m *ManifestTestSuite)
TestDataFileMetadataIsIsolatedFromExternalMutation() {
Review Comment:
This test only covers the builder path. The round-1 ask was to exercise the
Avro decode path too, and that's still the gap I care about most.
Here's why it's more than coverage for its own sake. `initPartitionData`
rebuilds `fieldIDToPartitionData` from `PartitionData`, and
`convertAvroValueToIcebergType` returns `[]byte` as-is, so after a decode the
internal `fieldIDToPartitionData` shares backing arrays with the decoder's
buffers. `Partition()` clones on the way out, so nothing leaks to callers
today, but the internal state is aliased and nothing proves it stays benign.
Can we round-trip a manifest here (encode, decode, mutate the source bytes)
and assert the decoded file's `Partition()` and bounds are unchanged? That
closes the one path this test doesn't touch.
##########
table/data_file_stats_ref_test.go:
##########
@@ -0,0 +1,167 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "testing"
+
+ iceberg "github.com/apache/iceberg-go"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+type publicStatsDataFile struct {
+ iceberg.DataFile
+ getterCalls int
+}
+
+func (f *publicStatsDataFile) ValueCounts() map[int]int64 {
+ f.getterCalls++
+
+ return f.DataFile.ValueCounts()
+}
+
+func (f *publicStatsDataFile) NullValueCounts() map[int]int64 {
+ f.getterCalls++
+
+ return f.DataFile.NullValueCounts()
+}
+
+func (f *publicStatsDataFile) NaNValueCounts() map[int]int64 {
+ f.getterCalls++
+
+ return f.DataFile.NaNValueCounts()
+}
+
+func (f *publicStatsDataFile) LowerBoundValues() map[int][]byte {
+ f.getterCalls++
+
+ return f.DataFile.LowerBoundValues()
+}
+
+func (f *publicStatsDataFile) UpperBoundValues() map[int][]byte {
+ f.getterCalls++
+
+ return f.DataFile.UpperBoundValues()
+}
+
+func testDataFileWithStats(t *testing.T) iceberg.DataFile {
+ t.Helper()
+
+ spec := iceberg.NewPartitionSpec(iceberg.PartitionField{
+ SourceIDs: []int{1},
+ FieldID: 1000,
+ Name: "part",
+ Transform: iceberg.IdentityTransform{},
+ })
+ builder, err := iceberg.NewDataFileBuilder(
+ spec,
+ iceberg.EntryContentData,
+ "s3://bucket/file.parquet",
+ iceberg.ParquetFile,
+ map[int]any{1000: "partition"},
+ nil,
+ nil,
+ 2,
+ 10,
+ )
+ require.NoError(t, err)
+
+ return builder.
+ ValueCounts(map[int]int64{1: 2}).
+ NullValueCounts(map[int]int64{1: 0}).
+ NaNValueCounts(map[int]int64{1: 0}).
+ LowerBoundValues(map[int][]byte{1: {1, 2}}).
+ UpperBoundValues(map[int][]byte{1: {3, 4}}).
+ Build()
+}
+
+func TestDataFileStatsUsesBorrowedView(t *testing.T) {
+ file := testDataFileWithStats(t)
+ require.Implements(t, (*dataFileStatsRefer)(nil), file)
+
+ valueCounts, nullCounts, nanCounts, lowerBounds, upperBounds :=
dataFileStats(file)
+ assert.Equal(t, map[int]int64{1: 2}, valueCounts)
+ assert.Equal(t, map[int]int64{1: 0}, nullCounts)
+ assert.Equal(t, map[int]int64{1: 0}, nanCounts)
+ assert.Equal(t, map[int][]byte{1: {1, 2}}, lowerBounds)
+ assert.Equal(t, map[int][]byte{1: {3, 4}}, upperBounds)
+
+ assert.Zero(t, testing.AllocsPerRun(100, func() {
Review Comment:
This still passes if `dataFileStats` regresses to returning five nil maps.
The closure discards every return, so it only proves "zero allocs", not "zero
allocs and correct". Round-1 asked to tighten this.
Capture a value and assert it, and `InDelta` avoids a flake if a GC cycle
sneaks a fractional alloc in:
```go
var vc map[int]int64
allocs := testing.AllocsPerRun(100, func() {
vc, _, _, _, _ = dataFileStats(file)
})
assert.InDelta(t, 0.0, allocs, 0.5)
assert.Equal(t, map[int]int64{1: 2}, vc)
```
--
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]
