laskoviymishka commented on code in PR #1547:
URL: https://github.com/apache/iceberg-go/pull/1547#discussion_r3690112836
##########
name_mapping.go:
##########
@@ -182,16 +182,20 @@ func (u *updateNameMappingVisitor) Fields(st
[]MappedField, fieldResults []Mappe
func (u *updateNameMappingVisitor) Field(field MappedField, fieldResult
[]MappedField) MappedField {
if field.FieldID == nil {
- return field
+ return MappedField{
+ Names: slices.Clone(field.Names),
+ Fields: fieldResult,
+ }
}
- fieldNames := field.Names
+ fieldID := *field.FieldID
Review Comment:
Now that `fieldID` holds the owned copy, the two remaining `*field.FieldID`
derefs (the `u.updates` lookup just below and the `addNewFields` call) could
just use `fieldID`. Same value, keeps the "own a fresh int" intent unambiguous.
Fine as a follow-up.
##########
name_mapping.go:
##########
@@ -200,7 +204,8 @@ func (u *updateNameMappingVisitor) Field(field MappedField,
fieldResult []Mapped
func (u *updateNameMappingVisitor) removeReassignedNames(field MappedField,
assignments map[string]int) *MappedField {
removedNames := make(map[string]struct{})
for _, name := range field.Names {
- if assignedID, exists := assignments[name]; exists &&
assignedID != *field.FieldID {
+ assignedID, exists := assignments[name]
+ if exists && (field.FieldID == nil || assignedID !=
*field.FieldID) {
Review Comment:
The nil-guard here is the right fix. While we're in this function though,
it's the one spot the isolation change doesn't reach: the return a few lines
down builds a fresh `MappedField` but copies `field.FieldID` by pointer and
assigns `field.Fields` uncloned, so a caller that mutated the result could
reach back into the original.
It's latent today since every call site passes an already-visited copy, but
it's inconsistent with what the rest of the PR sets up. I'd clone both here
too. wdyt?
##########
name_mapping.go:
##########
@@ -182,16 +182,20 @@ func (u *updateNameMappingVisitor) Fields(st
[]MappedField, fieldResults []Mappe
func (u *updateNameMappingVisitor) Field(field MappedField, fieldResult
[]MappedField) MappedField {
if field.FieldID == nil {
- return field
+ return MappedField{
Review Comment:
This branch quietly does two things now: it isolates `Names`, and it swaps
`field.Fields` for `fieldResult`, which propagates child visit results the old
`return field` silently dropped. The second part is a real fix, renames under
an anonymous parent used to be lost, and it looks right.
Only thing worth confirming: PyIceberg's `_UpdateMapping.field()` explicitly
returns the original field unchanged when `field_id is None`, so this diverges
from that reference. I'd confirm the propagation is intended and drop a
one-line comment, since it's not obvious from the diff that this changes
output. wdyt?
##########
name_mapping_test.go:
##########
@@ -184,6 +184,91 @@ func TestUpdateNameMapping(t *testing.T) {
assert.Equal(t, originalMapping, result)
})
+ t.Run("result does not alias the original", func(t *testing.T) {
+ fieldID, childID, anonymousChildID := 1, 2, 3
+ parentNames := make([]string, 2)
+ parentNames[0] = "parent"
+ original := iceberg.NameMapping{
+ {
+ FieldID: &fieldID,
+ Names: parentNames[:1],
+ Fields: []iceberg.MappedField{{
+ FieldID: &childID,
+ Names: []string{"child"},
+ }},
+ },
+ {
+ Names: []string{"anonymous"},
+ Fields: []iceberg.MappedField{{
+ FieldID: &anonymousChildID,
+ Names: []string{"anonymous-child"},
+ }},
+ },
+ }
+
+ result, err := iceberg.UpdateNameMapping(
+ original,
+ map[int]iceberg.NestedField{1: {ID: 1, Name:
"renamed-parent"}},
+ map[int][]iceberg.NestedField{},
+ )
+ require.NoError(t, err)
+ assert.Empty(t, parentNames[1])
Review Comment:
Two small robustness follow-ups in this block. `assert.Empty(t,
parentNames[1])` is the assertion that proves `slices.Clone` happened, and if
it ever fails the `*result[0].FieldID` derefs right below panic instead of
reporting cleanly, so I'd make it `require`. Same reason to add a
`require.NotNil` on the result FieldIDs before mutating them, so a regression
that returned a nil FieldID fails the test rather than crashing it.
##########
name_mapping_test.go:
##########
@@ -184,6 +184,91 @@ func TestUpdateNameMapping(t *testing.T) {
assert.Equal(t, originalMapping, result)
})
+ t.Run("result does not alias the original", func(t *testing.T) {
+ fieldID, childID, anonymousChildID := 1, 2, 3
Review Comment:
The "remove reassigned name" subtest just below uses `makeID(1)` for this.
Could use `makeID(1)`/`makeID(2)`/`makeID(3)` inline here too instead of the
three named vars, to match the other subtests. Follow-up whenever.
##########
name_mapping_test.go:
##########
@@ -184,6 +184,91 @@ func TestUpdateNameMapping(t *testing.T) {
assert.Equal(t, originalMapping, result)
})
+ t.Run("result does not alias the original", func(t *testing.T) {
+ fieldID, childID, anonymousChildID := 1, 2, 3
+ parentNames := make([]string, 2)
+ parentNames[0] = "parent"
+ original := iceberg.NameMapping{
+ {
+ FieldID: &fieldID,
+ Names: parentNames[:1],
+ Fields: []iceberg.MappedField{{
+ FieldID: &childID,
+ Names: []string{"child"},
+ }},
+ },
+ {
+ Names: []string{"anonymous"},
+ Fields: []iceberg.MappedField{{
+ FieldID: &anonymousChildID,
+ Names: []string{"anonymous-child"},
+ }},
+ },
+ }
+
+ result, err := iceberg.UpdateNameMapping(
+ original,
+ map[int]iceberg.NestedField{1: {ID: 1, Name:
"renamed-parent"}},
+ map[int][]iceberg.NestedField{},
+ )
+ require.NoError(t, err)
+ assert.Empty(t, parentNames[1])
+
+ *result[0].FieldID = 10
Review Comment:
This proves element-level isolation (cloned child FieldID and Names), which
is great, but it doesn't catch `Fields` slice-header aliasing: appending to
`result[0].Fields` with spare capacity could still reach into the original.
That's the same class of bug this PR fixes on `Names`, and it's the gap that
would let the `removeReassignedNames` point above slip through. Worth an
append-based assertion, or sizing an inner `Fields` slice with spare capacity
so the shared-backing path is detectable.
--
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]
