laskoviymishka commented on code in PR #1990: URL: https://github.com/apache/iceberg-go/pull/1990#discussion_r3966666681
########## table/metadata_partition_bench_test.go: ########## @@ -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{}, Review Comment: `clonePartitionField`'s switch only allocates for `*BucketTransform` / `*TruncateTransform`; `IdentityTransform{}` falls straight through with no copy. So the benchmark measures the cheapest path and never touches the pointer-cloning branch that's the actual behavior change here. I'd swap in (or add a variant with) `&iceberg.BucketTransform{NumBuckets: 16}` so the numbers reflect the work the clone actually does. ########## partitions.go: ########## @@ -543,6 +543,16 @@ func NewPartitionSpecID(id int, fields ...PartitionField) PartitionSpec { return ret } +func (ps PartitionSpec) Clone() PartitionSpec { Review Comment: `Clone()` is the first exported clone method on a spec type here; the rest of the codebase surfaces isolation through per-getter copies (`Field`, `FieldsBySourceID`), not an explicit `Clone`. The only callers are the internal `clonePartitionSpec` wrapper and tests, so I'd either keep it unexported as `clone()`, or add a godoc line stating the full-independence guarantee, since exporting locks that contract in. Minor while we're here: the value receiver copies the source struct (including the `sourceIdToFields` map header) on every call, and that copy is discarded once `initialize()` allocates a fresh map. Every other read and mutating method on `PartitionSpec` is a pointer receiver. A `*PartitionSpec` receiver would match the pattern and skip the struct copy, and the body doesn't need to change. wdyt? ########## partitions_test.go: ########## @@ -123,6 +123,23 @@ func TestNewPartitionSpecIDCopiesFields(t *testing.T) { assert.Equal(t, []int{1}, restored.SourceIDs) } +func TestPartitionSpecCloneCopiesFields(t *testing.T) { + transform := &iceberg.BucketTransform{NumBuckets: 16} + spec := iceberg.NewPartitionSpecID(7, iceberg.PartitionField{ + SourceIDs: []int{1}, FieldID: 1000, Name: "id", Transform: transform, + }) + + clone := spec.Clone() + field := clone.Field(0) + field.SourceIDs[0] = 2 + field.Transform.(*iceberg.BucketTransform).NumBuckets = 32 Review Comment: This doesn't actually pin what it's meant to. `clone.Field(0)` returns a fresh `clonePartitionField(...)` copy, so mutating `field.Transform` and `field.SourceIDs` touches a throwaway, and the assertions read back through `spec.Field(0)` which copies again. The test would stay green even if `Clone()` were a no-op that returned the original. Since this file is package `iceberg`, I'd reach into the raw fields so a real sharing bug turns it red: ```go cloneTransform, ok := clone.fields[0].Transform.(*iceberg.BucketTransform) require.True(t, ok) require.False(t, cloneTransform == transform, "Clone must not share the transform pointer") cloneTransform.NumBuckets = 32 require.Equal(t, 16, spec.fields[0].Transform.(*iceberg.BucketTransform).NumBuckets) ``` Same idea for `SourceIDs` (mutate `clone.fields[0].SourceIDs[0]` and assert `spec.fields[0].SourceIDs` is unchanged). The comma-ok form is worth using on these so a fixture change surfaces as a failed assertion rather than a panic. ########## table/metadata.go: ########## @@ -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) Review Comment: Worth calling this out explicitly: the old `cloneSortOrder` only cloned `SourceIDs` and left the `Transform` pointer aliased between the clone and the source, so a caller mutating a `*BucketTransform` through a `SortOrders()` result could corrupt the parent metadata. `cloneSortField` closes that. It's a real correctness fix riding along in a perf PR, so I'd note it in the description (or a short comment here) rather than leaving it silent. The hardened sort-order assertion I suggested in the getters test is what would pin it. ########## view/metadata.go: ########## @@ -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()..., Review Comment: The table-side `cloneSchema` change now has a rigorous guard in `TestMetadataSchemaGetterCopiesNestedValues`, which reaches the clone's raw fields via `FieldsRef` and mutates nested defaults. This view-side removal is the same edit but gets no equivalent test; the existing `TestCloneSchemaCopiesNestedValues` reads through `Field(0)` on both sides, so it's vacuous for nested-type mutations in the same way. `Schema.Fields()` is shared, so the behavior is covered transitively by the table test, but the view `cloneSchema` call chain isn't exercised at the unit level. I'd add a parallel view test mirroring the table one (`FieldsRef` on the cloned result) so this file has its own guard. ########## table/metadata_getters_test.go: ########## @@ -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 Review Comment: Same problem as the `Clone` test: `partitionSpecs[0].Field(0)` and `sortOrders[0].Fields()` (the loop lower down) both hand back fresh copies via `clonePartitionField` / `cloneSortField`, so the new `NumBuckets = 32` mutations land on throwaways and the readbacks go through accessors that were never touched. Both new transform assertions pass regardless of whether the getters copy the transform pointer. This test is package `table`, so it can read the raw fields directly. For the sort order that matters especially: asserting on `metadata.SortOrderList[0].fields[0].Transform` after mutating the returned copy would actually catch the pre-PR `cloneSortOrder` aliasing (see my note on `cloneSortOrder`), where the transform pointer was shared. I'd mutate the returned copy's transform and assert the raw source field is still `16` for both the partition and sort-order cases. -- 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]
