laskoviymishka commented on code in PR #1522:
URL: https://github.com/apache/iceberg-go/pull/1522#discussion_r3646099320
##########
catalog/glue/schema.go:
##########
@@ -49,6 +49,27 @@ func schemasToGlueColumns(metadata table.Metadata)
[]types.Column {
}
}
+ existingComments := make(map[string]*string)
+ for _, column := range existingColumns {
+ name := aws.ToString(column.Name)
+ if column.Comment == nil {
+ continue
+ }
+ if _, ok := existingComments[name]; !ok {
+ existingComments[name] = column.Comment
+ }
+ }
+
+ for name, column := range results {
+ if aws.ToString(column.Comment) != "" {
Review Comment:
This closes the clobbering bug but opens its mirror: once a Glue comment
exists, there's no way to clear it. A user who sets `Doc` then clears it
produces `Comment = aws.String("")` from `fieldToGlueColumn`, this loop sees
empty and restores the old comment, so the clear is dropped forever.
Root cause is `fieldToGlueColumn` writing `aws.String(field.Doc)`
unconditionally — `nil` and empty-string become indistinguishable downstream.
I'd have it emit `nil` when `Doc == ""`, then gate this loop on `column.Comment
== nil` rather than the empty-string check. That distinguishes "doc never set,
defer to Glue" from "explicitly cleared." Thoughts?
##########
catalog/glue/glue_test.go:
##########
@@ -280,6 +280,35 @@ func
TestGlueConstructParametersPreservesReservedParameters(t *testing.T) {
assert.Equal("value", params["custom"])
}
+func TestGlueConstructTableInputPreservesExistingColumnComments(t *testing.T) {
+ metadata, err := table.NewMetadata(
+ testSchema,
+ nil,
+ table.UnsortedSortOrder,
+ "s3://test-bucket/test_table",
+ nil,
+ )
+ require.NoError(t, err)
+ staged := table.New(
+ TableIdentifier("test_database", "test_table"),
+ metadata,
+ "s3://test-bucket/test_table/metadata/v2.metadata.json",
+ nil,
+ nil,
+ )
+ previous := &types.Table{
+ StorageDescriptor: &types.StorageDescriptor{
+ Columns: []types.Column{
+ {Name: aws.String("foo"), Comment:
aws.String("existing comment")},
+ },
+ },
+ }
+
+ input := constructTableInput("test_table", staged, previous)
+
+ require.Equal(t, "existing comment",
aws.ToString(input.StorageDescriptor.Columns[0].Comment))
Review Comment:
This only checks `foo`. `testSchema` also has `bar`/`baz` with no matching
previous column, so I'd assert those come back with empty comments too — that's
what catches anything spurious getting back-filled.
`constructTableInput` now branches on `previousGlueTable` being nil (the
create path), and that branch is only exercised implicitly. A case passing
`nil` previous would be worth having so the refactor's new nil-guard is
actually covered.
##########
catalog/glue/schema_test.go:
##########
@@ -448,6 +448,29 @@ func TestSchemasToGlueColumns(t *testing.T) {
metadata, err = mb.Build()
assert.NoError(t, err)
- columns := schemasToGlueColumns(metadata)
+ columns := schemasToGlueColumns(metadata, nil)
assert.Equal(t, expectedColumns, columns)
}
+
+func TestSchemasToGlueColumnsPreservesExistingComments(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "preserved", Type:
iceberg.PrimitiveTypes.String},
+ iceberg.NestedField{ID: 2, Name: "overridden", Type:
iceberg.PrimitiveTypes.String, Doc: "iceberg comment"},
+ iceberg.NestedField{ID: 3, Name: "missing", Type:
iceberg.PrimitiveTypes.String},
+ )
+ metadata, err := table.NewMetadata(schema, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
+
+ existingColumns := []types.Column{
+ {Name: aws.String("preserved"), Comment: aws.String("existing
comment")},
+ {Name: aws.String("preserved"), Comment: aws.String("duplicate
comment")},
+ {Name: aws.String("overridden"), Comment: aws.String("existing
comment")},
+ {Name: aws.String("missing")},
+ }
+
+ columns := schemasToGlueColumns(metadata, existingColumns)
Review Comment:
This covers preserve/override/missing but not the case that actually gates
the design discussion — a field whose `Doc` was non-empty and is now `""`.
Whatever we land on for the clear semantics, I'd add a case here so the
decision is pinned down and can't silently regress.
##########
catalog/glue/schema_test.go:
##########
@@ -448,6 +448,29 @@ func TestSchemasToGlueColumns(t *testing.T) {
metadata, err = mb.Build()
assert.NoError(t, err)
- columns := schemasToGlueColumns(metadata)
+ columns := schemasToGlueColumns(metadata, nil)
assert.Equal(t, expectedColumns, columns)
}
+
+func TestSchemasToGlueColumnsPreservesExistingComments(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "preserved", Type:
iceberg.PrimitiveTypes.String},
+ iceberg.NestedField{ID: 2, Name: "overridden", Type:
iceberg.PrimitiveTypes.String, Doc: "iceberg comment"},
+ iceberg.NestedField{ID: 3, Name: "missing", Type:
iceberg.PrimitiveTypes.String},
+ )
+ metadata, err := table.NewMetadata(schema, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
+
+ existingColumns := []types.Column{
+ {Name: aws.String("preserved"), Comment: aws.String("existing
comment")},
+ {Name: aws.String("preserved"), Comment: aws.String("duplicate
comment")},
+ {Name: aws.String("overridden"), Comment: aws.String("existing
comment")},
+ {Name: aws.String("missing")},
+ }
+
+ columns := schemasToGlueColumns(metadata, existingColumns)
+
+ assert.Equal(t, "existing comment", aws.ToString(columns[0].Comment))
Review Comment:
These assert by positional index, which leans on the sort-by-field-ID order
holding. A named lookup would make the intent explicit and survive a reorder.
##########
catalog/glue/schema_test.go:
##########
@@ -448,6 +448,29 @@ func TestSchemasToGlueColumns(t *testing.T) {
metadata, err = mb.Build()
assert.NoError(t, err)
- columns := schemasToGlueColumns(metadata)
+ columns := schemasToGlueColumns(metadata, nil)
assert.Equal(t, expectedColumns, columns)
}
+
+func TestSchemasToGlueColumnsPreservesExistingComments(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "preserved", Type:
iceberg.PrimitiveTypes.String},
+ iceberg.NestedField{ID: 2, Name: "overridden", Type:
iceberg.PrimitiveTypes.String, Doc: "iceberg comment"},
+ iceberg.NestedField{ID: 3, Name: "missing", Type:
iceberg.PrimitiveTypes.String},
+ )
+ metadata, err := table.NewMetadata(schema, nil, table.SortOrder{},
"s3://example/path", nil)
+ assert.NoError(t, err)
Review Comment:
`assert.NoError` keeps going on failure, and `NewMetadata` returns a nil
`Metadata` interface on error — so a failure here turns into a nil-deref panic
in `CurrentSchema()` instead of a clean assertion failure. I'd use
`require.NoError`. The pre-existing `TestSchemasToGlueColumns` above has the
same pattern, worth fixing while we're here.
##########
catalog/glue/schema.go:
##########
@@ -49,6 +49,27 @@ func schemasToGlueColumns(metadata table.Metadata)
[]types.Column {
}
}
+ existingComments := make(map[string]*string)
+ for _, column := range existingColumns {
+ name := aws.ToString(column.Name)
+ if column.Comment == nil {
+ continue
+ }
+ if _, ok := existingComments[name]; !ok {
+ existingComments[name] = column.Comment
Review Comment:
Small thing: this aliases the `*string` straight from `previousGlueTable`
into the output column, so the returned `TableInput` shares memory with the
previous table. Strings are immutable so it's harmless today, but I'd copy it
(`aws.String(aws.ToString(column.Comment))`) to keep the output independent of
the previous table's lifetime.
##########
catalog/glue/schema.go:
##########
@@ -49,6 +49,27 @@ func schemasToGlueColumns(metadata table.Metadata)
[]types.Column {
}
}
+ existingComments := make(map[string]*string)
+ for _, column := range existingColumns {
+ name := aws.ToString(column.Name)
Review Comment:
Keying on column name means a rename or drop+re-add carries a stale comment
onto a semantically different field — rename field ID=5 `address`, add a new
field ID=7 also named `address`, and the new one inherits the old comment on
the next commit.
Both sides already carry `iceberg.field.id` in `Parameters`, so I'd key
`existingComments` off that instead of `Name`. It's strictly more correct and
it also removes the duplicate-name ambiguity for free. Java's converter matches
on name too so this isn't a regression, but I'd rather not inherit the
limitation. wdyt?
--
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]
