You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/10/19 18:08:25 UTC
(iceberg-go) branch main updated: perf(manifest): avoid reflection when cloning DataFile Avro fields (#2202)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 0bc8d2f72 perf(manifest): avoid reflection when cloning DataFile Avro fields (#2202)
0bc8d2f72 is described below
commit 0bc8d2f727e73948779863d406ec9b8841b038cc
Author: Minh Vu <vu...@gmail.com>
AuthorDate: Mon Oct 19 20:08:19 2026 +0200
perf(manifest): avoid reflection when cloning DataFile Avro fields (#2202)
* perf(manifest): avoid reflection when cloning DataFile Avro fields
Signed-off-by: Minh Vu <vu...@gmail.com>
* test(manifest): cover shallow Avro clones and all format versions
Signed-off-by: Minh Vu <vu...@gmail.com>
* test(manifest): benchmark batch Avro entry serialization
* style(manifest): use reflect type field iterator
---------
Signed-off-by: Minh Vu <vu...@gmail.com>
---
data_file_codec.go | 73 +++++++++++++++++--------------------
data_file_codec_bench_test.go | 34 ++++++++++++++++++
data_file_codec_test.go | 84 +++++++++++++++++++++++++++++++++++--------
manifest_projection_test.go | 9 ++---
4 files changed, 140 insertions(+), 60 deletions(-)
diff --git a/data_file_codec.go b/data_file_codec.go
index ea0b96ba6..b11fec574 100644
--- a/data_file_codec.go
+++ b/data_file_codec.go
@@ -19,7 +19,6 @@ package iceberg
import (
"fmt"
- "reflect"
"strconv"
"strings"
@@ -72,10 +71,9 @@ type AvroEntryMarshaler interface {
// MarshalAvroEntry encodes this DataFile as Avro bytes using the
// manifest-entry encoding for the given partition spec, table schema
// and format version (1, 2, or 3). The wire format is the same one a
-// manifest carries for this data file, so adding a field to the
-// underlying struct (and its avro tags) automatically extends what
-// MarshalAvroEntry transports — there is no separate wire-mirror
-// struct to keep in sync.
+// manifest carries for this data file. The Avro tags define the wire fields;
+// TestCloneDataFileAvroFieldsCopiesOnlyAvroFields checks that the explicit
+// clone list stays synchronized when new fields are added.
//
// MarshalAvroEntry is the iceberg-package side of the
// [github.com/apache/iceberg-go/codec] DataFile codec; callers
@@ -183,45 +181,38 @@ func newDecodeEntry(version int) (any, *dataFile) {
return &manifestEntry{Data: df}, df
}
-var dataFileAvroFieldIndexes = avroFieldIndexes(reflect.TypeFor[dataFile]())
-
-func avroFieldIndexes(t reflect.Type) []int {
- indexes := make([]int, 0, t.NumField())
- for i := range t.NumField() {
- if _, hasAvroTag := t.Field(i).Tag.Lookup("avro"); hasAvroTag {
- indexes = append(indexes, i)
- }
- }
-
- return indexes
-}
-
-// cloneDataFileAvroFields returns a fresh *dataFile populated with src's
-// avro-tagged fields. Internal state (sync.Once, lazy-init caches,
-// specID, the field-id lookup maps) is intentionally left at zero
-// values because the avro encoder reads only the avro-tagged fields.
+// cloneDataFileAvroFields shallow-copies the Avro-tagged fields without
+// reflection. Internal state, including sync.Once, caches, and spec IDs, is
+// intentionally left at zero values. The test checks every Avro-tagged field
+// so that new fields cannot be silently omitted from this list.
//
-// The avro-tagged field indexes are discovered once when the package is
-// initialized. This keeps dataFile as the single source of truth for the
-// wire shape while avoiding a reflect.Type and StructTag lookup for every
-// encoded manifest entry. It also sidesteps the go-vet copies-lock warning
-// that would fire on a struct-literal copy of *dataFile (it embeds sync.Once).
-//
-// Note: this is a shallow copy. Pointer-typed avro fields (ColSizes,
-// LowerBounds, etc.) share their backing storage with the source.
-// The no-mutation guarantee of MarshalAvroEntry depends on the avro
-// encoder being read-only on the values it walks; TestMarshalAvroEntry
-// DoesNotMutate asserts this end-to-end across every avro-tagged
-// field, so a future regression in the encoder surfaces in tests.
+// Pointer-typed fields share backing storage with src. The encoder must not
+// mutate these values (see TestMarshalAvroEntryDoesNotMutateAnyAvroField).
func cloneDataFileAvroFields(src *dataFile) *dataFile {
- out := &dataFile{}
- srcVal := reflect.ValueOf(src).Elem()
- outVal := reflect.ValueOf(out).Elem()
- for _, i := range dataFileAvroFieldIndexes {
- outVal.Field(i).Set(srcVal.Field(i))
+ return &dataFile{
+ Content: src.Content,
+ Path: src.Path,
+ Format: src.Format,
+ PartitionData: src.PartitionData,
+ RecordCount: src.RecordCount,
+ FileSize: src.FileSize,
+ BlockSizeInBytes: src.BlockSizeInBytes,
+ ColSizes: src.ColSizes,
+ ValCounts: src.ValCounts,
+ NullCounts: src.NullCounts,
+ NaNCounts: src.NaNCounts,
+ DistinctCounts: src.DistinctCounts,
+ LowerBounds: src.LowerBounds,
+ UpperBounds: src.UpperBounds,
+ Key: src.Key,
+ Splits: src.Splits,
+ EqualityIDs: src.EqualityIDs,
+ SortOrder: src.SortOrder,
+ FirstRowIDField: src.FirstRowIDField,
+ ReferencedDataFileField: src.ReferencedDataFileField,
+ ContentOffsetField: src.ContentOffsetField,
+ ContentSizeInBytesField: src.ContentSizeInBytesField,
}
-
- return out
}
// avroEncodePartitionData converts an id-keyed partition tuple (carrying
diff --git a/data_file_codec_bench_test.go b/data_file_codec_bench_test.go
index c812c48e0..f7080065a 100644
--- a/data_file_codec_bench_test.go
+++ b/data_file_codec_bench_test.go
@@ -99,3 +99,37 @@ func BenchmarkDataFileSchemaCache(b *testing.B) {
}
}
}
+
+var marshalAvroEntryBatchBenchmarkSink []byte
+
+func BenchmarkMarshalAvroEntryBatch(b *testing.B) {
+ const batchSize = 128
+ for _, version := range []int{1, 2, 3} {
+ b.Run("v"+strconv.Itoa(version)+"/files=128", func(b *testing.B) {
+ spec, schema, first := fullyPopulatedDataFileForCodec(b, version)
+ files := make([]*dataFile, batchSize)
+ files[0] = first.(*dataFile)
+ for i := 1; i < len(files); i++ {
+ _, _, df := fullyPopulatedDataFileForCodec(b, version)
+ files[i] = df.(*dataFile)
+ }
+ if _, err := files[0].MarshalAvroEntry(spec, schema, version); err != nil {
+ b.Fatal(err)
+ }
+
+ b.ReportAllocs()
+ b.ResetTimer()
+ for b.Loop() {
+ for _, df := range files {
+ encoded, err := df.MarshalAvroEntry(spec, schema, version)
+ if err != nil {
+ b.Fatal(err)
+ }
+ marshalAvroEntryBatchBenchmarkSink = encoded
+ }
+ }
+ b.StopTimer()
+ b.ReportMetric(float64(batchSize), "files/op")
+ })
+ }
+}
diff --git a/data_file_codec_test.go b/data_file_codec_test.go
index 0e35bcce7..74501ec10 100644
--- a/data_file_codec_test.go
+++ b/data_file_codec_test.go
@@ -217,30 +217,84 @@ func TestManifestEntrySchemaForDistinguishesSameSpecIDDifferentPartitionTypes(t
}
func TestMarshalAvroEntryDoesNotMutateAnyAvroField(t *testing.T) {
- spec, schema, df := fullyPopulatedDataFileForCodec(t, 2)
- impl := df.(*dataFile)
+ for _, version := range []int{1, 2, 3} {
+ t.Run("v"+strconv.Itoa(version), func(t *testing.T) {
+ spec, schema, df := fullyPopulatedDataFileForCodec(t, version)
+ impl := df.(*dataFile)
- before := snapshotAvroFields(impl)
- _, err := impl.MarshalAvroEntry(spec, schema, 2)
- require.NoError(t, err)
- after := snapshotAvroFields(impl)
+ before := snapshotAvroFields(impl)
+ _, err := impl.MarshalAvroEntry(spec, schema, version)
+ require.NoError(t, err)
+ after := snapshotAvroFields(impl)
- require.Equal(t, before, after,
- "MarshalAvroEntry must not mutate any avro-tagged field of the source DataFile, "+
- "including pointer-typed fields whose backing storage is shared with the clone")
+ require.Equal(t, before, after,
+ "MarshalAvroEntry must not mutate any avro-tagged field of the source DataFile, "+
+ "including pointer-typed fields whose backing storage is shared with the clone")
+ })
+ }
}
-func TestDataFileAvroFieldIndexesCoverEveryAvroField(t *testing.T) {
+func TestCloneDataFileAvroFieldsCopiesOnlyAvroFields(t *testing.T) {
+ src := &dataFile{}
typ := reflect.TypeFor[dataFile]()
- want := make([]int, 0, typ.NumField())
+ srcVal := reflect.ValueOf(src).Elem()
+
+ // Give every tagged field a nonzero value, including fields added later.
for i := range typ.NumField() {
- if _, ok := typ.Field(i).Tag.Lookup("avro"); ok {
- want = append(want, i)
+ field := typ.Field(i)
+ if _, tagged := field.Tag.Lookup("avro"); !tagged {
+ continue
+ }
+
+ value := srcVal.Field(i)
+ switch value.Kind() {
+ case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+ value.SetInt(17)
+ case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+ value.SetUint(17)
+ case reflect.String:
+ value.SetString("sentinel")
+ case reflect.Map:
+ value.Set(reflect.MakeMap(value.Type()))
+ case reflect.Pointer:
+ value.Set(reflect.New(value.Type().Elem()))
+ default:
+ t.Fatalf("unhandled Avro field %s of kind %s", field.Name, value.Kind())
}
}
- require.Equal(t, want, dataFileAvroFieldIndexes,
- "the precomputed clone indexes must track every avro-tagged dataFile field")
+ // The clone must not carry state from lazily initialized source data.
+ src.projected = true
+ src.specID = 42
+ src.fieldNameToID = map[string]int{"field": 1}
+ src.initPartition.Do(func() {})
+ src.initColumnStats.Do(func() {})
+
+ cloneVal := reflect.ValueOf(cloneDataFileAvroFields(src)).Elem()
+ for i := range typ.NumField() {
+ field := typ.Field(i)
+ if _, tagged := field.Tag.Lookup("avro"); tagged {
+ source := srcVal.Field(i)
+ cloned := cloneVal.Field(i)
+ require.Equal(t, source.Interface(), cloned.Interface(),
+ "Avro field %s must be copied", field.Name)
+ switch source.Kind() {
+ case reflect.Pointer:
+ require.Same(t, source.Interface(), cloned.Interface(),
+ "pointer-typed Avro field %s must be shallow-copied", field.Name)
+ case reflect.Map:
+ // Mutating the source map must be visible through the clone.
+ // This detects accidental deep copies in this hot path.
+ key := reflect.Zero(source.Type().Key())
+ source.SetMapIndex(key, reflect.Zero(source.Type().Elem()))
+ require.True(t, cloned.MapIndex(key).IsValid(),
+ "map-typed Avro field %s must be shallow-copied", field.Name)
+ }
+ } else {
+ require.True(t, cloneVal.Field(i).IsZero(),
+ "internal field %s must be reset", field.Name)
+ }
+ }
}
func TestMarshalAvroEntryDecimalPartitionRoundTrip(t *testing.T) {
diff --git a/manifest_projection_test.go b/manifest_projection_test.go
index fdfe138af..42e8f9a2d 100644
--- a/manifest_projection_test.go
+++ b/manifest_projection_test.go
@@ -33,10 +33,11 @@ func TestManifestEntryProjectionWhitelistCoversDataFileSchema(t *testing.T) {
ID: 1, Name: "id", Type: PrimitiveTypes.Int64, Required: true,
})
dataFileType := reflect.TypeFor[dataFile]()
- dataFileFields := make(map[string]struct{}, len(dataFileAvroFieldIndexes))
- for _, index := range dataFileAvroFieldIndexes {
- name := dataFileType.Field(index).Tag.Get("avro")
- dataFileFields[name] = struct{}{}
+ dataFileFields := make(map[string]struct{})
+ for field := range dataFileType.Fields() {
+ if name, ok := field.Tag.Lookup("avro"); ok {
+ dataFileFields[name] = struct{}{}
+ }
}
optionalFields := map[string]struct{}{
"column_sizes": {},