You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dttung2905 (via GitHub)" <gi...@apache.org> on 2025/12/15 19:25:55 UTC
[PR] fix(schema): Add duplicate field ID checking mechanism [iceberg-go]
dttung2905 opened a new pull request, #658:
URL: https://github.com/apache/iceberg-go/pull/658
As outlined in the issue, currently we do not perform field id duplication check in this `IndexNameByID` method. This leads to the issue of last one wins if multiple field ID are presented in the schema
https://github.com/apache/iceberg-go/blob/f886a245b43df648b9b42148ed0d77c04f1d34c8/schema.go#L788-L800
https://github.com/apache/iceberg-go/blob/f886a245b43df648b9b42148ed0d77c04f1d34c8/schema.go#L810-L817
I added a quick validation under `ByID()` and panic if duplicates are found and put this schema validation under `init()` as suggested in the issue
Fixes https://github.com/apache/iceberg-go/issues/593
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#issuecomment-3721213025
This is ballooning into something more complicated than I thought initally. Let me find some time this weekend to look thoroughly into this
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#discussion_r2662117239
##########
schema.go:
##########
@@ -809,8 +814,12 @@ type indexByName struct {
func (i *indexByName) ByID() map[int]string {
idToName := make(map[int]string)
- for k, v := range i.index {
- idToName[v] = k
+ for name, id := range i.index {
+ if existingName, ok := idToName[id]; ok && existingName != name {
+ panic(fmt.Errorf("%w: multiple fields for id %d: %s and %s",
+ ErrInvalidSchema, id, existingName, name))
+ }
+ idToName[id] = name
}
Review Comment:
that would be a good spot yea
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#discussion_r2644409033
##########
schema.go:
##########
@@ -809,8 +814,12 @@ type indexByName struct {
func (i *indexByName) ByID() map[int]string {
idToName := make(map[int]string)
- for k, v := range i.index {
- idToName[v] = k
+ for name, id := range i.index {
+ if existingName, ok := idToName[id]; ok && existingName != name {
+ panic(fmt.Errorf("%w: multiple fields for id %d: %s and %s",
+ ErrInvalidSchema, id, existingName, name))
+ }
+ idToName[id] = name
}
Review Comment:
Let me find another way to go around this . Are you saying we test it in the addField() method? :thinking:
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#discussion_r2641331157
##########
manifest_test.go:
##########
@@ -462,7 +462,7 @@ var (
NestedField{ID: 16, Name: "improvement_surcharge", Type: PrimitiveTypes.Float64, Required: false},
NestedField{ID: 17, Name: "total_amount", Type: PrimitiveTypes.Float64, Required: true},
NestedField{ID: 18, Name: "congestion_surcharge", Type: PrimitiveTypes.Float64, Required: false},
- NestedField{ID: 19, Name: "VendorID", Type: PrimitiveTypes.Int32, Required: false},
+ NestedField{ID: 19, Name: "vendor_id_alt", Type: PrimitiveTypes.Int32, Required: false},
Review Comment:
why change the field name? Isn't this intended to test the case of multiple fields with the same name but different IDs?
##########
schema.go:
##########
@@ -809,8 +814,12 @@ type indexByName struct {
func (i *indexByName) ByID() map[int]string {
idToName := make(map[int]string)
- for k, v := range i.index {
- idToName[v] = k
+ for name, id := range i.index {
+ if existingName, ok := idToName[id]; ok && existingName != name {
+ panic(fmt.Errorf("%w: multiple fields for id %d: %s and %s",
+ ErrInvalidSchema, id, existingName, name))
+ }
+ idToName[id] = name
}
Review Comment:
I prefer that we forbid duplicate IDs in the first place so that we don't need to have this check at all
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#issuecomment-5254681664
**🛠️ Maintainer triage note for @dttung2905** · by `zeroshade` · 2026-08-11 UTC
This PR has been inactive for over 30 days with review feedback waiting. If you're still working on it, please reply or push an update addressing the open review threads. CI is also failing on the current head, so it will need a rebase and a green run.
If we don't hear back within 30 days we may close it to keep the review queue manageable — reopening (or a fresh PR) later is always welcome. See the [Pull Request quality criteria](https://github.com/apache/iceberg-go/blob/main/CONTRIBUTING.md).
<sub>_Automated triage — may be imperfect; a maintainer takes the next look._</sub>
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #658:
URL: https://github.com/apache/iceberg-go/pull/658#discussion_r2623457301
##########
schema.go:
##########
@@ -86,6 +86,11 @@ func (s *Schema) init() {
s.lazyNameMapping = sync.OnceValue(func() NameMapping {
return createMappingFromSchema(s)
})
+
+ // Validate that the schema does not contain duplicate field IDs.
+ if _, err := IndexNameByID(s); err != nil {
+ panic(err)
+ }
Review Comment:
I think we should not have runtime panics that can be caused by user error, we should make the schema init fallible instead and return an err
--
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(schema): Add duplicate field ID checking mechanism [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 closed pull request #658: fix(schema): Add duplicate field ID checking mechanism
URL: https://github.com/apache/iceberg-go/pull/658
--
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