badalprasadsingh commented on code in PR #2511:
URL: https://github.com/apache/iceberg-go/pull/2511#discussion_r4534609189
##########
name_mapping.go:
##########
@@ -352,13 +354,18 @@ func (n indexedNameMappingAccessor) indexFields(field
*MappedField) {
for i := range field.Fields {
mappedField := &field.Fields[i]
for _, name := range mappedField.Names {
- if _, exists := fieldsByName[name]; !exists {
- fieldsByName[name] = mappedField
+ if existing, ok := fieldsByName[name]; ok && existing
!= mappedField {
+ return fmt.Errorf("%w: duplicate name mapping
%q", ErrInvalidSchema, name)
Review Comment:
nit: the error gives only the name, so a nested duplicate such as element is
hard to find in a large mapping. Adding both field IDs helps, and your
ErrorContains checks still pass:
```go
return fmt.Errorf("%w: duplicate name mapping %q (field IDs %d and %d)",
ErrInvalidSchema, name, existing.ID(), mappedField.ID())
```
##########
name_mapping.go:
##########
@@ -352,13 +354,18 @@ func (n indexedNameMappingAccessor) indexFields(field
*MappedField) {
for i := range field.Fields {
mappedField := &field.Fields[i]
for _, name := range mappedField.Names {
- if _, exists := fieldsByName[name]; !exists {
- fieldsByName[name] = mappedField
+ if existing, ok := fieldsByName[name]; ok && existing
!= mappedField {
Review Comment:
scans of files without field ids now fail on these mappings, where today the
first entry silently wins
iceberg-ava fails too, but users will see a new error, so I'd add a bullet
under "Unreleased compatibility notes" in `website/src/releases.md`, like the
identifier-field one
##########
name_mapping_test.go:
##########
@@ -117,6 +117,74 @@ func TestApplyNameMappingUsesAlias(t *testing.T) {
assert.Equal(t, 1, result.Field(0).ID)
}
+func TestApplyNameMappingRejectsDuplicateAliases(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 100, Name: "shared", Type:
iceberg.PrimitiveTypes.String},
+ )
+ mapping := iceberg.NameMapping{
+ {FieldID: new(1), Names: []string{"original", "shared"}},
+ {FieldID: new(2), Names: []string{"shared"}},
+ }
+
+ result, err := iceberg.ApplyNameMapping(schema, mapping)
+ require.Nil(t, result)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.ErrorContains(t, err, `duplicate name mapping "shared"`)
+}
+
+func TestApplyNameMappingRejectsDuplicateNestedAliases(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ ID: 100, Name: "parent",
+ Type: &iceberg.StructType{FieldList:
[]iceberg.NestedField{
+ {ID: 101, Name: "shared", Type:
iceberg.PrimitiveTypes.String},
+ }},
+ },
+ )
+ mapping := iceberg.NameMapping{
+ {FieldID: new(1), Names: []string{"parent"}, Fields:
[]iceberg.MappedField{
+ {FieldID: new(2), Names: []string{"original",
"shared"}},
+ {FieldID: new(3), Names: []string{"shared"}},
+ }},
+ }
+
+ result, err := iceberg.ApplyNameMapping(schema, mapping)
+ require.Nil(t, result)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.ErrorContains(t, err, `duplicate name mapping "shared"`)
+}
+
+func TestApplyNameMappingAllowsSameNestedNameInDifferentStructs(t *testing.T) {
Review Comment:
no test covers the `existing != mappedField` exemption, so removing it would
go unnoticed: accept a field that lists its own name twice (names is a set) ->
add an allow case
##########
name_mapping.go:
##########
@@ -529,7 +536,11 @@ func (n *nameMapProjectVisitor) Variant(v VariantType,
variantPartner *MappedFie
func ApplyNameMapping(schemaWithoutIDs *Schema, nameMapping NameMapping)
(*Schema, error) {
partner := &MappedField{Fields: nameMapping}
- accessor := newIndexedNameMappingAccessor(partner)
+ accessor, err := newIndexedNameMappingAccessor(partner)
+ if err != nil {
+ return nil, err
+ }
Review Comment:
P1 (not new here): two mapping entries with the same field id still crash
reads, if a file has both columns, NewSchema panics in a scan go-routine and
kills the process.
as this PR validates mappings, better: return an error too (or file an issue
and raise a follow-up pr).
```go
fields := top.Type.(*StructType).FieldList
if err := checkDuplicateFieldIDs(nil, fields); err != nil {
return nil, err
}
return NewSchema(schemaWithoutIDs.ID, fields...), nil
```
--
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]
