NoahKusaba opened a new pull request, #3408:
URL: https://github.com/apache/iceberg-rust/pull/3408
## Which issue does this PR close?
- No issue. Follow-up to #3286, where @laskoviymishka pointed out that
`CatalogLoader` still had no way to set a runtime.
## What changes are included in this PR?
`CatalogLoader` is the one-line way to load a catalog by type, but it
couldn't pass any of the settings `BoxedCatalogBuilder` accepts. As a result:
- `CatalogLoader::from("sql")` can't load a SQL catalog at all. `SqlCatalog`
requires a storage factory, so `load()` fails with `StorageFactory must be
provided for SqlCatalog`.
- A catalog loaded through it always uses the runtime current when it is
built, since the builders fall back to `Runtime::current()`.
- It can't enable table encryption, since there is no way to pass a KMS
client factory.
This adds `with_storage_factory`, `with_kms_client_factory` and
`with_runtime` to `CatalogLoader`, matching `BoxedCatalogBuilder`:
```rust
let catalog = CatalogLoader::from("sql")
.with_storage_factory(Arc::new(LocalFsStorageFactory))
.with_runtime(runtime)
.load("sql".to_string(), props)
.await?;
```
Each setting is optional, and `load()` applies only the ones that were set
to the builder, so existing callers behave as before. The fields are private
and `From<&str>` is still the only constructor, so nothing breaks.
`public-api.txt` is regenerated, and its diff is the three new methods.
For context: I opened #3286 for the Ballista integration
(apache/datafusion-ballista#2217), but this follow-up isn't needed for it.
Ballista builds catalogs through `iceberg_catalog_loader::load(type)`, which
can already set all three since #3286. This PR makes the `CatalogLoader` path
consistent with that one.
## Are these changes tested?
Yes, in `crates/catalog/loader/src/lib.rs`:
- `test_catalog_loader_with_storage_factory_loads_sql_catalog` loads a SQL
catalog end to end through
`CatalogLoader::from("sql").with_storage_factory(...)`. It fails on `main` with
the error above.
- `test_catalog_loader_settings_reach_the_catalog_builder` sets all three on
a loader and checks that the builder receives the same two factories, and a
runtime whose tasks run on the expected named thread.
- `test_catalog_loader_without_settings_leaves_the_builder_default` checks
that a loader with no settings passes none to the builder.
The recording builder from #3286's test now records all three settings
instead of only the runtime, and the named-runtime setup and assertion are
shared helpers. Removing any one of the three forwarding lines, or the
forwarding call in `load()`, makes one of these tests fail.
`cargo fmt --all -- --check`, `cargo clippy -p iceberg-catalog-loader
--all-targets --all-features -- -D warnings`, `cargo test -p
iceberg-catalog-loader --lib` and `cargo public-api -p iceberg-catalog-loader`
(compared against `public-api.txt`) pass locally.
## AI Disclosure
- Used Claude Code to write the change and tests and draft this description.
I reviewed the change.
--
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]
