laskoviymishka commented on code in PR #1486:
URL: https://github.com/apache/iceberg-go/pull/1486#discussion_r3610624123
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
Review Comment:
nice that this reads `existing.Parameters` and not `updated.Parameters` — if
`newProps` ever carried `metadata_location`, reading the mutated map would
capture the wrong previous location. It's subtle enough that a one-line comment
on why would save the next person from "simplifying" it back.
##########
catalog/hive/hive.go:
##########
@@ -532,7 +532,14 @@ func (c *Catalog) CommitTable(ctx context.Context,
identifier table.Identifier,
}
if current != nil {
- updatedHiveTbl := updateHiveTableForCommit(currentHiveTbl,
staged.MetadataLocation())
+ updatedHiveTbl := updateHiveTableForCommit(
+ currentHiveTbl,
+ current.Properties(),
+ staged.Properties(),
Review Comment:
six positional args with two adjacent `iceberg.Properties` (`currentProps`,
`newProps`) is easy to transpose silently — swap them and the diff logic
quietly does the wrong thing.
Since there's a single call site, passing the current/staged `Metadata` (or
a small delta struct) instead of deconstructed fields would make the swap
impossible. Not blocking, but worth doing while it's fresh.
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
Review Comment:
this assumes the HMS `Parameters` we're deleting are exactly the Iceberg
user-properties we last wrote — but old commits only ever stamped
`metadata_location`, so any property a table already had in HMS (or anything an
operator set directly that happens to collide with an Iceberg key name) gets
silently dropped on the first commit after this lands.
I'd at minimum document that invariant right here as a known limitation, or
gate the delete on an allow-list of Iceberg key prefixes so we're not reaching
into keys we didn't write. wdyt?
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
updated.Parameters[PreviousMetadataLocationKey] = oldLocation
}
- // Set new metadata location
+ updated.Parameters[TableTypeKey] = TableTypeIceberg
updated.Parameters[MetadataLocationKey] = newMetadataLocation
+ updated.Parameters[ExternalKey] = "TRUE"
+ updated.Parameters["storage_handler"] =
"org.apache.iceberg.mr.hive.HiveIcebergStorageHandler"
+
+ if existing.Sd == nil {
+ updated.Sd = constructHiveTable(
+ existing.DbName,
+ existing.TableName,
+ newLocation,
+ newMetadataLocation,
+ newSchema,
+ newProps,
+ ).Sd
+ } else {
+ storageDescriptor := *existing.Sd
+ storageDescriptor.Cols = schemaToHiveColumns(newSchema)
Review Comment:
`schemaToHiveColumns` calls `newSchema.Fields()` straight off the pointer,
so a nil `newSchema` panics here (and in the `constructHiveTable` branch
above). `staged.Schema()` is non-nil in practice so this is latent, but the
six-arg signature makes a nil mis-call easy. A nil guard or a documented
precondition would be enough.
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
updated.Parameters[PreviousMetadataLocationKey] = oldLocation
}
- // Set new metadata location
+ updated.Parameters[TableTypeKey] = TableTypeIceberg
Review Comment:
Java's `HMSTablePropertyHelper` translates `gc.enabled` →
`external.table.purge` when it syncs (`ICEBERG_TO_HMS_TRANSLATION`). We copy
`gc.enabled` through verbatim but never write `external.table.purge`, so Hive's
own `DROP TABLE` won't honour the GC intent — `gc.enabled=false` won't stop the
purge, and `gc.enabled=true` won't trigger it.
This was inert before because commits only touched `metadata_location`; now
that we sync every property on commit it's live. I'd mirror `gc.enabled` into
`external.table.purge` here, and match the same in `constructHiveTable` so both
paths agree.
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
updated.Parameters[PreviousMetadataLocationKey] = oldLocation
}
- // Set new metadata location
+ updated.Parameters[TableTypeKey] = TableTypeIceberg
updated.Parameters[MetadataLocationKey] = newMetadataLocation
+ updated.Parameters[ExternalKey] = "TRUE"
+ updated.Parameters["storage_handler"] =
"org.apache.iceberg.mr.hive.HiveIcebergStorageHandler"
Review Comment:
`storage_handler` and the handler class are string literals here and again
in `constructHiveTable` (and now the test) — every other reserved key has a
named constant in `options.go`. I'd pull `StorageHandlerKey` /
`IcebergStorageHandler` out and use them in all three spots.
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
updated.Parameters[PreviousMetadataLocationKey] = oldLocation
}
- // Set new metadata location
+ updated.Parameters[TableTypeKey] = TableTypeIceberg
updated.Parameters[MetadataLocationKey] = newMetadataLocation
+ updated.Parameters[ExternalKey] = "TRUE"
+ updated.Parameters["storage_handler"] =
"org.apache.iceberg.mr.hive.HiveIcebergStorageHandler"
+
+ if existing.Sd == nil {
+ updated.Sd = constructHiveTable(
+ existing.DbName,
+ existing.TableName,
+ newLocation,
+ newMetadataLocation,
+ newSchema,
+ newProps,
+ ).Sd
+ } else {
+ storageDescriptor := *existing.Sd
Review Comment:
`storageDescriptor := *existing.Sd` is a shallow copy — `SerdeInfo` (a
`*SerDeInfo`) and the SD-level `Parameters` map stay aliased to `existing.Sd`.
It's inert today since we only replace `Cols` and `Location` and discard
`existing` after `AlterTable`, but a real HMS response populates
`SerdeInfo.Parameters`, so this is a footgun waiting on the next edit.
I'd clone `SerdeInfo` (and its `Parameters` map) when it's non-nil,
consistent with the `maps.Clone` we already do for the table `Parameters`.
##########
catalog/hive/schema.go:
##########
@@ -160,22 +161,51 @@ func constructHiveTable(dbName, tableName, location,
metadataLocation string, sc
}
}
-func updateHiveTableForCommit(existing *hive_metastore.Table,
newMetadataLocation string) *hive_metastore.Table {
- // Copy the existing table
+func updateHiveTableForCommit(
+ existing *hive_metastore.Table,
+ currentProps, newProps iceberg.Properties,
+ newLocation string,
+ newSchema *iceberg.Schema,
+ newMetadataLocation string,
+) *hive_metastore.Table {
updated := *existing
-
- // Update parameters
+ updated.Parameters = maps.Clone(existing.Parameters)
if updated.Parameters == nil {
updated.Parameters = make(map[string]string)
}
- // Store previous metadata location
- if oldLocation, ok := updated.Parameters[MetadataLocationKey]; ok {
+ for key := range currentProps {
+ delete(updated.Parameters, key)
+ }
+ for key, value := range newProps {
+ updated.Parameters[key] = value
+ }
+ delete(updated.Parameters, PreviousMetadataLocationKey)
+
+ if oldLocation, ok := existing.Parameters[MetadataLocationKey]; ok {
updated.Parameters[PreviousMetadataLocationKey] = oldLocation
}
- // Set new metadata location
+ updated.Parameters[TableTypeKey] = TableTypeIceberg
updated.Parameters[MetadataLocationKey] = newMetadataLocation
+ updated.Parameters[ExternalKey] = "TRUE"
+ updated.Parameters["storage_handler"] =
"org.apache.iceberg.mr.hive.HiveIcebergStorageHandler"
+
+ if existing.Sd == nil {
+ updated.Sd = constructHiveTable(
Review Comment:
we call `constructHiveTable` here only to pull its `.Sd`, but `.Sd` is
derived entirely from location and schema — `newProps` doesn't affect it, so
passing it reads as if it does. A small `buildIcebergSD(location, schema)`
helper called from both spots would be clearer.
Also nothing covers this branch — `existing.Sd == nil` is never hit by the
test. wdyt about a case that constructs `existing` with a nil `Sd`?
##########
catalog/hive/hive_test.go:
##########
@@ -888,6 +888,62 @@ func TestConstructHiveTablePreservesReservedParameters(t
*testing.T) {
assert.Equal("gzip", hiveTbl.Parameters["write.metadata.compression"])
}
+func TestUpdateHiveTableForCommitSynchronizesMetadata(t *testing.T) {
Review Comment:
the test drives `updateHiveTableForCommit` directly, which is great for the
merge logic, but nothing exercises the real path through `CommitTable` — that
`current.Properties()` maps to `currentProps` and `staged.Properties()` to
`newProps`, and that `AlterTable` actually receives the merged table.
Given the arg list grew to six positional params, a mock `CommitTable` test
asserting the `AlterTable` payload has the new columns and merged params would
lock the wiring down. Right now a swapped-arg regression would sail through.
##########
catalog/hive/hive_test.go:
##########
@@ -888,6 +888,62 @@ func TestConstructHiveTablePreservesReservedParameters(t
*testing.T) {
assert.Equal("gzip", hiveTbl.Parameters["write.metadata.compression"])
}
+func TestUpdateHiveTableForCommitSynchronizesMetadata(t *testing.T) {
+ existing := constructHiveTable(
+ "test_database",
+ "test_table",
+ "s3://bucket/old-location",
+ "s3://bucket/old-location/metadata/v1.metadata.json",
+ testSchema,
+ iceberg.Properties{
+ "changed": "old",
+ "removed": "value",
+ },
+ )
+ existing.Parameters["hms-owned"] = "keep"
+ originalParameters := maps.Clone(existing.Parameters)
+ originalStorageDescriptor := *existing.Sd
+
+ newSchema := iceberg.NewSchema(
+ 1,
+ iceberg.NestedField{ID: 1, Name: "renamed", Type:
iceberg.PrimitiveTypes.String},
+ )
+ updated := updateHiveTableForCommit(
+ existing,
+ iceberg.Properties{"changed": "old", "removed": "value"},
+ iceberg.Properties{
+ "changed": "new",
+ "added": "value",
+ TableTypeKey: "HIVE",
+ MetadataLocationKey: "s3://wrong/metadata.json",
+ PreviousMetadataLocationKey: "s3://wrong/previous.json",
+ ExternalKey: "FALSE",
+ "storage_handler": "wrong.StorageHandler",
+ },
+ "s3://bucket/new-location",
+ newSchema,
+ "s3://bucket/new-location/metadata/v2.metadata.json",
+ )
+
+ require.Equal(t, "new", updated.Parameters["changed"])
+ require.Equal(t, "value", updated.Parameters["added"])
+ require.NotContains(t, updated.Parameters, "removed")
+ require.Equal(t, "keep", updated.Parameters["hms-owned"])
+ require.Equal(t, TableTypeIceberg, updated.Parameters[TableTypeKey])
+ require.Equal(t, "TRUE", updated.Parameters[ExternalKey])
+ require.Equal(t,
"org.apache.iceberg.mr.hive.HiveIcebergStorageHandler",
updated.Parameters["storage_handler"])
+ require.Equal(t, "s3://bucket/old-location/metadata/v1.metadata.json",
updated.Parameters[PreviousMetadataLocationKey])
+ require.Equal(t, "s3://bucket/new-location/metadata/v2.metadata.json",
updated.Parameters[MetadataLocationKey])
+ require.Equal(t, "s3://bucket/new-location", updated.Sd.Location)
+ require.Equal(t, schemaToHiveColumns(newSchema), updated.Sd.Cols)
+ require.Equal(t, originalStorageDescriptor.InputFormat,
updated.Sd.InputFormat)
+ require.Equal(t, originalStorageDescriptor.OutputFormat,
updated.Sd.OutputFormat)
+ require.Equal(t, originalStorageDescriptor.SerdeInfo,
updated.Sd.SerdeInfo)
Review Comment:
this passes whether `SerdeInfo` is a fresh copy or the same pointer —
`require.Equal` is a deep-equal, and here it's literally the same pointer, so
it doesn't prove the independence the copy is supposed to give us.
To actually assert it, mutate `updated.Sd.SerdeInfo.Parameters` after the
call and check `existing.Sd.SerdeInfo.Parameters` is untouched (or at least
assert the pointers differ). Pairs with the shallow-copy note on the SD.
--
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]
