zeroshade commented on code in PR #1990:
URL: https://github.com/apache/iceberg-go/pull/1990#discussion_r3937635865
##########
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:
**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.
<details><summary>Evidence</summary>
```text
Mutated view/metadata.go:425 `schema.Fields()...` ->
`schema.FieldsRef(iceint.SchemaRef{})...` (full aliasing); `go test -count=1
./view` => `ok github.com/apache/iceberg-go/view 0.408s`. Same mutation on
table/metadata.go:2407 correctly fails: `--- FAIL:
TestMetadataSchemaGetterCopiesNestedValues` with diff `InitialDefault: 01 02 03
-> 63 02 03` and `Name: "list" -> "changed"`. Restored with git checkout; git
status --porcelain empty.
```
</details>
##########
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}
Review Comment:
**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.
##########
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:
**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().
##########
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
require.Equal(t, []int{1}, metadata.Specs[0].Field(0).SourceIDs)
Review Comment:
**minor** — New transform-mutation assertions in
TestMetadataGettersReturnDefensiveCopies are vacuous
The added `partitionField.Transform.(*iceberg.BucketTransform).NumBuckets =
32` (line 115) and the sort-order equivalent (line 166) read through
PartitionSpec.Field() and SortOrder.Fields(), both of which already deep-copy
the transform (partitions.go:757, table/sorting.go:328). The assertions
therefore pass regardless of whether cloneSortOrder/PartitionSpec.Clone copy
transforms, so they add no regression protection for the behavior the PR
description claims they cover. (The cloneSortOrder guarantee itself is still
pinned by TestMetadataBuilderFromBaseCopiesBuiltinMetadata, so this is coverage
theater rather than a hole.)
##########
table/metadata.go:
##########
@@ -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())
Review Comment:
**nit** — clonePartitionSpec is now a bare one-line pass-through
After this PR clonePartitionSpec(spec) is just `return spec.Clone()`. Five
call sites could call spec.Clone() directly and drop the wrapper, or keep it if
a doc comment explains the indirection.
--
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]
