zeroshade commented on code in PR #1575:
URL: https://github.com/apache/iceberg-go/pull/1575#discussion_r3732122922
##########
table/scanner.go:
##########
@@ -182,7 +182,7 @@ func newPartitionRecord(partitionData map[int]any,
partitionType *iceberg.Struct
// GetPartitionRecord converts a DataFile's partition map into a positional
// record ordered by the fields of the given partition struct type.
func GetPartitionRecord(dataFile iceberg.DataFile, partitionType
*iceberg.StructType) iceberg.StructLike {
- return newPartitionRecord(dataFile.Partition(), partitionType)
+ return newPartitionRecord(dataFilePartition(dataFile), partitionType)
Review Comment:
**Blocking — this re-opens, through an exported API, the exact mutation hole
the PR is titled to close.**
`GetPartitionRecord` (line 184) is exported and used outside this repo. It
calls `dataFilePartition(dataFile)`, which for a concrete `*dataFile` returns
`d.fieldIDToPartitionData` **uncopied** (`data_file_refs.go:48-51`).
`newPartitionRecord` (`scanner.go:173`) copies the *values* into a `[]any`, but
for a `[]byte` partition value that copies the slice header, not the bytes — so
`partitionRecord.Get(pos)` (`scanner.go:127`) hands the caller the DataFile's
live `[]byte`. `iceberg.StructLike` is fully public with both `Get` and `Set`
(`utils.go:47-56`).
Concrete failure:
```go
table.GetPartitionRecord(df, partType).Get(0).([]byte)[0] = 0xff
```
permanently corrupts `d.fieldIDToPartitionData`. `initPartitionData` is
`sync.Once`-guarded (`manifest.go:2076`, `manifest.go:2080-2092`), so the map
is never rebuilt and there is no recovery.
**The corruption reaches disk.** `ManifestWriter.Add` calls
`entry.Data.Partition()` (`manifest.go:1462`), which clones the
already-corrupted map and writes it as that entry's partition tuple.
`MarshalAvroEntry` does the same at `data_file_codec.go:110`. A wrong partition
tuple in a manifest means files that no longer prune correctly.
This is not a regression — pre-PR, `Partition()` returned the internal map
too. The problem is that the PR closes the `Partition()` hole and leaves the
equivalent one open one function away, on the public surface. The asymmetry
matters: `dataFileStats`' two in-package consumers are read-only visitors you
control, while `GetPartitionRecord`'s consumers are unbounded.
**Suggested fix:** either revert this line to `dataFile.Partition()` and
keep `dataFilePartition` for the internal-only sites at `scanner.go:568` and
`:581`, or have `newPartitionRecord` clone `[]byte` values as it fills the
slice. The second costs one clone per *partition field* (typically 1-3), not
per column, so it stays cheap while making the exported API honest.
##########
table/data_file_stats_ref.go:
##########
@@ -0,0 +1,60 @@
+// 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 (
+ iceberg "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/internal"
+)
+
+type dataFileStatsRefer interface {
+ DataFileStatsRef(internal.DataFileRef) (
+ valueCounts map[int]int64,
+ nullCounts map[int]int64,
+ nanCounts map[int]int64,
+ lowerBounds map[int][]byte,
+ upperBounds map[int][]byte,
+ )
+}
+
+type dataFilePartitionRefer interface {
+ DataFilePartitionRef(internal.DataFileRef) map[int]any
+}
+
+func dataFileStats(file iceberg.DataFile) (
Review Comment:
The read-only contract is documented thoroughly, but on the function nobody
calls. `data_file_refs.go:22-30` spells out the aliasing rules on
`(*dataFile).DataFileStatsRef` — which no in-package caller invokes directly.
`dataFileStats`, which is what callers actually use, has no doc comment at all.
That matters more than usual here because ownership of the return value
silently depends on the argument's *dynamic* type: borrowed when it's a
`*dataFile`, an independent copy for any other `DataFile` implementation. A
future caller in package `table` who writes into `ev.lowerBounds` would pass
every test that uses a wrapper type and corrupt cached state in production.
**Suggested fix:** move (or duplicate) the ownership contract onto
`dataFileStats` and `dataFilePartition`, and say explicitly that the caller
must not mutate or retain the returned maps regardless of which branch is taken.
##########
manifest.go:
##########
@@ -2476,7 +2521,7 @@ func NewDataFileBuilder(
RecordCount: recordCount,
FileSize: fileSize,
specID: int32(spec.id),
- fieldIDToPartitionData: fieldIDToPartitionData,
+ fieldIDToPartitionData:
clonePartitionMap(fieldIDToPartitionData),
fieldNameToID: fieldNameToID,
fieldIDToLogicalType: fieldIDToLogicalType,
Review Comment:
`fieldIDToPartitionData` is cloned two lines up, but `fieldIDToLogicalType`
is retained by reference from the caller — inconsistent with the goal stated in
the PR title.
This is harmless today, but only by accident: the builder path never reaches
`convertAvroValueToIcebergType`, because the `len(d.fieldIDToPartitionData) <
len(d.PartitionData)` guard at `manifest.go:2082` is always false for a
builder-constructed file. So the map the caller can still mutate is never read.
That's an accident of the current control flow, not a design.
**Suggested fix:** clone it alongside the partition map, or add a comment
explaining why it is deliberately shared.
##########
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)
+}
+
+func clonePartitionMap(src map[int]any) map[int]any {
Review Comment:
The `[]byte`-only deep copy is correct, but only because of a non-obvious
property of the upstream conversion, and nothing here records that.
I checked every type `convertAvroValueToIcebergType` can produce:
`Date`/`Time`/`Timestamp`/`TimestampNano` are integer types,
`DecimalLiteral.Val` holds a `decimal128.Num` by value, and `uuid.UUID` is a
`[16]byte` array — so `[]byte` really is the only reference-typed value that
can appear in this map.
**Suggested fix:** state that in the function's own comment. Without it, the
next person to add a partition value type has no signal that this function
needs updating, and the failure would be silent aliasing rather than a compile
error.
##########
table/data_file_stats_ref.go:
##########
@@ -0,0 +1,60 @@
+// 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 (
+ iceberg "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/internal"
+)
+
+type dataFileStatsRefer interface {
+ DataFileStatsRef(internal.DataFileRef) (
+ valueCounts map[int]int64,
+ nullCounts map[int]int64,
+ nanCounts map[int]int64,
+ lowerBounds map[int][]byte,
+ upperBounds map[int][]byte,
+ )
+}
+
+type dataFilePartitionRefer interface {
+ DataFilePartitionRef(internal.DataFileRef) map[int]any
+}
+
+func dataFileStats(file iceberg.DataFile) (
+ valueCounts map[int]int64,
+ nullCounts map[int]int64,
+ nanCounts map[int]int64,
+ lowerBounds map[int][]byte,
+ upperBounds map[int][]byte,
+) {
+ if ref, ok := file.(dataFileStatsRefer); ok {
+ return ref.DataFileStatsRef(internal.DataFileRef{})
+ }
+
+ return file.ValueCounts(), file.NullValueCounts(),
file.NaNValueCounts(),
+ file.LowerBoundValues(), file.UpperBoundValues()
+}
+
+func dataFilePartition(file iceberg.DataFile) map[int]any {
Review Comment:
Same missing contract as `dataFileStats` above, and it applies more sharply
here: this is the function feeding `GetPartitionRecord` at `scanner.go:185`, so
the "must not mutate or retain" rule is the only thing standing between an
exported API and permanent corruption of `d.fieldIDToPartitionData`.
**Suggested fix:** document the borrowed-vs-copied return contract here, and
note that the `*dataFile` branch aliases state protected by a `sync.Once`
(`manifest.go:2080`), so a violation is unrecoverable rather than merely stale.
##########
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)
+}
+
+func clonePartitionMap(src map[int]any) map[int]any {
+ if src == nil {
+ return nil
+ }
+
+ out := maps.Clone(src)
+ for id, value := range out {
+ if bytes, ok := value.([]byte); ok {
+ out[id] = slices.Clone(bytes)
+ }
+ }
+
+ return out
+}
+
+func cloneByteMap(src map[int][]byte) map[int][]byte {
Review Comment:
**Highest-value missing test.** Nothing currently locks nil-vs-present-empty
through the new clone helpers, and this repo cares about that distinction
specifically — see `manifest_test.go:2920` and the assertions at `:2966-2969`
requiring that `column_sizes: []` survive a decode → re-encode rewrite as a
present array rather than an Avro null (issue #1309).
Today it still works, but only incidentally: `maps.Clone` preserves non-nil
for an empty map, and `cloneByteMap`/`clonePartitionMap` nil-check *before* the
`make`.
Given the allocation pressure this PR introduces, `if len(src) == 0 { return
nil }` is exactly the optimization someone applies to `cloneByteMap` next — and
it would silently collapse present-empty
`lower_bounds`/`upper_bounds`/`column_sizes` to null on every manifest rewrite,
with no failing test anywhere.
**Suggested fix:** for each cloned getter, assert all three states — absent
→ nil, present-empty → non-nil empty, populated → equal — after a decode →
getter → builder → re-encode round trip. That pins the invariant at the layer
where the optimization would be applied.
##########
data_file_refs.go:
##########
@@ -0,0 +1,52 @@
+// 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 iceberg
+
+import "github.com/apache/iceberg-go/internal"
+
+// DataFileStatsRef returns statistics maps owned by the data file. The token
+// restricts this zero-copy accessor to trusted in-module callers; the public
+// DataFile getters continue returning defensive copies. The returned maps and
+// every byte slice reachable through the bounds maps alias the DataFile and
+// must not be mutated or retained beyond the current evaluation.
+//
+// This view intentionally contains only the maps consumed by metric
Review Comment:
"intentionally contains only the maps consumed by metric evaluators" reads
as though the set were exhaustive, but `ColumnSizes` and `DistinctValueCounts`
are also column-stat maps and they are left on the copying path with no stated
rationale.
**Suggested fix:** either say why those two are excluded (I assume: no
evaluator reads them, so no hot path justifies the extra surface), or fold them
in. As written, the next person to touch this can't tell whether the omission
is deliberate or an oversight.
##########
manifest.go:
##########
@@ -2534,35 +2579,38 @@ func (b *DataFileBuilder) DistinctValueCounts(counts
map[int]int64) *DataFileBui
// LowerBoundValues sets the lower bound values for the data file.
func (b *DataFileBuilder) LowerBoundValues(bounds map[int][]byte)
*DataFileBuilder {
- b.d.LowerBounds = mapToAvroColMap(bounds)
+ b.d.LowerBounds = mapToAvroColMap(cloneByteMap(bounds))
Review Comment:
`mapToAvroColMap(cloneByteMap(bounds))` builds a throwaway map that is
immediately flattened into a slice, so the intermediate map allocation and its
bucket growth are pure waste. Same on line 2589 for upper bounds.
**Suggested fix:** inline the `slices.Clone` into the flatten loop inside
`mapToAvroColMap` (or a `mapToAvroColMapCloned` variant) and drop one map
allocation per builder call in each direction. Small, but this is on the write
path for every data file.
##########
manifest_test.go:
##########
@@ -2969,6 +2969,127 @@ func (m *ManifestTestSuite)
TestManifestEntryPresentEmptyListSurvivesRewrite() {
"present-empty column_sizes must survive a decode -> re-encode
rewrite as a present array")
}
+func (m *ManifestTestSuite)
TestDataFileMetadataIsIsolatedFromExternalMutation() {
+ partition := []byte{0x01, 0x02}
+ partitionData := map[int]any{1000: partition}
+ spec := NewPartitionSpec(PartitionField{SourceIDs: []int{1}, FieldID:
1000, Name: "part", Transform: IdentityTransform{}})
+ builder, err := NewDataFileBuilder(spec, EntryContentData,
"s3://bucket/file.parquet", ParquetFile,
+ partitionData, nil, nil, 1, 10)
+ m.Require().NoError(err)
+
+ columnSizes := map[int]int64{1: 10}
+ valueCounts := map[int]int64{1: 2}
+ nullCounts := map[int]int64{1: 1}
+ nanCounts := map[int]int64{1: 0}
+ distinctCounts := map[int]int64{1: 2}
+ lower := map[int][]byte{1: {0x03, 0x04}}
+ upper := map[int][]byte{1: {0x05, 0x06}}
+ key := []byte{0x07, 0x08}
+ splits := []int64{10, 20}
+ equalityIDs := []int{1, 2}
+
+ dataFile := builder.
+ ColumnSizes(columnSizes).
+ ValueCounts(valueCounts).
+ NullValueCounts(nullCounts).
+ NaNValueCounts(nanCounts).
+ DistinctValueCounts(distinctCounts).
+ LowerBoundValues(lower).
+ UpperBoundValues(upper).
+ KeyMetadata(key).
+ SplitOffsets(splits).
+ EqualityFieldIDs(equalityIDs).
+ SortOrderID(3).
+ FirstRowID(4).
+ ReferencedDataFile("data.parquet").
+ ContentOffset(5).
+ ContentSizeInBytes(6).
+ Build()
+
+ partition[0], lower[1][0], upper[1][0], key[0], splits[0],
equalityIDs[0] = 0xff, 0xff, 0xff, 0xff, 99, 99
+ partitionData[1000] = []byte{0xff}
+ columnSizes[1], valueCounts[1], nullCounts[1], nanCounts[1],
distinctCounts[1] = 99, 99, 99, 99, 99
+
+ m.Equal([]byte{0x01, 0x02}, builder.d.PartitionData["part"])
+ dataFile.Partition()[1000].([]byte)[0] = 0xff
+ dataFile.ColumnSizes()[1] = 99
+ dataFile.ValueCounts()[1] = 99
+ dataFile.NullValueCounts()[1] = 99
+ dataFile.NaNValueCounts()[1] = 99
+ dataFile.DistinctValueCounts()[1] = 99
+ dataFile.LowerBoundValues()[1][0] = 0xff
+ dataFile.UpperBoundValues()[1][0] = 0xff
+ dataFile.KeyMetadata()[0] = 0xff
+ dataFile.SplitOffsets()[0] = 99
+ dataFile.EqualityFieldIDs()[0] = 99
+ *dataFile.SortOrderID() = 99
+ *dataFile.FirstRowID() = 99
+ *dataFile.ReferencedDataFile() = "changed"
+ *dataFile.ContentOffset() = 99
+ *dataFile.ContentSizeInBytes() = 99
+
+ m.Equal([]byte{0x01, 0x02}, dataFile.Partition()[1000])
+ m.Equal(map[int]int64{1: 10}, dataFile.ColumnSizes())
+ m.Equal(map[int]int64{1: 2}, dataFile.ValueCounts())
+ m.Equal(map[int]int64{1: 1}, dataFile.NullValueCounts())
+ m.Equal(map[int]int64{1: 0}, dataFile.NaNValueCounts())
+ m.Equal(map[int]int64{1: 2}, dataFile.DistinctValueCounts())
+ m.Equal([]byte{0x03, 0x04}, dataFile.LowerBoundValues()[1])
+ m.Equal([]byte{0x05, 0x06}, dataFile.UpperBoundValues()[1])
+ m.Equal([]byte{0x07, 0x08}, dataFile.KeyMetadata())
+ m.Equal([]int64{10, 20}, dataFile.SplitOffsets())
+ m.Equal([]int{1, 2}, dataFile.EqualityFieldIDs())
+ m.Equal(3, *dataFile.SortOrderID())
+ m.Equal(int64(4), *dataFile.FirstRowID())
+ m.Equal("data.parquet", *dataFile.ReferencedDataFile())
+ m.Equal(int64(5), *dataFile.ContentOffset())
+ m.Equal(int64(6), *dataFile.ContentSizeInBytes())
+}
+
+func (m *ManifestTestSuite)
TestDecodedDataFileMetadataIsIsolatedFromSourceAndGetterMutation() {
Review Comment:
Good test — mutating the encoded buffer in place after decode is a genuinely
convincing way to prove the decoder doesn't alias the source. Two gaps worth
closing.
Decode-path isolation here covers only `Partition`, `LowerBoundValues`, and
`UpperBoundValues`. `ColumnSizes`, `ValueCounts`, `NullValueCounts`,
`NaNValueCounts`, `DistinctValueCounts`, `KeyMetadata`, `SplitOffsets`,
`EqualityFieldIDs`, and all five `clonePointer` getters are exercised on the
builder path only — so a decode-path aliasing bug in any of them would go
unnoticed.
**Suggested fix:** extend the post-mutation assertions to the remaining
getters. The scaffolding is already here; it's mostly additional assertion
lines.
##########
table/data_file_stats_ref_test.go:
##########
@@ -0,0 +1,268 @@
+// 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
+}
+
+type publicPartitionDataFile struct {
+ iceberg.DataFile
+}
+
+func (f *publicPartitionDataFile) Partition() map[int]any {
+ return f.DataFile.Partition()
+}
+
+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)
+
+ var measuredValueCounts map[int]int64
+ allocs := testing.AllocsPerRun(100, func() {
+ measuredValueCounts, _, _, _, _ = dataFileStats(file)
+ })
+ assert.InDelta(t, 0.0, allocs, 0.5)
+ assert.Equal(t, map[int]int64{1: 2}, measuredValueCounts)
+}
+
+func TestDataFileStatsFallsBackToPublicGetters(t *testing.T) {
+ file := &publicStatsDataFile{DataFile: testDataFileWithStats(t)}
+
+ 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.Equal(t, 5, file.getterCalls)
+}
+
+func TestDataFilePartitionUsesBorrowedView(t *testing.T) {
+ file := testDataFileWithStats(t)
+ require.Implements(t, (*dataFilePartitionRefer)(nil), file)
+
+ partition := dataFilePartition(file)
+ assert.Equal(t, map[int]any{1000: "partition"}, partition)
+
+ allocs := testing.AllocsPerRun(100, func() {
+ partition = dataFilePartition(file)
+ })
+ assert.InDelta(t, 0.0, allocs, 0.5)
+ assert.Equal(t, "partition", partition[1000])
+}
+
+func TestDataFilePartitionFallsBackToPublicGetter(t *testing.T) {
+ file := &publicPartitionDataFile{DataFile: testDataFileWithStats(t)}
+
+ assert.Equal(t, map[int]any{1000: "partition"}, dataFilePartition(file))
+}
+
+func BenchmarkInclusiveMetricsEvalDataFileStats(b *testing.B) {
Review Comment:
This benchmark is the justification for the whole token mechanism, and it
understates its own case in one way while being unfair in another.
**It uses a single-column file** (lines 167-170). Copy cost is dominated by
the `2N` byte-slice clones, so one column understates the saving by roughly two
orders of magnitude for a realistic table. Sweeping 1 / 50 / 500 columns would
*strengthen* the argument for the fast path — while simultaneously making the
cost at the ~20 unmitigated call sites impossible to overlook.
**Only the control arm is instrumented.** The "public defensive copies" arm
wraps the file in `publicStatsDataFile` (line 189), whose getters each do
`f.getterCalls++` (lines 43, 49, 55, 61, 67) inside the benchmarked loop, plus
an extra interface hop. The "borrowed stats" arm has neither. So some of the
measured delta is instrumentation, not copying.
**Suggested fix:** parameterize over column count, and benchmark against a
plain non-instrumented wrapper — keep `publicStatsDataFile` for the correctness
tests where the call counting is the point.
##########
manifest_test.go:
##########
@@ -2969,6 +2969,127 @@ func (m *ManifestTestSuite)
TestManifestEntryPresentEmptyListSurvivesRewrite() {
"present-empty column_sizes must survive a decode -> re-encode
rewrite as a present array")
}
+func (m *ManifestTestSuite)
TestDataFileMetadataIsIsolatedFromExternalMutation() {
+ partition := []byte{0x01, 0x02}
+ partitionData := map[int]any{1000: partition}
+ spec := NewPartitionSpec(PartitionField{SourceIDs: []int{1}, FieldID:
1000, Name: "part", Transform: IdentityTransform{}})
+ builder, err := NewDataFileBuilder(spec, EntryContentData,
"s3://bucket/file.parquet", ParquetFile,
+ partitionData, nil, nil, 1, 10)
+ m.Require().NoError(err)
+
+ columnSizes := map[int]int64{1: 10}
+ valueCounts := map[int]int64{1: 2}
+ nullCounts := map[int]int64{1: 1}
+ nanCounts := map[int]int64{1: 0}
+ distinctCounts := map[int]int64{1: 2}
+ lower := map[int][]byte{1: {0x03, 0x04}}
+ upper := map[int][]byte{1: {0x05, 0x06}}
+ key := []byte{0x07, 0x08}
+ splits := []int64{10, 20}
+ equalityIDs := []int{1, 2}
+
+ dataFile := builder.
+ ColumnSizes(columnSizes).
+ ValueCounts(valueCounts).
+ NullValueCounts(nullCounts).
+ NaNValueCounts(nanCounts).
+ DistinctValueCounts(distinctCounts).
+ LowerBoundValues(lower).
+ UpperBoundValues(upper).
+ KeyMetadata(key).
+ SplitOffsets(splits).
+ EqualityFieldIDs(equalityIDs).
+ SortOrderID(3).
+ FirstRowID(4).
+ ReferencedDataFile("data.parquet").
+ ContentOffset(5).
+ ContentSizeInBytes(6).
+ Build()
+
+ partition[0], lower[1][0], upper[1][0], key[0], splits[0],
equalityIDs[0] = 0xff, 0xff, 0xff, 0xff, 99, 99
+ partitionData[1000] = []byte{0xff}
+ columnSizes[1], valueCounts[1], nullCounts[1], nanCounts[1],
distinctCounts[1] = 99, 99, 99, 99, 99
+
+ m.Equal([]byte{0x01, 0x02}, builder.d.PartitionData["part"])
Review Comment:
This asserts on `builder.d.PartitionData["part"]`, reaching into an
unexported field of the type under test. That couples the test to the internal
representation and, more importantly, doesn't exercise the path a caller
actually takes.
**Suggested fix:** assert through `Build()` followed by `Partition()`. That
tests the same isolation property via the public surface and would keep passing
across an internal refactor — while also failing if the *getter* stops copying,
which is what the PR is really about.
--
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]
