Add flag to allow disabling creation of catalog tables #1155

isc-psulin · Sep 10, 2024

Remove unnecessary _ensure_tables_exist method as this is already the default behavior of Metadata.create_all()

… default behavior of Metadata.create_all()

kevinjqliu

Thanks for the PR! Do you mind adding a test to show that this works for when

tables already exist
some tables exist, but not all
none of the tables exist yet

Fokko · Sep 10, 2024

@isc-patrick do you know how it will check if the table exists? I'm asking since a CREATE TABLE IF NOT EXISTS ... might require CREATE TABLE permissions.

isc-psulin · Sep 11, 2024

I can certainly add tests, but that is really testing the Metadata.create_all() function in SQLAlchemy and not pyiceberg code. I think that CREATE TABLE IF NOT EXISTS requires same permissions as CREATE TABLE. How this is executed will be determined by the Dialect.

kevinjqliu · Sep 11, 2024

please do! we want to ensure that this change does not break new and existing DB integrations.

isc-psulin · Sep 11, 2024

It does not look like CREATE TABLE IF NOT EXISTS is what is used. It is specific to each dialect and how they implement the has_table method, but the ones I looked at use metadata on the schema to determine the existence of the table.

…lready exist

…eted

sungwy

Hi @isc-patrick - thank you very much for putting up this PR, and for leading the thorough discussion on #1148.

I have added some nits, and in addition could we:

update the PR description to fit the updated scope of work for this PR
add a line to the SqlCatalog property so that users will be able to find this property?

iceberg-python/mkdocs/docs/configuration.md

Lines 261 to 265 in 0dc5408

    
           | Key           | Example                                                      | Default | Description                                                                                                                                                                                    | 
        
           | ------------- | ------------------------------------------------------------ | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | 
        
           | uri           | postgresql+psycopg2://username:password@localhost/mydatabase |         | SQLAlchemy backend URL for the catalog database (see [documentation for URL format](https://docs.sqlalchemy.org/en/20/core/engines.html#backend-specific-urls))                                | 
        
           | echo          | true                                                         | false   | SQLAlchemy engine [echo param](https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.echo) to log all statements to the default log handler                      | 
        
           | pool_pre_ping | true                                                         | false   | SQLAlchemy engine [pool_pre_ping param](https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.pool_pre_ping) to test connections for liveness upon each checkout |

sungwy · Sep 16, 2024

tests/catalog/test_sql.py

    )


+def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:


Suggested change

def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:

def confirm_no_tables_exist(alchemy_engine: Engine) -> List[str]:

for python3.8 compatibility (which will be deprecated soon), but this is currently needed for our CI to pass

Not returning this anymore, so removed, CATALOG_TABLES is no a constant. PR title updated and flag added to docs

sungwy · Sep 16, 2024

tests/catalog/test_sql.py

+        if inspector.has_table(c.__tablename__):
+            c.__table__.drop(alchemy_engine)
+
+    catalog_tables = [c.__tablename__ for c in SqlCatalogBaseTable.__subclasses__()]


This function seems to serve two purposes:

confirm that no tables exist

return the list of SqlCatalogBaseTable.__subclasses__()

(2) is a static list of tables required for the SqlCatalog. Would it be cleaner to separate out (2) to a global variable in this module, instead of returning it and passing it as an input to confirm_all_tables_exist ?

Made it a constant

sungwy · Sep 16, 2024

tests/catalog/test_sql.py

+    return catalog_tables
+
+
+def confirm_all_tables_exist(inspector: Inspector, catalog_tables: list[str], catalog: Catalog) -> None:


It looks like we are passing the catalog here just to assert its type - is this necessary for this check? Was the intention to check the tables through the catalog's engine instead like:

inspect(catalog.engine).get_table_names()

Not passing the inspector anymore. Also needed to cast the Catalog to use the engine. Made the uri a fixture to insure using same source between catalog and inspector that was creating tables.

sungwy · Oct 18, 2024

Thank you again for working on this PR @isc-patrick !

* Remove unnecessary _ensure_tables_exist method as this is already the default behavior of Metadata.create_all() * Added tests for creating Catalog tables when no, some or all tables already exist * add init_catalog_tables flag to SQLCatalog * add _ensure_tables_exists back until postgres integration tests completed * fixed tests, added flag to docs

Remove unnecessary _ensure_tables_exist method as this is already the…

59206fb

… default behavior of Metadata.create_all()

kevinjqliu reviewed Sep 10, 2024

View reviewed changes

Added tests for creating Catalog tables when no, some or all tables a…

00efac1

…lready exist

sungwy mentioned this pull request Sep 15, 2024

Modify SQLCatalog initialization so that classes are not always created and update how these classes are created to be more open to tother DB's #1148

Closed

isc-psulin added 2 commits September 16, 2024 12:12

add init_catalog_tables flag to SQLCatalog

58f9e53

add _ensure_tables_exists back until postgres integration tests compl…

c045175

…eted

sungwy reviewed Sep 16, 2024

View reviewed changes

isc-psulin changed the title ~~Remove unnecessary _ensure_tables_exist method~~ Add flag to allow diabling creation of catalog tables Sep 17, 2024

fixed tests, added flag to docs

11d9f3a

isc-psulin changed the title ~~Add flag to allow diabling creation of catalog tables~~ Add flag to allow disabling creation of catalog tables Sep 17, 2024

sungwy approved these changes Oct 18, 2024

View reviewed changes

sungwy merged commit 1c50f53 into apache:main Oct 18, 2024

	\| Key \| Example \| Default \| Description \|
	\| ------------- \| ------------------------------------------------------------ \| ------- \| ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- \|
	\| uri \| postgresql+psycopg2://username:password@localhost/mydatabase \| \| SQLAlchemy backend URL for the catalog database (see [documentation for URL format](https://docs.sqlalchemy.org/en/20/core/engines.html#backend-specific-urls)) \|
	\| echo \| true \| false \| SQLAlchemy engine [echo param](https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.echo) to log all statements to the default log handler \|
	\| pool_pre_ping \| true \| false \| SQLAlchemy engine [pool_pre_ping param](https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.pool_pre_ping) to test connections for liveness upon each checkout \|

		)


		def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:

		return catalog_tables


		def confirm_all_tables_exist(inspector: Inspector, catalog_tables: list[str], catalog: Catalog) -> None:

Search code, repositories, users, issues, pull requests...

Add flag to allow disabling creation of catalog tables #1155

Add flag to allow disabling creation of catalog tables #1155

Uh oh!

Conversation

isc-psulin commented Sep 10, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko commented Sep 10, 2024

Uh oh!

isc-psulin commented Sep 11, 2024

Uh oh!

kevinjqliu commented Sep 11, 2024

Uh oh!

isc-psulin commented Sep 11, 2024

Uh oh!

sungwy left a comment

Choose a reason for hiding this comment

Uh oh!

sungwy Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

isc-psulin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

sungwy Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

isc-psulin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

sungwy Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

isc-psulin Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

sungwy commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants