Skip to content

Support register_table and unregister_table procedures in Iceberg JDBC catalog#15874

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-jdbc-unregister-table
Feb 14, 2023
Merged

Support register_table and unregister_table procedures in Iceberg JDBC catalog#15874
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-jdbc-unregister-table

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Jan 27, 2023

Description

Support register_table and unregister_table procedures in Iceberg JDBC catalog
Fixes #15853

Release notes

(x) Release notes are required, with the following suggested text:

# Iceberg
* Add support for `register_table` and `unregister_table` procedures in JDBC catalog. ({issue}`15853`)

@cla-bot cla-bot bot added the cla-signed label Jan 27, 2023
@ebyhr ebyhr self-assigned this Jan 27, 2023
@ebyhr ebyhr force-pushed the ebi/iceberg-jdbc-unregister-table branch from 2fca4f1 to 6436ac9 Compare February 14, 2023 00:06
@ebyhr ebyhr force-pushed the ebi/iceberg-jdbc-unregister-table branch from 6436ac9 to 6664d0e Compare February 14, 2023 07:11
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd rather avoid using static imports for these properties. Their names are rather generic and this can turn out to be error prone in the maintenance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CATALOG_IMPL comes from CatalogProperties class. I don't feel qualified access improves anything.

@ebyhr ebyhr requested review from electrum and phd3 February 14, 2023 07:38
Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add the reason behind not using the jdbc catalogs' register table (seems like Cannot invoke "org.apache.iceberg.util.SerializableSupplier.get()" because "this.hadoopConf" is null) so that someone doesn't try to "optimize" this by removing jdbcClient object that's being used for this specific purpose? Or when the underlying code gets fixed - we can update this.

@ebyhr ebyhr force-pushed the ebi/iceberg-jdbc-unregister-table branch from 6664d0e to 6d00587 Compare February 14, 2023 22:16
@ebyhr ebyhr merged commit ead3432 into trinodb:master Feb 14, 2023
@ebyhr ebyhr deleted the ebi/iceberg-jdbc-unregister-table branch February 14, 2023 22:33
@ebyhr ebyhr mentioned this pull request Feb 14, 2023
@github-actions github-actions bot added this to the 407 milestone Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Add support for register_table and unregister_table procedures in Iceberg JDBC catalog

5 participants