laskoviymishka commented on code in PR #1563:
URL: https://github.com/apache/iceberg-go/pull/1563#discussion_r3680751307
##########
catalog/glue/schema_test.go:
##########
@@ -534,3 +534,47 @@ func
TestSchemasToGlueColumnsClearsPreviousIcebergComment(t *testing.T) {
require.NotNil(t, columns[0].Comment)
assert.Empty(t, aws.ToString(columns[0].Comment))
}
+
+func TestSchemasToGlueColumnsPreservesLogicalOrder(t *testing.T) {
+ original := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 3, Name: "address", Type:
iceberg.StringType{}, Required: false},
+ )
+
+ // "address" is dropped, "email" is added and the remaining columns are
reordered
+ current := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 4, Name: "email", Type:
iceberg.StringType{}, Required: false},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ )
+
+ metadata, err := table.NewMetadata(original, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
+
+ mb, err := table.MetadataBuilderFromBase(metadata, "")
+ assert.NoError(t, err)
+
+ err = mb.AddSchema(current)
+ assert.NoError(t, err)
+
+ err = mb.SetCurrentSchemaID(1)
+ assert.NoError(t, err)
+
+ metadata, err = mb.Build()
+ assert.NoError(t, err)
+
+ columns := schemasToGlueColumns(metadata, nil)
+
+ names := make([]string, len(columns))
+ for i, c := range columns {
+ names[i] = aws.ToString(c.Name)
+ }
+
+ assert.Equal(t, []string{"email", "name", "id", "address"}, names,
"Glue column order must follow the current logical schema")
+ assert.Equal(t, "false", columns[3].Parameters[icebergFieldCurrentKey])
Review Comment:
Same robustness thing here — if a regression returns fewer columns,
`columns[3]` (and the `range 3` loop just below) panics index-out-of-range
instead of failing cleanly. I'd either promote the names check on the line
above to `require.Equal`, or drop a `require.Len(t, columns, 4)` in before this
line so the count is guaranteed first.
##########
catalog/glue/schema_test.go:
##########
@@ -534,3 +534,47 @@ func
TestSchemasToGlueColumnsClearsPreviousIcebergComment(t *testing.T) {
require.NotNil(t, columns[0].Comment)
assert.Empty(t, aws.ToString(columns[0].Comment))
}
+
+func TestSchemasToGlueColumnsPreservesLogicalOrder(t *testing.T) {
+ original := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 3, Name: "address", Type:
iceberg.StringType{}, Required: false},
+ )
+
+ // "address" is dropped, "email" is added and the remaining columns are
reordered
+ current := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 4, Name: "email", Type:
iceberg.StringType{}, Required: false},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ )
+
+ metadata, err := table.NewMetadata(original, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
Review Comment:
I'd switch the five setup steps here (NewMetadata through Build) to
`require.NoError`. With `assert`, a failure in `AddSchema` or `Build` doesn't
stop the test — it falls through to `schemasToGlueColumns` with nil/invalid
metadata and panics, so you get a confusing stack trace instead of a clean
failure at the step that actually broke.
##########
catalog/glue/schema.go:
##########
@@ -43,9 +54,7 @@ func schemasToGlueColumns(metadata table.Metadata,
existingColumns []types.Colum
}
for _, field := range schemaToGlueColumns(schema, false) {
- if _, ok := results[aws.ToString(field.Name)]; !ok {
- results[aws.ToString(field.Name)] = field
- }
+ addColumnWithDedupe(field)
Review Comment:
Not a bug today — `commonMetadata.Schemas()` returns insertion order, so the
historical column that wins dedupe here is stable and matches Java. What I'd
flag is that the `Metadata` interface doesn't promise any iteration order, so
this leans on an implementation detail — a future `Schemas()` that reorders
would silently change which historical column lands here.
A one-line comment noting we assume insertion order (or sorting by schema-ID
before the loop) would make that explicit. Not blocking.
##########
catalog/glue/schema_test.go:
##########
@@ -534,3 +534,47 @@ func
TestSchemasToGlueColumnsClearsPreviousIcebergComment(t *testing.T) {
require.NotNil(t, columns[0].Comment)
assert.Empty(t, aws.ToString(columns[0].Comment))
}
+
+func TestSchemasToGlueColumnsPreservesLogicalOrder(t *testing.T) {
+ original := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 3, Name: "address", Type:
iceberg.StringType{}, Required: false},
+ )
+
+ // "address" is dropped, "email" is added and the remaining columns are
reordered
+ current := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 4, Name: "email", Type:
iceberg.StringType{}, Required: false},
+ iceberg.NestedField{ID: 2, Name: "name", Type:
iceberg.StringType{}, Required: true},
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.Int64Type{}, Required: true},
+ )
+
+ metadata, err := table.NewMetadata(original, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
+
+ mb, err := table.MetadataBuilderFromBase(metadata, "")
+ assert.NoError(t, err)
+
+ err = mb.AddSchema(current)
+ assert.NoError(t, err)
+
+ err = mb.SetCurrentSchemaID(1)
+ assert.NoError(t, err)
+
+ metadata, err = mb.Build()
+ assert.NoError(t, err)
+
+ columns := schemasToGlueColumns(metadata, nil)
+
+ names := make([]string, len(columns))
+ for i, c := range columns {
+ names[i] = aws.ToString(c.Name)
+ }
+
+ assert.Equal(t, []string{"email", "name", "id", "address"}, names,
"Glue column order must follow the current logical schema")
+ assert.Equal(t, "false", columns[3].Parameters[icebergFieldCurrentKey])
+
+ for i := range 3 {
+ assert.Equal(t, "true",
columns[i].Parameters[icebergFieldCurrentKey])
+ }
+}
Review Comment:
This new test covers ordering well. Since the back-fill loop now applies to
appended historical columns too, I'd add a sibling case: a historical schema
column with a comment that the current schema drops, then assert the appended
tail column gets its comment restored. That's exactly the reconciliation
@zeroshade asked to confirm, turned into a regression guard rather than a
manual check.
##########
catalog/glue/schema.go:
##########
@@ -81,33 +90,22 @@ func schemasToGlueColumns(metadata table.Metadata,
existingColumns []types.Colum
}
}
- for name, column := range results {
- if column.Comment != nil {
+ for i := range columns {
+ if columns[i].Comment != nil {
continue
}
- fieldID := column.Parameters[icebergFieldIDKey]
+ fieldID := columns[i].Parameters[icebergFieldIDKey]
Review Comment:
Clearly pre-existing and out of scope here — flagging only because this PR
is the reconciliation point for the back-fill. This loop keys off
`icebergFieldIDKey`, whereas Java keys existing comments by column name, so on
a rename we carry the old comment forward by field ID while Java drops it. Came
in with #1522, not this PR — a short comment on why we key by field ID (or a
follow-up to align with Java) would be enough. Not blocking.
--
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]
