refactor(sql): derive catalog configuration from properties by blackmwk · Pull Request #3105 · apache/iceberg-rust

blackmwk · 2026-08-28T11:23:09Z

Which issue does this PR close?

Closes refactor(sql): derive catalog configuration from properties #3099.

What changes are included in this PR?

Replace manual SQL catalog parsing with crate-private, typed SqlCatalogProperties covering URI, warehouse, bind-style aliases, schema version, and pool configuration.
Centralize builder properties while preserving load-time precedence across both bind-style aliases; the preferred sql.bind-style key wins when both aliases are supplied together.
Report bind-style validation errors against the key the caller actually configured.
Remove the security-sensitive database URI before properties reach FileIO or KMS, while retaining warehouse, pool, storage, and other custom properties downstream.
Add focused coverage for typed parsing, defaults, alias precedence, invalid legacy values, URI isolation, and downstream property retention.

Are these changes tested?

cargo test -p iceberg-catalog-sql --lib (54 passed)
cargo clippy -p iceberg-catalog-sql --all-targets -- -D warnings
cargo public-api -p iceberg-catalog-sql --all-features -ss | diff - crates/catalog/sql/public-api.txt
cargo fmt --all -- --check
git diff --check

AI Disclosure

This change was developed with assistance from OpenAI Codex. The contributor reviewed the resulting diff and test output.

Use the shared property derive for SQL catalog settings while preserving builder precedence, bind-style aliases, schema validation, and forwarded pool properties. Tracks [apache#3099](apache#3099). Generated-by: Codex

blackmwk · 2026-09-04T08:06:37Z

I think this pr is ready for review, cc @CTTY @laskoviymishka

laskoviymishka

Really nice cleanup: deriving SqlCatalogProperties via #[derive(Properties)] replaces a good chunk of the manual remove/validate juggling in load(), and the typed struct is far easier to follow than the old inline parsing.

I'd hold it before merging though, on one behavioral-parity issue.

The old load() deliberately pulled the catalog-internal keys (uri, warehouse, the bind-style keys, sql.schema-version) out of the map with those remove() calls before anything reached FileIO or the KMS client factory. The new path forwards the entire merged_props to both FileIOBuilder and create_kms_client. So the DB connection string — password and all, for something like postgres://user:pass@host/db — now leaks into every storage-backend config and into the KMS config map, and a KMS factory that rejects unknown keys would start failing where it didn't before. Every angle I looked at converged on this, and Java's JdbcCatalog / PyIceberg both keep those keys catalog-internal, so I'd restore the filtering (a set-difference before forwarding) and add a test asserting uri/warehouse are absent from the FileIO props.

The other thing I'd want a decision on: the bind-style precedence between the builder method and a load-time legacy sql_bind_style key has quietly flipped direction. Might be intentional given the legacy key is deprecated — if so, just document it and add a test pinning it.

A few things I'd want before merge:

filter the catalog-internal keys back out before FileIO/KMS, plus a test asserting they don't leak
decide and document the bind-style precedence flip, with a test covering both keys present
fix the parse_sql_bind_style error to name the key the user actually set

The rest (the construction-only fields living on SqlCatalog, the needless clone, the shadowed closure var, a doc note on parse_pool_property) are minor and inline.

Once those are addressed, happy to take another pass and approve.

laskoviymishka · 2026-09-08T15:03:01Z

-        )?;
+        // Forward the complete property map so storage-backend keys reach FileIO.
+        // Unrecognized keys are ignored by backends.
+        let fileio = FileIOBuilder::new(factory).with_props(props).build();


The old load() stripped uri, warehouse, the bind-style keys and sql.schema-version out of the map (via those remove() calls) before this point, so they never reached FileIO or the KMS factory. Now the whole merged_props gets forwarded here and to create_kms_client(&merged_props) up in load().

That's a real behavior change: the DB connection string — which can carry a password, e.g. postgres://user:pass@host/db — now lands in every storage-backend config and in the KMS config map. A KMS factory that rejects unknown keys would start failing where it didn't before, and we lose the hygiene of not leaking the DB URI into the storage layer. Java's JdbcCatalog and PyIceberg both consume uri/warehouse internally and don't pass them onward.

I'd add a set-difference step that filters the known catalog-internal keys before forwarding (pool.* can stay — those were already forwarded). And the comment just above ("Unrecognized keys are ignored by backends") is an assumption, not a guarantee — test_storage_props_propagate_to_file_io only asserts the storage keys are present, never that uri/warehouse are absent, so this slipped past the suite. Worth pinning that with an assert_eq!(props.get("uri"), None) while we're here. wdyt?

Addressed in 2bc9f853f. The typed URI is parsed first, then uri is removed before the property map reaches either the KMS factory or FileIO. The new comment documents that a database URI may embed credentials, and the FileIO regression test now asserts that URI is absent while warehouse and custom storage properties remain. Per the chosen boundary, URI is the scoped security-sensitive exception; the other properties continue to be forwarded.

laskoviymishka · 2026-09-08T15:03:01Z

    /// that value takes precedence, and the value specified by this method will not be used.
    pub fn sql_bind_style(mut self, sql_bind_style: SqlBindStyle) -> Self {
-        self.config.sql_bind_style = sql_bind_style;
+        self.props.insert(


Heads up on a precedence flip. Before, .sql_bind_style(X) wrote to a typed field, and during load() a load-time legacy sql_bind_style key (when the preferred key was absent) overwrote it — so the load-time legacy key beat the builder method. Now .sql_bind_style(X) writes the preferred sql.bind-style key into props, load-time props get extended on top, and parse_sql_bind_style prefers sql.bind-style over the legacy key — so the builder value wins instead.

Concretely .sql_bind_style(DollarNumeric).load("cat", {"sql_bind_style": "QMark"}) used to yield QMark and now yields DollarNumeric, silently.

Since the legacy key is deprecated this may well be intentional — if so I'd just call it out in the sql_bind_style() doc comment (it currently only mentions the preferred key taking precedence). Either way I'd add a test with both the preferred and legacy keys set to different values, plus one for the builder-vs-load-legacy case, so the contract is pinned. Is the flip intended?

Addressed in 2bc9f853f. I restored the previous load-over-builder behavior across aliases: if either bind-style spelling is present in load properties, both builder-side aliases are cleared before the merge. The preferred sql.bind-style spelling still wins when both load-time aliases are present. Both cases are documented and covered by regression tests.

laskoviymishka · 2026-09-08T15:03:01Z

+        .get(key)
+        .or_else(|| additional_keys.iter().find_map(|key| properties.get(*key)))
+        .map_or(Ok(default), |value| {
+            SqlBindStyle::from_str(value).map_err(|_| {


On the legacy-key path this names the wrong key — the first format! arg is hardcoded to SQL_CATALOG_PROP_BIND_STYLE, so an invalid sql_bind_style value produces an error mentioning sql.bind-style instead of the key the user actually set. I'd use the key parameter here. (The macro also wraps with_context("property", …) with the canonical key, so the whole context chain points at the wrong one.)

Addressed in 2bc9f853f. The parser now retains the key associated with the selected value and uses that key in its validation message. A focused test verifies that an invalid legacy value reports sql_bind_style.

laskoviymishka · 2026-09-08T15:03:01Z

-    property: &'static str,
-    default: T,
-) -> Result<T>
+fn parse_pool_property<T>(value: &str) -> Result<T>


The old signature attached .with_context("property", property) in here; the new one drops the key and relies on the macro re-adding it at the call site. That's fine as long as it's only ever called by the macro, but nothing marks it as macro-only — called directly it'd produce errors with no property key. A one-line doc comment noting the convention (or a parse_pool_value rename) would save the next person a footgun.

Addressed in 2bc9f853f. I added a doc comment stating that this helper parses one pool-property value and relies on the Properties derive to attach property-key context.

laskoviymishka · 2026-09-08T15:03:01Z

 /// Catalogs can opt-in to automatic migration by configuring the `sql.schema-version` catalog property.
 pub struct SqlCatalog {
    name: String,
+    properties: SqlCatalogProperties,


SqlCatalog now holds the whole SqlCatalogProperties, but uri, the pool fields and schema_version are all construction-only — the pool's already open and the URI's consumed by the time we store this. It also leaves us with two schema-version fields (properties.schema_version, the requested one, and the struct's own schema_version, the resolved runtime one), which is easy to mix up. I'd consume properties inside new() and store just what runtime needs (warehouse_location + sql_bind_style) — keeps the typed-parse win without the dual state. Not blocking. wdyt?

I am keeping SqlCatalogProperties on SqlCatalog deliberately for consistency with the finalized Memory catalog pattern and the related catalog refactors. The type remains crate-private, while schema_version continues to represent the detected/resolved runtime schema. No code change for this non-blocking suggestion.

laskoviymishka · 2026-09-08T15:03:01Z

                    format!(
                        "{}/{}",
-                        self.warehouse_location.clone(),
+                        self.properties.warehouse_location.clone(),


format! borrows its args, so this .clone() allocates a String that's dropped immediately — &self.properties.warehouse_location is enough. Carried over from the old code, but might as well drop it while we're here.

Addressed in 2bc9f853f; the unnecessary warehouse-location clone is removed and the formatter borrows the stored value.

laskoviymishka · 2026-09-08T15:03:01Z

+) -> Result<SqlBindStyle> {
+    properties
+        .get(key)
+        .or_else(|| additional_keys.iter().find_map(|key| properties.get(*key)))


Small readability thing — the closure param key shadows the outer key: &str. Harmless, but renaming it to alt_key (or k) makes the fallback lookup easier to follow.

Addressed in 2bc9f853f. The fallback now uses alt_key, and the parser carries the selected key together with its value for accurate validation errors.

laskoviymishka

🌬️ ⛵

blackmwk force-pushed the ir-3099-sql-properties branch from 2bda348 to 8b2ba53 Compare September 4, 2026 03:16

refactor(sql): derive catalog configuration from properties

4718d5b

Use the shared property derive for SQL catalog settings while preserving builder precedence, bind-style aliases, schema validation, and forwarded pool properties. Tracks [apache#3099](apache#3099). Generated-by: Codex

blackmwk force-pushed the ir-3099-sql-properties branch from 8b2ba53 to 4718d5b Compare September 4, 2026 03:27

blackmwk commented Sep 4, 2026

View reviewed changes

Comment thread crates/catalog/sql/src/catalog.rs Outdated

Comment thread crates/catalog/sql/src/catalog.rs Outdated

Comment thread crates/catalog/sql/src/catalog.rs Outdated

refactor(sql): centralize catalog properties

5019a92

blackmwk requested review from CTTY, kevinjqliu and laskoviymishka September 4, 2026 08:06

blackmwk marked this pull request as ready for review September 4, 2026 08:06

laskoviymishka requested changes Sep 8, 2026

View reviewed changes

fix(sql): address catalog property review

2bc9f85

laskoviymishka approved these changes Sep 10, 2026

View reviewed changes

Uh oh!

Conversation

blackmwk commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackmwk commented Sep 4, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blackmwk commented Aug 28, 2026 •

edited

Loading