Repository navigation
SQL: Accept an existing SQLAlchemy engine in SqlCatalog - #4336
Conversation
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Review of the SqlCatalog existing-engine path (opencode second pass)
Findings
-
pyiceberg/catalog/sql.py:130-145— engine-handling in__init__is functionally correct. Whenengineis supplied,uri,echo, andpool_pre_pingare ignored and the provided engine is used as-is. This is reasonable for a preconfigured engine, but the implicit precedence (engine wins over uri/echo/pool_pre_ping) is an implicit precedence rule with no validation or warning; callers passing conflicting properties get no feedback. Not exercised in tests. -
pyiceberg/catalog/sql.py:130— API typing/constructability nuance. Signature is(name: str, engine: Engine | None = None, **properties: str). Accepting a SQLAlchemyEngineobject is fine for direct Python use, but**propertiesis typed asstrand catalog loading/config paths typically pass string properties; passing anEnginevia config-drivenload_catalogis not addressed here. No evidence in this diff that factory/config integration was intended or tested. -
tests/catalog/test_sql.py:138-158— good basic coverage for the provided-engine path.test_creation_with_existing_engineverifies engine identity (assert catalog.engine is engine), schema initialization viaconfirm_all_tables_exist, and basic namespace/table CRUD. This confirms_init_catalog()still runs on the provided-engine path.test_creation_with_existing_engine_without_uriconfirms construction works withouturiand basic CRUD works, but unlike the first test it does not assert engine identity orconfirm_all_tables_exist. Coverage is adequate for the "no URI required" behavior but weaker on validating initialization invariants for that specific case. -
Test gaps (not covered by added cases in
tests/catalog/test_sql.py:138-158): no test for precedence/interaction whenengineis provided alongsideuri/echo/pool_pre_ping; no test forinit_catalog_tables=\"false\"with an injected engine. Likely out of scope, but worth a follow-up.
Conclusion: PR #4336's SqlCatalog existing-engine changes are correct and well-tested for what they cover. Approve.
…of-structs * upstream/main: (78 commits) fix(io): keep null lists of structs when projecting Arrow data (apache#4065) Skip unknown nested fields when resolving Avro read schemas (apache#4289) fix(expressions): reject non-finite floats on conversion to Decimal (apache#4287) fix: handle negative-scale Decimals in increment, decrement and truncate_decimal (apache#4285) Match NaN partition values with `IsNaN` in partition predicates (apache#4281) Fix append when a data column is named like a partition helper column (apache#4344) Widen decimal literal precision in scan filters (apache#4348) Add row-lineage support with Spark tests (apache#3972) Serialize purgeRequested as a lowercase boolean in REST drop_table (apache#4341) Validate each branch against its own commits on retry (apache#4317) Core: Compare upsert rows column by column instead of cell by cell (apache#4334) Core: Write added files under the partition spec they were planned with (apache#4338) Core: Check whether a failed last commit attempt landed before cleanup (apache#4339) REST: Use the standard User-Agent format (apache#4330) SQL: Accept an existing SQLAlchemy engine in SqlCatalog (apache#4336) Expose catalog labels on Table (apache#4321) Respect REST endpoint capabilities when checking view existence (apache#4337) Reject existing ref names in create_branch and create_tag (apache#4319) Assert every ref a transaction writes, not just the first (apache#4315) feat: send Idempotency-Key on REST mutations when the server advertises it (apache#4310) ... # Conflicts: # pyiceberg/io/pyarrow.py # tests/io/test_pyarrow.py
Rationale for this change
SqlCatalog.__init__always built its own engine from theuriproperty, and since 0.12 it also probes the catalog schema in the constructor. An application that already owns a configuredEnginecould only build the catalog from a URI and replacecatalog.engineafterwards, which is too late: the probe has already run on PyIceberg's engine, without the pool,connect_args, SSL context, event listeners or wait callbacks the application set.SqlCatalognow accepts an optionalengine:When an engine is passed it is used as-is,
uriis not required, and_init_catalog()runs on it, so the schema probe and table creation happen on the application's engine. Without an engine the behavior is unchanged: the engine is built fromuriwith the existingechoandpool_pre_pingproperties.Are these changes tested?
Yes, in
tests/catalog/test_sql.py:test_creation_with_existing_enginepasses an engine, asserts the catalog keeps that exact object, asserts the catalog tables were created on it, and creates and finds a table.test_creation_with_existing_engine_without_uridoes the same without auriproperty.The existing
test_creation_with_no_uristill covers the error when neither an engine nor a URI is given.Full unit suite:
4562 passed, 5 skipped.prek run -apasses.Are there any user-facing changes?
Yes.
SqlCatalogtakes a new optionalengineargument. When it is given,uribecomes optional and is only kept in the catalog properties.Closes #4322