Shruthirdy opened a new pull request, #2475: URL: https://github.com/apache/iceberg-go/pull/2475
Closes #2393. When two fields share a field ID (most often both left unset, so both are 0), `CreateTable` succeeds but can bind the partition to the wrong column. `reassignIDs` maps each partition source ID back to a column name with `FindColumnName`, and with two fields at ID 0 the ID → name map keeps whichever name Go's map iteration writes last. So `day(created_at)` usually became `day(event)`, `Append` wrote null partition values, and the first error only appeared at query time. Sort orders go through the same lookup and had the same problem. `checkDuplicateFieldIDs` doesn't catch this because it skips IDs ≤ 0 (#879). `NewMetadataWithUUID` now rejects a user schema with duplicate field IDs, including 0 and negative IDs, before `reassignIDs` runs, next to the existing reserved-ID check (#1107). A single field with ID 0 is still valid (#761). This applies to every catalog that builds metadata through `NewMetadata`. Before: `CreateTable` succeeds, and the table is partitioned by `day(event)` in about 7 runs out of 8. After: ``` invalid schema: multiple fields for id 0: created_at and event ``` ### Testing - `TestValidateUniqueFieldIDs`: duplicate 0 and negative IDs, duplicates in nested structs, list elements and map values are rejected; unique IDs and a single 0 pass. - `TestNewMetadataRejectsDuplicateFieldIDs`: the issue's schema with a partition or sort order on `created_at` now fails with `ErrInvalidSchema`; a single 0 keeps both on `created_at`. - Both fail if the fix is removed. - `go test ./...` and `golangci-lint` pass. ### Notes - This also rejects unpartitioned schemas with duplicate unset IDs (e.g. two fields with ID 0), which worked before. This matches Java, where the `Schema` constructor rejects duplicate field IDs. - Adds `iceberg.ValidateUniqueFieldIDs`, which reuses the `checkDuplicateFieldIDs` walker but also checks IDs ≤ 0. `table` can't call the unexported one. - I also considered fixing it at the root instead, by making `ByID` fail on duplicate IDs the way Java's `ImmutableMap` does. `FindColumnName` ignores that error and only returns `(string, bool)`, so the user would get a misleading "partition source field 0 not found", and it would change behavior for every caller of that map. Validating the input in `NewMetadataWithUUID` instead keeps the change local and gives a clear error. - `Schema.UnmarshalJSON` has the same gap: it uses the same check, which skips IDs ≤ 0, so a metadata file with two `"id": 0` fields still parses. I left it out because changing the parser affects reading existing tables: a table that loads today could stop loading. I'm happy to open a separate issue for it so that can be decided on its own. I'm happy to change the approach if you'd prefer something different on any of the points above. -- 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]
