You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dttung2905 (via GitHub)" <gi...@apache.org> on 2025/04/13 20:59:48 UTC
[PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
dttung2905 opened a new pull request, #395:
URL: https://github.com/apache/iceberg-go/pull/395
I'm adding the functionality to set table properties in AWS Glue catalog
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 closed pull request #395: feat(catalog/glue): Add SetTableProperties for glue catalog
URL: https://github.com/apache/iceberg-go/pull/395
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #395:
URL: https://github.com/apache/iceberg-go/pull/395#discussion_r2069456731
##########
catalog/glue/glue.go:
##########
@@ -330,6 +331,53 @@ func (c *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier
return c.LoadTable(ctx, identifier, nil)
}
+// SetTableProperties updates the properties of an existing Iceberg table.
+// The properties will be merged with existing properties. If a property key already exists,
+// its value will be overwritten.
+func (c *Catalog) SetTableProperties(ctx context.Context, identifier table.Identifier, props iceberg.Properties) error {
+ database, tableName, err := identifierToGlueTable(identifier)
+ if err != nil {
+ return err
+ }
+
+ existingTable, err := c.getTable(ctx, database, tableName)
+ if err != nil {
+ return err
+ }
+
+ updatedParameters := map[string]string{}
+ for k, v := range existingTable.Parameters {
+ updatedParameters[k] = v
+ }
+ for k, v := range props {
+ updatedParameters[k] = v
+ }
Review Comment:
Thanks @lliangyu-lin for the PR. I see it has been merged already. I will close this PR then :pray: Thanks alot for the contribution
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #395:
URL: https://github.com/apache/iceberg-go/pull/395#discussion_r2054570747
##########
catalog/glue/glue.go:
##########
@@ -330,6 +331,53 @@ func (c *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier
return c.LoadTable(ctx, identifier, nil)
}
+// SetTableProperties updates the properties of an existing Iceberg table.
+// The properties will be merged with existing properties. If a property key already exists,
+// its value will be overwritten.
+func (c *Catalog) SetTableProperties(ctx context.Context, identifier table.Identifier, props iceberg.Properties) error {
+ database, tableName, err := identifierToGlueTable(identifier)
+ if err != nil {
+ return err
+ }
+
+ existingTable, err := c.getTable(ctx, database, tableName)
+ if err != nil {
+ return err
+ }
+
+ updatedParameters := map[string]string{}
+ for k, v := range existingTable.Parameters {
+ updatedParameters[k] = v
+ }
+ for k, v := range props {
+ updatedParameters[k] = v
+ }
Review Comment:
Thank you for working on this! I have a general question on how does unsetting the property works? Seems like right now the logic is only adding new properties or overriding existing properties.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on code in PR #395:
URL: https://github.com/apache/iceberg-go/pull/395#discussion_r2059393092
##########
catalog/glue/glue.go:
##########
@@ -330,6 +331,53 @@ func (c *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier
return c.LoadTable(ctx, identifier, nil)
}
+// SetTableProperties updates the properties of an existing Iceberg table.
+// The properties will be merged with existing properties. If a property key already exists,
+// its value will be overwritten.
+func (c *Catalog) SetTableProperties(ctx context.Context, identifier table.Identifier, props iceberg.Properties) error {
+ database, tableName, err := identifierToGlueTable(identifier)
+ if err != nil {
+ return err
+ }
+
+ existingTable, err := c.getTable(ctx, database, tableName)
+ if err != nil {
+ return err
+ }
+
+ updatedParameters := map[string]string{}
+ for k, v := range existingTable.Parameters {
+ updatedParameters[k] = v
+ }
+ for k, v := range props {
+ updatedParameters[k] = v
+ }
Review Comment:
I did some checks with your change locally. I think only updating the table properties on Glue catalog might not be enough. When I try loading the updated table, the properties in the table metadata is not updated. This is because glue `LoadTable` is [reading the metadata file](https://github.com/apache/iceberg-go/blob/main/catalog/glue/glue.go#L219-L222), but Glue update table API call doesn't create a new metadata file.
I raised this PR https://github.com/apache/iceberg-go/pull/403 on using the CommitTable which directly hooked into the CLI and can directly use the set and unset properties. https://github.com/apache/iceberg-go/blob/main/table/updates.go#L36-L45. Feel free to take a look.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #395:
URL: https://github.com/apache/iceberg-go/pull/395#discussion_r2054848344
##########
catalog/glue/glue.go:
##########
@@ -330,6 +331,53 @@ func (c *Catalog) RegisterTable(ctx context.Context, identifier table.Identifier
return c.LoadTable(ctx, identifier, nil)
}
+// SetTableProperties updates the properties of an existing Iceberg table.
+// The properties will be merged with existing properties. If a property key already exists,
+// its value will be overwritten.
+func (c *Catalog) SetTableProperties(ctx context.Context, identifier table.Identifier, props iceberg.Properties) error {
+ database, tableName, err := identifierToGlueTable(identifier)
+ if err != nil {
+ return err
+ }
+
+ existingTable, err := c.getTable(ctx, database, tableName)
+ if err != nil {
+ return err
+ }
+
+ updatedParameters := map[string]string{}
+ for k, v := range existingTable.Parameters {
+ updatedParameters[k] = v
+ }
+ for k, v := range props {
+ updatedParameters[k] = v
+ }
Review Comment:
Hi @lliangyu-lin, thank you for the quick review. its a very good question and you are right, the current `SetTableProperties` method only does the `set` or overwrite existing key and not the `unsett`. I was thinking about having another `UnSetTableProperties` for such purposes. 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(catalog/glue): Add SetTableProperties for glue catalog [iceberg-go]
Posted by "lliangyu-lin (via GitHub)" <gi...@apache.org>.
lliangyu-lin commented on PR #395:
URL: https://github.com/apache/iceberg-go/pull/395#issuecomment-2822029991
Thank you for working on this! I have a general question on how does unsetting the property works? Seems like right now the logic is only adding new properties or overriding existing properties.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org