badalprasadsingh commented on code in PR #2453:
URL: https://github.com/apache/iceberg-go/pull/2453#discussion_r4495573781
##########
table/update_schema.go:
##########
@@ -174,6 +174,15 @@ func NewUpdateSchema(txn *Transaction, caseSensitive bool,
allowIncompatibleChan
// referenced by historical schemas.
u.lastColumnID = meta.LastColumnID()
+ // A last-column-id below an ID already in use would hand out a
+ // duplicate field ID. Fail with an error rather than letting
+ // iceberg.NewSchema panic later in Apply.
+ if err := meta.validateLastColumnID(); err != nil {
+ u.err = err
+
+ return u
+ }
Review Comment:
for the case when `last-column-id` is lower than an existing field id, this
check fails: every schema change, renames and doc edits that need no new id,
those work today so with this the table can't change its schema from go at all
but I would suggest to go with a different approach:
#1988 fixed the same problem for partition ids without an error, I'd like to
match it:
1. `UpdateSchema`: start new ids above both the stored counter and the
highest id in any schema, old ones too
2. `AddSchema`: save the same value as the counter so metadata go writes is
correct too
this approach never reuses an id, nothing fails, and this check can go too
let me know your take on this
##########
table/update_schema_test.go:
##########
@@ -58,6 +59,34 @@ func TestNewUpdateSchemaWithNilTransactionReturnsError(t
*testing.T) {
assert.ErrorContains(t, err, "transaction is nil")
}
+func TestUpdateSchemaLastColumnIDBelowHighestFieldIDReturnsError(t *testing.T)
{
+ // Field "z" has ID 3, but the counter says 2, so the next new
+ // column would reuse ID 3.
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-column-id": 3`, `"last-column-id": 2`, 1)
+ meta, err := ParseMetadataString(data)
+ require.NoError(t, err)
+ require.Equal(t, 2, meta.LastColumnID())
Review Comment:
can we add a case where the highest field id exists only in an older schema
(a dropped col), and chexck that a new col gets an id above it?
without this: a fix that only looks at the current schema would pass and
silently reuse the dropped col id
--
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]
