This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3956-0d584073f67948c46f34d4c2a1672455bc10b5c9 in repository https://gitbox.apache.org/repos/asf/iceberg-python.git
commit a46cbdf09a77bf893d05c3e49caf6b72b3c24d57 Author: Yuang Gao <[email protected]> AuthorDate: Thu Sep 24 19:14:24 2026 +0000 fix: rank catalog configuration above table properties when constructing FileIO (#3956) * Rank catalog configuration above table properties when constructing FileIO * Cover FileIO property precedence in StaticTable.from_metadata --- pyiceberg/catalog/__init__.py | 15 +++++++++++---- pyiceberg/catalog/bigquery_metastore.py | 2 +- pyiceberg/catalog/dynamodb.py | 2 +- pyiceberg/catalog/glue.py | 6 +++--- pyiceberg/catalog/hive.py | 2 +- pyiceberg/catalog/rest/__init__.py | 15 +++++++++++---- pyiceberg/catalog/sql.py | 2 +- pyiceberg/table/__init__.py | 2 +- tests/catalog/test_rest.py | 28 +++++++++++++++++++++++++++- tests/catalog/test_sql.py | 20 ++++++++++++++++++++ tests/table/test_init.py | 6 ++++++ 11 files changed, 83 insertions(+), 17 deletions(-) diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py index 8de113404..0842f51cc 100644 --- a/pyiceberg/catalog/__init__.py +++ b/pyiceberg/catalog/__init__.py @@ -833,8 +833,15 @@ class Catalog(ABC): return tuple_identifier[0], tuple_identifier[1] - def _load_file_io(self, properties: Properties = EMPTY_DICT, location: str | None = None) -> FileIO: - return load_file_io({**self.properties, **properties}, location) + def _load_file_io( + self, + properties: Properties = EMPTY_DICT, + location: str | None = None, + table_properties: Properties = EMPTY_DICT, + ) -> FileIO: + # table_properties ranks lowest: a principal who can commit to a table must not + # be able to redirect its readers. + return load_file_io({**table_properties, **self.properties, **properties}, location) @staticmethod def _convert_schema_if_needed( @@ -1020,7 +1027,7 @@ class MetastoreCatalog(Catalog, ABC): metadata = new_table_metadata( location=location, schema=schema, partition_spec=partition_spec, sort_order=sort_order, properties=properties ) - io = self._load_file_io(properties=properties, location=metadata_location) + io = self._load_file_io(location=metadata_location, table_properties=properties) return StagedTable( identifier=(database_name, table_name), metadata=metadata, @@ -1054,7 +1061,7 @@ class MetastoreCatalog(Catalog, ABC): identifier=table_identifier, metadata=updated_metadata, metadata_location=new_metadata_location, - io=self._load_file_io(properties=updated_metadata.properties, location=new_metadata_location), + io=self._load_file_io(location=new_metadata_location, table_properties=updated_metadata.properties), catalog=self, ) diff --git a/pyiceberg/catalog/bigquery_metastore.py b/pyiceberg/catalog/bigquery_metastore.py index cc84b0b42..1267233e9 100644 --- a/pyiceberg/catalog/bigquery_metastore.py +++ b/pyiceberg/catalog/bigquery_metastore.py @@ -404,7 +404,7 @@ class BigQueryMetastoreCatalog(MetastoreCatalog): identifier=(dataset_name, table_name), metadata=metadata, metadata_location=metadata_location, - io=self._load_file_io(metadata.properties, metadata_location), + io=self._load_file_io(location=metadata_location, table_properties=metadata.properties), catalog=self, ) diff --git a/pyiceberg/catalog/dynamodb.py b/pyiceberg/catalog/dynamodb.py index 10f74fb9a..b4111a6fb 100644 --- a/pyiceberg/catalog/dynamodb.py +++ b/pyiceberg/catalog/dynamodb.py @@ -697,7 +697,7 @@ class DynamoDbCatalog(MetastoreCatalog): identifier=(database_name, table_name), metadata=metadata, metadata_location=metadata_location, - io=self._load_file_io(metadata.properties, metadata_location), + io=self._load_file_io(location=metadata_location, table_properties=metadata.properties), catalog=self, ) diff --git a/pyiceberg/catalog/glue.py b/pyiceberg/catalog/glue.py index 12b36efc5..977876918 100644 --- a/pyiceberg/catalog/glue.py +++ b/pyiceberg/catalog/glue.py @@ -389,7 +389,7 @@ class GlueCatalog(MetastoreCatalog): identifier=(database_name, table_name), metadata=metadata, metadata_location=metadata_location, - io=self._load_file_io(metadata.properties, metadata_location), + io=self._load_file_io(location=metadata_location, table_properties=metadata.properties), catalog=self, ) @@ -535,7 +535,7 @@ class GlueCatalog(MetastoreCatalog): identifier=self.identifier_to_tuple(identifier), metadata=staged_table.metadata, metadata_location=staged_table.metadata_location, - io=self._load_file_io(staged_table.metadata.properties, staged_table.metadata_location), + io=self._load_file_io(location=staged_table.metadata_location, table_properties=staged_table.metadata.properties), catalog=self, ) @@ -599,7 +599,7 @@ class GlueCatalog(MetastoreCatalog): identifier=self.identifier_to_tuple(identifier), metadata=staged_table.metadata, metadata_location=staged_table.metadata_location, - io=self._load_file_io(staged_table.metadata.properties, staged_table.metadata_location), + io=self._load_file_io(location=staged_table.metadata_location, table_properties=staged_table.metadata.properties), catalog=self, ) diff --git a/pyiceberg/catalog/hive.py b/pyiceberg/catalog/hive.py index ed6d3c6fd..a44da81b6 100644 --- a/pyiceberg/catalog/hive.py +++ b/pyiceberg/catalog/hive.py @@ -353,7 +353,7 @@ class HiveCatalog(MetastoreCatalog): identifier=(table.dbName, table.tableName), metadata=metadata, metadata_location=metadata_location, - io=self._load_file_io(metadata.properties, metadata_location), + io=self._load_file_io(location=metadata_location, table_properties=metadata.properties), catalog=self, ) diff --git a/pyiceberg/catalog/rest/__init__.py b/pyiceberg/catalog/rest/__init__.py index d8f58773e..cb20d4ddc 100644 --- a/pyiceberg/catalog/rest/__init__.py +++ b/pyiceberg/catalog/rest/__init__.py @@ -638,8 +638,13 @@ class RestCatalog(Catalog): return best_match.config if best_match else {} - def _load_file_io(self, properties: Properties = EMPTY_DICT, location: str | None = None) -> FileIO: - merged_properties = {**self.properties, **properties} + def _load_file_io( + self, + properties: Properties = EMPTY_DICT, + location: str | None = None, + table_properties: Properties = EMPTY_DICT, + ) -> FileIO: + merged_properties = {**table_properties, **self.properties, **properties} if self._auth_manager: merged_properties[AUTH_MANAGER] = self._auth_manager return load_file_io(merged_properties, location) @@ -1152,8 +1157,9 @@ class RestCatalog(Catalog): metadata_location=table_response.metadata_location, # type: ignore metadata=table_response.metadata, io=self._load_file_io( - {**table_response.metadata.properties, **table_response.config, **credential_config}, + {**table_response.config, **credential_config}, table_response.metadata_location, + table_properties=table_response.metadata.properties, ), catalog=self, config=table_response.config, @@ -1169,8 +1175,9 @@ class RestCatalog(Catalog): metadata_location=table_response.metadata_location, # type: ignore metadata=table_response.metadata, io=self._load_file_io( - {**table_response.metadata.properties, **table_response.config, **credential_config}, + {**table_response.config, **credential_config}, table_response.metadata_location, + table_properties=table_response.metadata.properties, ), catalog=self, ) diff --git a/pyiceberg/catalog/sql.py b/pyiceberg/catalog/sql.py index abe31194b..13e67997d 100644 --- a/pyiceberg/catalog/sql.py +++ b/pyiceberg/catalog/sql.py @@ -237,7 +237,7 @@ class SqlCatalog(MetastoreCatalog): identifier=Catalog.identifier_to_tuple(table_namespace) + (table_name,), metadata=metadata, metadata_location=metadata_location, - io=self._load_file_io(metadata.properties, metadata_location), + io=self._load_file_io(location=metadata_location, table_properties=metadata.properties), catalog=self, ) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index ac67e3187..0caba0e5d 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -1995,7 +1995,7 @@ class StaticTable(Table): identifier=("static-table", metadata_location), metadata_location=metadata_location, metadata=metadata, - io=load_file_io({**properties, **metadata.properties}, location=metadata_location), + io=load_file_io({**metadata.properties, **properties}, location=metadata_location), catalog=NoopCatalog("static-table"), ) diff --git a/tests/catalog/test_rest.py b/tests/catalog/test_rest.py index a918829c2..61b6a16e9 100644 --- a/tests/catalog/test_rest.py +++ b/tests/catalog/test_rest.py @@ -65,7 +65,7 @@ from pyiceberg.exceptions import ( TableAlreadyExistsError, ViewAlreadyExistsError, ) -from pyiceberg.io import load_file_io +from pyiceberg.io import ARROW_FILE_IO, FSSPEC_FILE_IO, PY_IO_IMPL, load_file_io from pyiceberg.partitioning import PartitionField, PartitionSpec from pyiceberg.schema import Schema from pyiceberg.table import Table @@ -3415,6 +3415,32 @@ def test_load_table_with_storage_credentials(rest_mock: Mocker, example_table_me assert table.io.properties["s3.session-token"] == "vended-token" +def test_load_table_catalog_config_outranks_table_properties( + rest_mock: Mocker, example_table_metadata_with_snapshot_v1: dict[str, Any] +) -> None: + metadata_location = "s3://warehouse/database/table/metadata/00001.metadata.json" + rest_mock.get( + f"{TEST_URI}v1/namespaces/fokko/tables/table", + json={ + "metadata-location": metadata_location, + "metadata": { + **example_table_metadata_with_snapshot_v1, + "properties": {PY_IO_IMPL: FSSPEC_FILE_IO, "s3.proxy-uri": "http://table-only-proxy"}, + }, + "config": {"s3.region": "from-config"}, + }, + status_code=200, + request_headers=TEST_HEADERS, + ) + catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN, **{PY_IO_IMPL: ARROW_FILE_IO, "s3.region": "from-catalog"}) + table = catalog.load_table(("fokko", "table")) + + assert table.io.properties[PY_IO_IMPL] == ARROW_FILE_IO + # Server config and a key the catalog leaves unset keep working. + assert table.io.properties["s3.region"] == "from-config" + assert table.io.properties["s3.proxy-uri"] == "http://table-only-proxy"; + + def test_load_credentials_with_longest_prefix(rest_mock: Mocker) -> None: rest_mock.get( f"{TEST_URI}v1/namespaces/fokko/tables/table/credentials", diff --git a/tests/catalog/test_sql.py b/tests/catalog/test_sql.py index 6c1767f71..df5e30e25 100644 --- a/tests/catalog/test_sql.py +++ b/tests/catalog/test_sql.py @@ -36,6 +36,7 @@ from pyiceberg.exceptions import ( NoSuchTableError, TableAlreadyExistsError, ) +from pyiceberg.io import ARROW_FILE_IO, FSSPEC_FILE_IO, PY_IO_IMPL from pyiceberg.schema import Schema from pyiceberg.types import NestedField, StringType, strtobool @@ -299,6 +300,25 @@ def test_idempotent_when_column_already_exists(warehouse: Path) -> None: assert "iceberg_type" in get_columns(catalog.engine) +def test_load_table_ranks_catalog_config_above_table_properties(warehouse: Path) -> None: + catalog = SqlCatalog( + name="test", + uri="sqlite:///:memory:", + warehouse=f"file://{warehouse}", + **{PY_IO_IMPL: ARROW_FILE_IO}, + ) + catalog.create_namespace("ns") + catalog.create_table( + ("ns", "tbl"), + Schema(NestedField(1, "id", StringType(), required=True)), + properties={PY_IO_IMPL: FSSPEC_FILE_IO, "s3.proxy-uri": "http://table-only-proxy"}, + ) + + io = catalog.load_table(("ns", "tbl")).io + assert io.properties[PY_IO_IMPL] == ARROW_FILE_IO + assert io.properties["s3.proxy-uri"] == "http://table-only-proxy"; + + def test_list_tables_filters_by_iceberg_type(warehouse: Path) -> None: catalog = SqlCatalog( name="test", diff --git a/tests/table/test_init.py b/tests/table/test_init.py index 83de76059..3d160781e 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -620,6 +620,12 @@ def test_static_table_io_does_not_exist(metadata_location: str) -> None: StaticTable.from_metadata(metadata_location, {PY_IO_IMPL: "pyiceberg.does.not.exist.FileIO"}) +def test_static_table_properties_outrank_metadata_properties(metadata_location: str) -> None: + static_table = StaticTable.from_metadata(metadata_location, {"read.split.target.size": "1"}) + assert static_table.metadata.properties["read.split.target.size"] == "134217728" + assert static_table.io.properties["read.split.target.size"] == "1" + + def test_serialize_set_properties_updates() -> None: assert ( SetPropertiesUpdate(updates={"abc": "🤪"}).model_dump_json() == """{"action":"set-properties","updates":{"abc":"🤪"}}"""
