ShivamSharma43 commented on code in PR #2523:
URL: https://github.com/apache/iceberg-go/pull/2523#discussion_r4560331502
##########
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:
Applied in 1ace5c6, thanks. The note now says only the plain column delete
used to succeed, and that the struct delete already failed and now has a
clearer message.
##########
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:
Added in 1ace5c6 as `delete then rename the identifier onto the deleted name
fails`. It passes at this head.
--
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]
