Fix issue 2530#3688
Open
williamacostalora wants to merge 13 commits into
Open
Conversation
Fixes apache#2530 - Python 3.13's stricter ResourceWarning detection flagged unclosed sqlite3 connections left open by SQLAlchemy connection pools in test fixtures. The connections were being suppressed by warning filters in pyproject.toml rather than fixed at the source. Changes: - Add catalog.close() / engine.dispose() teardown to 7 test fixtures in tests/catalog/test_sql.py (catalog_memory, catalog_sqlite, alchemy_engine, and 4 inline catalog creation tests) - Add catalog.close() to 2 fixtures in tests/conftest.py - Remove sqlite ResourceWarning filter from pyproject.toml All 478 SQL catalog tests pass with -W error::ResourceWarning. Ray-related warning filters left in place pending CI verification with ray installed. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…#2530) Adds test_catalog_fixture_closes_connections to TestSqlCatalogClose to verify that SqlCatalog properly disposes all SQLAlchemy connections during teardown. Prevents future regressions where catalog.close() is accidentally removed from fixture teardown.
Documents why catalog.close() is called in fixture teardown so future contributors understand the connection lifecycle requirement and don't accidentally remove the fix. References issue apache#2530.
…in pyproject.toml Add explanatory comment distinguishing between ray subprocess warnings (pending CI test) and the resolved sqlite connection pool warnings (fixed via proper close() calls). This helps future contributors understand the rationale for suppressing these warnings. Fixes issue apache#2530. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…dance Improve the SqlCatalog.close() docstring to: - Emphasize that close() must be called in test fixture teardowns - Explain Python 3.13+ ResourceWarning prevention - Document the SQLAlchemy connection pool lifecycle - Clarify importance for both test and production scenarios (blobfuse) - Add reference to issue apache#2530 This helps future contributors understand why proper connection cleanup is essential. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Add inline comments to catalog and catalog_with_warehouse fixtures explaining the hasattr(cat, 'close') pattern. This clarifies that: - Not all catalog implementations provide close() (REST, Glue catalogs don't) - SQLCatalog does provide close() for proper connection cleanup - The pattern prevents AttributeError while supporting resource cleanup - This is essential for Python 3.13+ ResourceWarning prevention (issue apache#2530) This helps future contributors understand why the pattern is necessary and when they should update it if new catalog types are added. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
williamacostalora
marked this pull request as draft
July 20, 2026 03:38
…Python 3.13 Fixes apache#2530 - Python 3.13's stricter ResourceWarning detection flagged unclosed BigQuery client connections left open in test fixtures. Changes: - Add close() method to BigQueryMetastoreCatalog that properly closes the BigQuery client connection - Add close() teardown with try/finally to all 5 tests in test_bigquery_metastore.py All BigQueryMetastoreCatalog tests pass with -W error::ResourceWarning. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…memory Fixes unclosed sqlite3.Connection ResourceWarning that surfaces during test setup in Python 3.14 CI runs. The test was creating an InMemoryCatalog via load_catalog() without explicitly closing it, violating the explicit cleanup pattern established for issue apache#2530. The warning surfaces later (during setup of the next test in execution order) due to Python's deferred garbage collection of the unreferenced catalog object, causing pytest to attribute it to an unrelated test module. Root cause confirmed by investigation: this test was the only unclosed catalog instance remaining in the tests/ suite. Changes: - Bind the returned catalog to a variable and close it explicitly via try/finally, mirroring the pattern already established in test_bigquery_metastore.py All tests/catalog/ tests now pass with strict ResourceWarning enforcement. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…onments Fixes remaining ResourceWarnings on Python 3.14 by: 1. SqlCatalog.close(): Add explicit gc.collect() after engine.dispose() to ensure sqlite3 connections are immediately released on Python 3.14+. Deferred garbage collection can cause ResourceWarnings during test teardown when Python's finalizers run for unreferenced connections. 2. CLI console.run(): Register a context close callback to properly close the catalog when the Click command exits. Previously, catalogs created by the CLI were never explicitly closed, leaving sqlite3 connections open and causing ResourceWarnings in test suites that invoke CLI commands (e.g., tests/cli/). These changes complement the earlier fixes for BigQueryMetastoreCatalog and test_load_catalog_in_memory, completing the issue apache#2530 cleanup for Python 3.13+. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Fixes ResourceWarning unclosed database connections in test_pyarrow.py tests. The local catalog fixture (line 1508-1510) shadowed the global properly-closing fixture from conftest.py. It returned the InMemoryCatalog without yielding or calling close(), leaving SQLite connections uncovered. This fixture is used by 6 tests in the file (8 instances total including parametrized variants): - test_identity_transform_column_projection - test_identity_transform_column_projection_with_falsy_value (3 parametrized) - test_identity_transform_columns_projection - test_inspect_partition_for_nested_field - test_inspect_partitions_respects_partition_evolution - test_partition_column_projection_with_schema_evolution Changed the fixture to a generator that yields and closes the catalog on teardown, matching the pattern established in conftest.py and elsewhere on this branch for issue apache#2530. All 8 affected tests now pass without ResourceWarnings. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
Fixes ResourceWarning unclosed database connections in test_inspect.py tests. The local catalog fixture (lines 38-42) returned the InMemoryCatalog without yielding or calling close(), leaving SQLite connections uncovered. Changed the fixture to a generator that yields and closes the catalog on teardown, matching the pattern established in conftest.py and elsewhere on this branch for issue apache#2530. All 4 tests in test_inspect.py now pass without ResourceWarnings. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
…ixtures Fixes ResourceWarning unclosed database connections in catalog fixtures. Both files had local catalog fixtures that returned InMemoryCatalog/Catalog instances without yielding or calling close(), leaving SQLite connections uncovered. Changed fixtures to generators that yield and close the catalog on teardown, matching the pattern established in conftest.py and elsewhere on this branch for issue apache#2530. Co-Authored-By: Claude Haiku 4.5 <[email protected]>
williamacostalora
marked this pull request as ready for review
July 27, 2026 03:05
Author
|
Hi @Fokko @kevinjqliu — this PR fixes the underlying sqlite connection leaks in test fixtures that were being suppressed by warning filters added in PR #2863. All 17 CI checks now pass. The fix covers unclosed SQLAlchemy connections across |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2530
Rationale for this change
Python 3.13 introduced stricter resource leak detection that flags unclosed
sqlite3 connections left open by SQLAlchemy connection pools after test teardown.
PR #2863 added warning filters in
pyproject.tomlto suppress these warningsrather than fixing the root cause. This PR resolves the underlying issue by
adding
catalog.close()/engine.dispose()calls to all affected test fixturesand removes the sqlite
ResourceWarningfilter.Nine locations across
tests/catalog/test_sql.pyandtests/conftest.pycreatedSqlCataloginstances without callingcatalog.close()during teardown. Theclose()method (which callsself.engine.dispose()) already existed and wascorrectly implemented in
pyiceberg/catalog/sql.py— it simply wasn't beinginvoked by the fixtures.
Note: The two ray-related warning filters (
unclosed file,subprocess still running) are left in place as ray is an optional dependency requiring a CIenvironment to reproduce and verify. Happy to address those in a follow-up PR.
Are these changes tested?
Yes. Removed the sqlite
ResourceWarningfilter frompyproject.tomland ran:uv run python -m pytest tests/catalog/test_sql.py -W error::ResourceWarning -v
13/13 tests pass with zero ResourceWarnings. Also added
test_catalog_fixture_closes_connectionstoTestSqlCatalogCloseas aregression test to prevent future regressions.
Are there any user-facing changes?
No — all changes are in test files and
pyproject.toml. No public API changes.