SQL Catalog: Filter on iceberg_type in commit_table (#3337) - #3846
SQL Catalog: Filter on iceberg_type in commit_table (#3337)#3846hedger9487 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR fixes SqlCatalog.commit_table so it consistently filters SQL catalog rows by iceberg_type (matching TABLE or NULL), preventing accidental operations on VIEW rows created by other Iceberg implementations.
Changes:
- Apply self._iceberg_type_filter() to the commit_table UPDATE statement (rowcount-based path) and the SELECT ... FOR UPDATE query (fallback path) in pyiceberg/catalog/sql.py.
- Add a regression unit test ensuring commits fail (and do not modify the row) when the underlying row has iceberg_type='VIEW'.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyiceberg/catalog/sql.py | Adds the iceberg_type predicate to commit_table’s update/locking queries to avoid touching VIEW rows. |
| tests/catalog/test_sql.py | Adds a unit test covering the commit_table behavior when the backing row is tampered into a VIEW. |
tests/catalog/test_sql.py:413
- The assertion query filters only on table_name, which can become ambiguous if additional rows with the same name exist (e.g., different namespaces). Include catalog_name and table_namespace in the WHERE clause to ensure the test checks the intended row.
with catalog.engine.connect() as conn:
row = conn.execute(text("SELECT iceberg_type FROM iceberg_tables WHERE table_name = 'a_view'")).fetchone()
assert row is not None
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
| # Tamper the table row into a VIEW (simulating external writer) | ||
| with catalog.engine.connect() as conn: | ||
| conn.execute(text("UPDATE iceberg_tables SET iceberg_type = 'VIEW' WHERE table_name = 'a_view'")) | ||
| conn.commit() |
| assert row[0] == "VIEW" | ||
|
|
||
|
|
||
| def test_commit_table_ignores_view_rows(warehouse: Path) -> None: |
There was a problem hiding this comment.
I don't think this is a valid regression test. It passes even if I revert sql.py's change.
Sorry, something went wrong.
…pe_filter regression
|
Good catch! @ebyhr The previous test was failing early in load_table (which already had the type filter applied during requirement checking), before ever reaching the SQL UPDATE statement. I have updated test_commit_table_ignores_view_rows to simulate the concurrent modification race condition (where load_table succeeded prior to the row becoming a VIEW). Now, if the type_filter in commit_table's SQL UPDATE is reverted, the test fails with DID NOT RAISE CommitFailedException as expected. Thanks for pointing this out! |
Sorry, something went wrong.
Reviewers
+1 more reviewerAt least 1 approving review is required to merge this pull request.
Description
Fixes #3337.
In SqlCatalog, _iceberg_type_filter() was added in #3263 to filter on iceberg_type (matching TABLE or NULL) and avoid operating on VIEW rows written by other Iceberg implementations (e.g., Java or Rust). While load_table, drop_table, rename_table, and list_tables incorporate this filter, commit_table omitted type_filter in its SQL update statement.
This PR applies type_filter = self._iceberg_type_filter() to the commit_table update query in pyiceberg/catalog/sql.py.
Testing