diff --git a/data_file_codec.go b/data_file_codec.go index 232da05e6..c7c90d47d 100644 --- a/data_file_codec.go +++ b/data_file_codec.go @@ -175,16 +175,29 @@ func newDecodeEntry(version int) (any, *dataFile) { return &manifestEntry{Data: df}, df } +var dataFileAvroFieldIndexes = avroFieldIndexes(reflect.TypeOf(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. // -// Using reflection over the tag set means a new avro-tagged field -// upstream is auto-copied without an update here — the dataFile struct -// remains the single source of truth for the wire shape. It also -// sidesteps the go-vet copies-lock warning that would fire on a -// struct-literal copy of *dataFile (it embeds sync.Once). +// 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. @@ -196,11 +209,8 @@ func cloneDataFileAvroFields(src *dataFile) *dataFile { out := &dataFile{} srcVal := reflect.ValueOf(src).Elem() outVal := reflect.ValueOf(out).Elem() - t := srcVal.Type() - for i := range t.NumField() { - if _, hasAvroTag := t.Field(i).Tag.Lookup("avro"); hasAvroTag { - outVal.Field(i).Set(srcVal.Field(i)) - } + for _, i := range dataFileAvroFieldIndexes { + outVal.Field(i).Set(srcVal.Field(i)) } return out diff --git a/data_file_codec_test.go b/data_file_codec_test.go index 13aa2b6ef..1c7606ca0 100644 --- a/data_file_codec_test.go +++ b/data_file_codec_test.go @@ -143,6 +143,19 @@ func TestMarshalAvroEntryDoesNotMutateAnyAvroField(t *testing.T) { "including pointer-typed fields whose backing storage is shared with the clone") } +func TestDataFileAvroFieldIndexesCoverEveryAvroField(t *testing.T) { + typ := reflect.TypeOf(dataFile{}) + want := make([]int, 0, typ.NumField()) + for i := range typ.NumField() { + if _, ok := typ.Field(i).Tag.Lookup("avro"); ok { + want = append(want, i) + } + } + + require.Equal(t, want, dataFileAvroFieldIndexes, + "the precomputed clone indexes must track every avro-tagged dataFile field") +} + func TestMarshalAvroEntryDecimalPartitionRoundTrip(t *testing.T) { schema := NewSchema(0, NestedField{ID: 1, Name: "price", Type: DecimalTypeOf(10, 2)}, @@ -246,7 +259,7 @@ func deepCopyReflect(src reflect.Value) reflect.Value { } } -func fullyPopulatedDataFileForCodec(t *testing.T, version int) (PartitionSpec, *Schema, DataFile) { +func fullyPopulatedDataFileForCodec(t testing.TB, version int) (PartitionSpec, *Schema, DataFile) { t.Helper() schema := NewSchema(123, NestedField{ID: 1, Name: "id", Type: Int64Type{}, Required: true}, @@ -296,3 +309,37 @@ func fullyPopulatedDataFileForCodec(t *testing.T, version int) (PartitionSpec, * return spec, schema, builder.Build() } + +var ( + benchmarkDataFileCloneSink *dataFile + benchmarkEncodedEntrySize int +) + +func BenchmarkCloneDataFileAvroFields(b *testing.B) { + _, _, df := fullyPopulatedDataFileForCodec(b, 2) + impl := df.(*dataFile) + + b.ReportAllocs() + b.ResetTimer() + for range b.N { + benchmarkDataFileCloneSink = cloneDataFileAvroFields(impl) + } +} + +func BenchmarkMarshalAvroEntry(b *testing.B) { + for _, version := range []int{1, 2, 3} { + b.Run("v"+strconv.Itoa(version), func(b *testing.B) { + spec, schema, df := fullyPopulatedDataFileForCodec(b, version) + impl := df.(*dataFile) + b.ReportAllocs() + b.ResetTimer() + for range b.N { + encoded, err := impl.MarshalAvroEntry(spec, schema, version) + if err != nil { + b.Fatal(err) + } + benchmarkEncodedEntrySize = len(encoded) + } + }) + } +} diff --git a/schema.go b/schema.go index 24b0ff4b1..4330103dc 100644 --- a/schema.go +++ b/schema.go @@ -346,8 +346,7 @@ func (s *Schema) MarshalJSON() ([]byte, error) { type Alias Schema - aliasCopy := *(*Alias)(s) - aliasCopy.IdentifierFieldIDs = ids + aliasCopy := Alias{ID: s.ID, IdentifierFieldIDs: ids} return json.Marshal(struct { Type string `json:"type"` diff --git a/schema_test.go b/schema_test.go index 7fa16e345..6d794ab71 100644 --- a/schema_test.go +++ b/schema_test.go @@ -24,6 +24,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "github.com/apache/iceberg-go" @@ -2293,3 +2294,48 @@ func TestVisitGeoSchemaWithSchemaVisitorPerPrimitiveType(t *testing.T) { assert.Equal(t, 1, v.geometryCalls) assert.Equal(t, 1, v.geographyCalls) } + +func TestSchemaMarshalJSONConcurrentLazyLookups(t *testing.T) { + for range 32 { + schema := iceberg.NewSchemaWithIdentifiers(17, nil, + iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true}, + iceberg.NestedField{ID: 2, Name: "data", Type: iceberg.PrimitiveTypes.String}, + ) + start := make(chan struct{}) + var wg sync.WaitGroup + for range 8 { + wg.Go(func() { + <-start + for range 8 { + _, err := json.Marshal(schema) + assert.NoError(t, err) + } + }) + wg.Go(func() { + <-start + _, found := schema.FindFieldByID(1) + assert.True(t, found) + _, found = schema.FindFieldByName("data") + assert.True(t, found) + _, found = schema.FindFieldByNameCaseInsensitive("DATA") + assert.True(t, found) + name, found := schema.FindColumnName(2) + assert.True(t, found) + assert.Equal(t, "data", name) + }) + } + close(start) + wg.Wait() + + data, err := json.Marshal(schema) + require.NoError(t, err) + assert.JSONEq(t, `{ + "type": "struct", "schema-id": 17, "identifier-field-ids": [], + "fields": [ + {"id": 1, "name": "id", "type": "long", "required": true}, + {"id": 2, "name": "data", "type": "string", "required": false} + ] + }`, string(data)) + assert.Nil(t, schema.IdentifierFieldIDs) + } +}