laskoviymishka commented on code in PR #2523:
URL: https://github.com/apache/iceberg-go/pull/2523#discussion_r4556871007
##########
website/src/releases.md:
##########
@@ -44,6 +44,7 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
- **Schema reads now reject invalid identifier fields.** Table and view
metadata with `identifier-field-ids` referring to an optional, float/double,
missing, non-primitive, or improperly nested field now fails to load with
`invalid schema` (for example, `invalid schema: identifier field "foo" must be
required` or `invalid schema: identifier field "foo" cannot be float or
double`). Older iceberg-go releases could load such metadata, but Java Iceberg
already rejects it. To recover an affected table, back up its metadata and use
an atomic catalog metadata repair to remove the invalid identifier IDs or
replace them with IDs of existing, required, non-floating primitive fields
nested only in required structs. Do not edit the currently referenced metadata
file in place. Repeated identifier IDs are deduplicated on read.
+- **`UpdateSchema` rejects deleting an identifier field.** `DeleteColumn` on
an identifier column, or on a struct that contains one, used to succeed and
quietly drop the column from the table's identifier fields. Now `Apply` (and
`Commit`) fails with `cannot delete identifier field …`, as in Java. To delete
the column anyway, also call `SetIdentifierField` with the identifier fields
the schema should keep (for example `SetIdentifierField([][]string{})` to clear
them). This also stops a new column that reuses a deleted identifier column's
name from becoming the identifier.
Review Comment:
Could the "used to succeed" part cover only the plain column delete? At the
merge base, deleting a struct that holds an identifier already failed: with the
nested schema from `TestDeleteIdentifierColumn`,
`DeleteColumn([]string{"key"})` returns `identifier field not found: key.id`
([`Apply` at
df0fe9a](https://github.com/apache/iceberg-go/blob/df0fe9a488985734575e21cf8999c5e68e4e1e6d/table/update_schema.go#L1129-L1142)),
which is also what the description says.
```suggestion
- **`UpdateSchema` rejects deleting an identifier field.** `DeleteColumn` on
an identifier column used to succeed and quietly drop the column from the
table's identifier fields. Now `Apply` (and `Commit`) fails with `cannot delete
identifier field …`, as in Java. Deleting a struct that contains one already
failed, and now reports `cannot delete field … as it will delete nested
identifier field …`. To delete the column anyway, also call
`SetIdentifierField` with the identifier fields the schema should keep (for
example `SetIdentifierField([][]string{})` to clear them). This also stops a
new column that reuses a deleted identifier column's name from becoming the
identifier.
```
##########
table/update_schema_test.go:
##########
@@ -1257,7 +1257,79 @@ func TestSetIdentifierField(t *testing.T) {
SetIdentifierField([][]string{{"name"}}).
Apply()
require.Error(t, err)
- require.ErrorContains(t, err, "identifier field not found:
name")
+ require.ErrorContains(t, err, "cannot delete identifier field
name")
+ })
+}
+
+func TestDeleteIdentifierColumn(t *testing.T) {
+ schemaWithIdentifiers := iceberg.NewSchemaWithIdentifiers(1, []int{1},
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int32, Required: true},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.PrimitiveTypes.String, Required: true},
+ iceberg.NestedField{ID: 3, Name: "age", Type:
iceberg.PrimitiveTypes.Int32},
+ )
+ metadata, err := NewMetadata(schemaWithIdentifiers, nil,
UnsortedSortOrder, "", nil)
+ require.NoError(t, err)
+ newTxn := func() *Transaction {
+ return New([]string{"id"}, metadata, "", nil,
nil).NewTransaction()
+ }
+
+ t.Run("deleting an identifier column fails", func(t *testing.T) {
+ _, err := NewUpdateSchema(newTxn(), true, true).
+ DeleteColumn([]string{"id"}).
+ Apply()
+ require.ErrorContains(t, err, "cannot delete identifier field
id")
+ })
+
+ t.Run("deleting an identifier column with different case fails", func(t
*testing.T) {
+ _, err := NewUpdateSchema(newTxn(), false, true).
+ DeleteColumn([]string{"ID"}).
+ Apply()
+ require.ErrorContains(t, err, "cannot delete identifier field
id")
+ })
+
+ t.Run("delete then add a column with the same name fails", func(t
*testing.T) {
+ _, err := NewUpdateSchema(newTxn(), true, true).
+ DeleteColumn([]string{"id"}).
+ AddColumn([]string{"id"}, iceberg.PrimitiveTypes.Int64,
"", false, nil).
+ Apply()
+ require.ErrorContains(t, err, "cannot delete identifier field
id")
+ })
Review Comment:
Could we pin the rename variant of this too?
`DeleteColumn([]string{"name"})` followed by `RenameColumn([]string{"id"},
"name")` passed at the merge base with the identifier still on field 1, and at
this head `Apply` fails with `cannot delete identifier field name`. That's what
Java's
[`applyChanges`](https://github.com/apache/iceberg/blob/52fc764127402ade8170b1b53ddd95fde13cb75c/core/src/main/java/org/apache/iceberg/SchemaUpdate.java#L541-L562)
does too, since `name` is found in the original schema.
```suggestion
})
t.Run("delete then rename the identifier onto the deleted name fails",
func(t *testing.T) {
_, err := NewUpdateSchema(newTxn(), true, true).
DeleteColumn([]string{"name"}).
RenameColumn([]string{"id"}, "name").
Apply()
require.ErrorContains(t, err, "cannot delete identifier field
name")
})
```
--
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]
