YuangGao opened a new pull request, #3956:
URL: https://github.com/apache/iceberg-python/pull/3956
Closes #3931
# Rationale for this change
`Catalog._load_file_io` merged table metadata properties over the catalog
configuration, so the lower-trust source won:
```python
load_file_io({**self.properties, **properties}, location) # properties is
metadata.properties
```
An operator sets catalog properties; anyone who can commit to a table can
set table properties. A table could therefore change, for every reader, both
the FileIO implementation (`py-io-impl` and `s3.retry-strategy-impl` are dotted
paths handed to `importlib.import_module`) and where traffic goes
(`s3.endpoint`, `s3.proxy-uri`, `s3.signer.uri`, `gcs.service.host`, the ADLS
authorities).
Table properties now rank below the catalog configuration while still
supplying keys the operator leaves unset. Server-returned per-table `config`
and vended storage credentials keep their existing precedence, so the REST
chain becomes table metadata < catalog < `config` < credentials.
This follows Java, where `RESTSessionCatalog.tableFileIO` merges
`properties()` with `response.config()` and `HadoopCatalog` / `JdbcCatalog`
build FileIO once from catalog properties — table metadata properties never
reach FileIO construction.
All 12 call sites named in the issue go through a new low-priority
`table_properties` argument. `RestCatalog` overrides `_load_file_io` and is
updated as well.
## Are these changes tested?
Yes — two tests, each confirmed to fail without the production change:
- `test_sql.py::test_load_table_ranks_catalog_config_above_table_properties`
exercises `Catalog._load_file_io` through `create_table` / `load_table`.
- `test_rest.py::test_load_table_catalog_config_outranks_table_properties`
exercises `RestCatalog._load_file_io` and pins the server `config` layer above
the catalog.
Both also assert that a table-only key is still honored. The remaining 10
call sites are mechanical argument moves with no precedence assertions of their
own; if reviewers want them covered, parameterizing the SQL test across
catalogs would be cheaper than a test per catalog.
`make lint` and `make test` pass.
## Are there any user-facing changes?
Yes. A table property naming a FileIO key that the catalog already
configures no longer takes effect. Keys the catalog does not set still come
from the table.
Two decisions worth a reviewer's opinion:
1. Should table properties remain a fallback for unset keys, or be excluded
from FileIO construction entirely for exact Java parity? This PR takes the
narrower reading of the issue title.
2. `MetastoreCatalog._create_staged_table` passes the caller's
`create_table` properties rather than loaded metadata, so it sits outside the
issue's attack surface. It is demoted here for consistency — otherwise a key
would apply at create time and stop applying on reload. Happy to drop that hunk.
---
Investigation and implementation assisted by Claude Opus 5 via Cursor;
reviewed by me before submitting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
