Skip to content

SQL Catalog: Filter on iceberg_type in commit_table (#3337) - #3846

Open
hedger9487 wants to merge 1 commit into
apache:mainfrom
hedger9487:fix/sql-catalog-filter-iceberg-type-3337
Open

SQL Catalog: Filter on iceberg_type in commit_table (#3337)#3846
hedger9487 wants to merge 1 commit into
apache:mainfrom
hedger9487:fix/sql-catalog-filter-iceberg-type-3337

Conversation

@hedger9487

Copy link
Copy Markdown

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

  • Added unit test test_commit_table_ignores_view_rows in tests/catalog/test_sql.py.
  • All 31 catalog unit tests pass locally.

Copilot AI lite review requested due to automatic review settings August 25, 2026 03:20

Copilot AI left a comment

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.

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.
Suppressed comments (1)

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.

Comment thread tests/catalog/test_sql.py
Comment on lines +400 to +403
# 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()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SqlCatalog table operations should filter on iceberg_type

2 participants