You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/25 20:41:11 UTC
[PR] fix(name_mapping): isolate updated mappings [iceberg-go]
fallintoplace opened a new pull request, #1547:
URL: https://github.com/apache/iceberg-go/pull/1547
## What changed
Clone mapped field IDs and name slices while building the result of UpdateNameMapping. Fields without IDs now retain the independently visited child results as well.
## Why
The update visitor reused FieldID pointers from the input mapping. Mutating the returned mapping could therefore change the original. Appending a renamed field to a name slice with spare capacity could also write into the input backing array during the update itself.
The regression test covers both alias paths across parent and nested fields.
## Testing
- go test .
- go vet .
- go test ./...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(name_mapping): isolate updated mappings [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1547:
URL: https://github.com/apache/iceberg-go/pull/1547
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(name_mapping): isolate updated mappings [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org