-
Notifications
You must be signed in to change notification settings - Fork 229
perf(metadata): avoid re-cloning defensive schema copies #1990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a34ca7f
9b28d8f
08f3a55
862fee7
5e04984
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2404,7 +2404,7 @@ func cloneSchema(schema *iceberg.Schema) *iceberg.Schema { | |
| return iceberg.NewSchemaWithIdentifiers( | ||
| schema.ID, | ||
| slices.Clone(schema.IdentifierFieldIDs), | ||
| cloneNestedFields(schema.Fields())..., | ||
| schema.Fields()..., | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -2421,47 +2421,8 @@ func cloneSchemas(schemas []*iceberg.Schema) []*iceberg.Schema { | |
| return clones | ||
| } | ||
|
|
||
| func cloneNestedFields(fields []iceberg.NestedField) []iceberg.NestedField { | ||
| clones := slices.Clone(fields) | ||
| for i := range clones { | ||
| clones[i].Type = cloneSchemaType(clones[i].Type) | ||
| clones[i].InitialDefault = iceberg.CloneDefaultValue(clones[i].InitialDefault) | ||
| clones[i].WriteDefault = iceberg.CloneDefaultValue(clones[i].WriteDefault) | ||
| } | ||
|
|
||
| return clones | ||
| } | ||
|
|
||
| func cloneSchemaType(typ iceberg.Type) iceberg.Type { | ||
| switch typ := typ.(type) { | ||
| case *iceberg.StructType: | ||
| return &iceberg.StructType{FieldList: cloneNestedFields(typ.FieldList)} | ||
| case *iceberg.ListType: | ||
| return &iceberg.ListType{ | ||
| ElementID: typ.ElementID, | ||
| Element: cloneSchemaType(typ.Element), | ||
| ElementRequired: typ.ElementRequired, | ||
| } | ||
| case *iceberg.MapType: | ||
| return &iceberg.MapType{ | ||
| KeyID: typ.KeyID, | ||
| KeyType: cloneSchemaType(typ.KeyType), | ||
| ValueID: typ.ValueID, | ||
| ValueType: cloneSchemaType(typ.ValueType), | ||
| ValueRequired: typ.ValueRequired, | ||
| } | ||
| default: | ||
| return typ | ||
| } | ||
| } | ||
|
|
||
| func clonePartitionSpec(spec iceberg.PartitionSpec) iceberg.PartitionSpec { | ||
| fields := make([]iceberg.PartitionField, spec.NumFields()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit — clonePartitionSpec is now a bare one-line pass-through After this PR clonePartitionSpec(spec) is just |
||
| for i := range fields { | ||
| fields[i] = spec.Field(i) | ||
| } | ||
|
|
||
| return iceberg.NewPartitionSpecID(spec.ID(), fields...) | ||
| return spec.Clone() | ||
| } | ||
|
|
||
| func clonePartitionSpecs(specs []iceberg.PartitionSpec) []iceberg.PartitionSpec { | ||
|
|
@@ -2575,8 +2536,7 @@ func cloneSortOrder(order SortOrder) SortOrder { | |
| clone := order | ||
| clone.fields = make([]SortField, len(order.fields)) | ||
| for i, field := range order.fields { | ||
| clone.fields[i] = field | ||
| clone.fields[i].SourceIDs = slices.Clone(field.SourceIDs) | ||
| clone.fields[i] = cloneSortField(field) | ||
| } | ||
|
|
||
| return clone | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import ( | |
| "testing" | ||
|
|
||
| "github.com/apache/iceberg-go" | ||
| iceinternal "github.com/apache/iceberg-go/internal" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
|
|
@@ -42,7 +43,7 @@ func TestMetadataGettersReturnDefensiveCopies(t *testing.T) { | |
| WriteDefault: iceberg.FixedLiteral{3, 4}, | ||
| })}, | ||
| Specs: []iceberg.PartitionSpec{iceberg.NewPartitionSpecID(1, iceberg.PartitionField{ | ||
| SourceIDs: []int{1}, FieldID: 1000, Name: "id", Transform: iceberg.IdentityTransform{}, | ||
| SourceIDs: []int{1}, FieldID: 1000, Name: "id", Transform: &iceberg.BucketTransform{NumBuckets: 16}, | ||
| })}, | ||
| SnapshotList: []Snapshot{{ | ||
| SnapshotID: 2, | ||
|
|
@@ -87,7 +88,7 @@ func TestMetadataGettersReturnDefensiveCopies(t *testing.T) { | |
| orderID: 1, | ||
| fields: []SortField{{ | ||
| SourceIDs: []int{10}, | ||
| Transform: iceberg.IdentityTransform{}, | ||
| Transform: &iceberg.BucketTransform{NumBuckets: 16}, | ||
| }}, | ||
| }}, | ||
| } | ||
|
|
@@ -111,7 +112,9 @@ func TestMetadataGettersReturnDefensiveCopies(t *testing.T) { | |
| partitionField := partitionSpecs[0].Field(0) | ||
| partitionField.SourceIDs[0] = 99 | ||
| partitionField.Name = "mutated" | ||
| partitionField.Transform.(*iceberg.BucketTransform).NumBuckets = 32 | ||
| require.Equal(t, []int{1}, metadata.Specs[0].Field(0).SourceIDs) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor — New transform-mutation assertions in TestMetadataGettersReturnDefensiveCopies are vacuous The added |
||
| require.Equal(t, 16, metadata.Specs[0].Field(0).Transform.(*iceberg.BucketTransform).NumBuckets) | ||
|
|
||
| currentSchema := metadata.CurrentSchema() | ||
| currentSchema.ID = 100 | ||
|
|
@@ -158,10 +161,73 @@ func TestMetadataGettersReturnDefensiveCopies(t *testing.T) { | |
| fields := sortOrders[0].Fields() | ||
| for _, field := range fields { | ||
| field.SourceIDs[0] = 99 | ||
| field.Transform.(*iceberg.BucketTransform).NumBuckets = 32 | ||
| } | ||
| require.Equal(t, []int{10}, metadata.SortOrderList[0].fields[0].SourceIDs) | ||
| require.Equal(t, 16, metadata.SortOrderList[0].fields[0].Transform.(*iceberg.BucketTransform).NumBuckets) | ||
|
|
||
| got, err := json.Marshal(metadata) | ||
| require.NoError(t, err) | ||
| require.JSONEq(t, string(original), string(got)) | ||
| } | ||
|
|
||
| func TestMetadataSchemaGetterCopiesNestedValues(t *testing.T) { | ||
| schema := nestedSchemaWithMutableDefaults() | ||
| metadata := commonMetadata{ | ||
| CurrentSchemaID: schema.ID, | ||
| SchemaList: []*iceberg.Schema{schema}, | ||
| } | ||
| originalFields := schema.Fields() | ||
|
|
||
| cloned := metadata.CurrentSchema() | ||
| cloned.IdentifierFieldIDs[0] = 99 | ||
| fields := cloned.FieldsRef(iceinternal.SchemaRef{}) | ||
| payload := fields[0].Type.(*iceberg.StructType) | ||
| payload.FieldList[0].InitialDefault.([]byte)[0] = 99 | ||
| payload.FieldList[0].WriteDefault.(iceberg.BinaryLiteral)[0] = 99 | ||
| payload.FieldList[1].Name = "changed" | ||
| payload.FieldList[1].Type.(*iceberg.ListType).Element.(*iceberg.StructType).FieldList[0].InitialDefault.([]any)[0].(map[string]any)["bytes"].([]byte)[0] = 99 | ||
| payload.FieldList[2].Type.(*iceberg.MapType).ValueType.(*iceberg.StructType).FieldList[0].WriteDefault.(map[string]any)["values"].([]any)[0] = "changed" | ||
|
|
||
| require.Equal(t, []int{1}, schema.IdentifierFieldIDs) | ||
| require.Equal(t, originalFields, schema.Fields()) | ||
| } | ||
|
|
||
| var cloneSchemaBenchmarkSink *iceberg.Schema | ||
|
|
||
| func BenchmarkCloneSchemaWithNestedDefaults(b *testing.B) { | ||
| schema := nestedSchemaWithMutableDefaults() | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for range b.N { | ||
| cloneSchemaBenchmarkSink = cloneSchema(schema) | ||
| } | ||
| } | ||
|
|
||
| func nestedSchemaWithMutableDefaults() *iceberg.Schema { | ||
| listElement := &iceberg.StructType{FieldList: []iceberg.NestedField{{ | ||
| ID: 5, Name: "element", Type: iceberg.PrimitiveTypes.String, | ||
| InitialDefault: []any{map[string]any{"bytes": []byte{7, 8}}}, | ||
| }}} | ||
| mapValue := &iceberg.StructType{FieldList: []iceberg.NestedField{{ | ||
| ID: 9, Name: "value", Type: iceberg.PrimitiveTypes.String, | ||
| WriteDefault: map[string]any{"values": []any{iceberg.FixedLiteral{10, 11}}}, | ||
| }}} | ||
| payload := &iceberg.StructType{FieldList: []iceberg.NestedField{ | ||
| { | ||
| ID: 2, Name: "binary", Type: iceberg.PrimitiveTypes.Binary, | ||
| InitialDefault: []byte{1, 2, 3}, WriteDefault: iceberg.BinaryLiteral{4, 5, 6}, | ||
| }, | ||
| {ID: 3, Name: "list", Type: &iceberg.ListType{ | ||
| ElementID: 4, Element: listElement, ElementRequired: true, | ||
| }}, | ||
| {ID: 6, Name: "map", Type: &iceberg.MapType{ | ||
| KeyID: 7, KeyType: iceberg.PrimitiveTypes.String, | ||
| ValueID: 8, ValueType: mapValue, ValueRequired: false, | ||
| }}, | ||
| }} | ||
|
|
||
| return iceberg.NewSchemaWithIdentifiers(1, []int{1}, iceberg.NestedField{ | ||
| ID: 1, Name: "payload", Type: payload, | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // 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 ( | ||
| "strconv" | ||
| "testing" | ||
|
|
||
| "github.com/apache/iceberg-go" | ||
| ) | ||
|
|
||
| var clonePartitionSpecsBenchmarkSink []iceberg.PartitionSpec | ||
|
|
||
| func BenchmarkClonePartitionSpecs(b *testing.B) { | ||
| for _, fieldCount := range []int{1, 8, 32} { | ||
| b.Run("fields="+strconv.Itoa(fieldCount), func(b *testing.B) { | ||
| specs := []iceberg.PartitionSpec{ | ||
| iceberg.NewPartitionSpecID(1, partitionSpecCloneBenchmarkFields(fieldCount)...), | ||
| } | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| for range b.N { | ||
| clonePartitionSpecsBenchmarkSink = clonePartitionSpecs(specs) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func partitionSpecCloneBenchmarkFields(count int) []iceberg.PartitionField { | ||
| fields := make([]iceberg.PartitionField, count) | ||
| for i := range fields { | ||
| fields[i] = iceberg.PartitionField{ | ||
| SourceIDs: []int{i + 1}, FieldID: i + 1000, | ||
| Name: "field", Transform: iceberg.IdentityTransform{}, | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit — Benchmark helper builds a spec with duplicate partition field names partitionSpecCloneBenchmarkFields assigns Name: "field" to all N fields, producing a spec UnmarshalJSON would reject and that no real table can have. Using strconv.Itoa(i) for the name (strconv is already imported) makes the benchmark measure a realistic spec, including distinct url.QueryEscape work in initialize(). |
||
| } | ||
|
|
||
| return fields | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -422,7 +422,7 @@ func cloneSchema(schema *iceberg.Schema) *iceberg.Schema { | |
| return iceberg.NewSchemaWithIdentifiers( | ||
| schema.ID, | ||
| slices.Clone(schema.IdentifierFieldIDs), | ||
| cloneNestedFields(schema.Fields())..., | ||
| schema.Fields()..., | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. major — View cloneSchema deep-copy guarantee is entirely unpinned; guarding test is vacuous This PR removes view's local cloneNestedFields/cloneSchemaType and relies on Schema.Fields() being a deep copy. No view test verifies that. TestCloneSchemaCopiesNestedValues (view/metadata_test.go:712) mutates cloned.Field(i), but Schema.Field (schema.go:279) itself returns cloneField(...), so every mutation lands on a throwaway copy. A future change to view cloneSchema or to Fields() would silently alias view metadata's internal field slice with no test failure. Fix: mirror the table-side test and read the internal slice via FieldsRef(internal.SchemaRef{}) as TestMetadataSchemaGetterCopiesNestedValues already does. Evidence |
||
| } | ||
|
|
||
|
|
@@ -439,40 +439,6 @@ func cloneSchemas(schemas []*iceberg.Schema) []*iceberg.Schema { | |
| return clones | ||
| } | ||
|
|
||
| func cloneNestedFields(fields []iceberg.NestedField) []iceberg.NestedField { | ||
| clones := slices.Clone(fields) | ||
| for i := range clones { | ||
| clones[i].Type = cloneSchemaType(clones[i].Type) | ||
| clones[i].InitialDefault = iceberg.CloneDefaultValue(clones[i].InitialDefault) | ||
| clones[i].WriteDefault = iceberg.CloneDefaultValue(clones[i].WriteDefault) | ||
| } | ||
|
|
||
| return clones | ||
| } | ||
|
|
||
| func cloneSchemaType(typ iceberg.Type) iceberg.Type { | ||
| switch typ := typ.(type) { | ||
| case *iceberg.StructType: | ||
| return &iceberg.StructType{FieldList: cloneNestedFields(typ.FieldList)} | ||
| case *iceberg.ListType: | ||
| return &iceberg.ListType{ | ||
| ElementID: typ.ElementID, | ||
| Element: cloneSchemaType(typ.Element), | ||
| ElementRequired: typ.ElementRequired, | ||
| } | ||
| case *iceberg.MapType: | ||
| return &iceberg.MapType{ | ||
| KeyID: typ.KeyID, | ||
| KeyType: cloneSchemaType(typ.KeyType), | ||
| ValueID: typ.ValueID, | ||
| ValueType: cloneSchemaType(typ.ValueType), | ||
| ValueRequired: typ.ValueRequired, | ||
| } | ||
| default: | ||
| return typ | ||
| } | ||
| } | ||
|
|
||
| func (m *metadata) validate() error { | ||
| if m.Loc == "" { | ||
| return fmt.Errorf("%w: location is required", ErrInvalidViewMetadata) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor — TestPartitionSpecCloneCopiesFields does not test that Clone copies fields
The test mutates clone.Field(0), but PartitionSpec.Field (partitions.go:757) already returns clonePartitionField(...), so the mutation never reaches clone.fields. Only the trailing FieldsBySourceID(1) assertion is non-vacuous (it pins initialize()). To actually guard the deep copy, compare spec/clone after mutating through an accessor that returns internal state, or assert on the SourceIDs slice identity of the two specs' FieldsBySourceID results.