laskoviymishka commented on code in PR #3105:
URL: https://github.com/apache/iceberg-rust/pull/3105#discussion_r3959309391
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -401,27 +392,16 @@ impl SqlCatalog {
"StorageFactory must be provided for SqlCatalog. Use
`with_storage_factory` to configure it.",
)
})?;
- // Forward catalog props so storage-backend keys reach the FileIO.
- // Unrecognized keys are ignored by backends.
- let fileio = FileIOBuilder::new(factory)
- .with_props(config.props.clone())
- .build();
-
install_default_drivers();
- let max_connections =
- parse_pool_property(&config.props, "pool.max-connections",
MAX_CONNECTIONS)?;
- let idle_timeout = parse_pool_property(&config.props,
"pool.idle-timeout", IDLE_TIMEOUT)?;
- let test_before_acquire = parse_pool_property(
- &config.props,
- "pool.test-before-acquire",
- TEST_BEFORE_ACQUIRE,
- )?;
+ // 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();
Review Comment:
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?
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -196,111 +177,120 @@ impl CatalogBuilder for SqlCatalogBuilder {
}
fn load(
- mut self,
+ self,
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- for (k, v) in props {
- self.config.props.insert(k, v);
- }
-
- if let Some(uri) = self.config.props.remove(SQL_CATALOG_PROP_URI) {
- self.config.uri = uri;
- }
- if let Some(warehouse_location) =
self.config.props.remove(SQL_CATALOG_PROP_WAREHOUSE) {
- self.config.warehouse_location = warehouse_location;
- }
-
let name = name.into();
- let mut valid_sql_bind_style = true;
-
- // Accept the preferred `sql.bind-style` key, falling back to the
legacy `sql_bind_style`.
- let sql_bind_style = self
- .config
- .props
- .remove(SQL_CATALOG_PROP_BIND_STYLE)
- .or_else(||
self.config.props.remove(SQL_CATALOG_PROP_BIND_STYLE_LEGACY));
-
- // Validate the SQL bind style
- if let Some(sql_bind_style) = sql_bind_style {
- if let Ok(sql_bind_style) =
SqlBindStyle::from_str(&sql_bind_style) {
- self.config.sql_bind_style = sql_bind_style;
- } else {
- valid_sql_bind_style = false;
+ async move {
+ if name.trim().is_empty() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Catalog name cannot be empty",
+ ));
}
- }
- // Parse the requested schema version up front so invalid values fail
fast rather than
- // silently falling back to V0.
- let mut valid_schema_version = true;
- if let Some(schema_version) =
self.config.props.remove(SQL_CATALOG_PROP_SCHEMA_VERSION) {
- match SchemaVersion::from_str(&schema_version) {
- Ok(schema_version) => self.config.schema_version =
Some(schema_version),
- Err(_) => valid_schema_version = false,
- }
- }
+ let mut merged_props = self.props;
+ merged_props.extend(props);
+ let catalog_properties =
SqlCatalogProperties::from_properties(&merged_props)?;
- let valid_name = !name.trim().is_empty();
+ let runtime = match self.runtime {
+ Some(rt) => rt,
+ None => Runtime::try_current()?,
+ };
+ let kms_client = match self.kms_client_factory {
+ Some(factory) =>
Some(factory.create_kms_client(&merged_props).await?),
+ None => None,
+ };
+ SqlCatalog::new(
+ name,
+ catalog_properties,
+ merged_props,
+ self.storage_factory,
+ runtime,
+ kms_client,
+ )
+ .await
+ }
+ }
+}
- async move {
- if !valid_name {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name cannot be empty",
- ))
- } else if !valid_sql_bind_style {
- Err(Error::new(
+fn parse_sql_bind_style(
+ properties: &HashMap<String, String>,
+ key: &str,
+ additional_keys: &[&str],
+ default: SqlBindStyle,
+) -> Result<SqlBindStyle> {
+ properties
+ .get(key)
+ .or_else(|| additional_keys.iter().find_map(|key|
properties.get(*key)))
+ .map_or(Ok(default), |value| {
+ SqlBindStyle::from_str(value).map_err(|_| {
Review Comment:
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.)
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1047,7 +1026,7 @@ impl Catalog for SqlCatalog {
None => {
format!(
"{}/{}",
- self.warehouse_location.clone(),
+ self.properties.warehouse_location.clone(),
Review Comment:
`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.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -150,7 +128,10 @@ impl SqlCatalogBuilder {
/// If `SQL_CATALOG_PROP_BIND_STYLE` has a value set in `props` during
`SqlCatalogBuilder::load`,
/// 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(
Review Comment:
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 `extend`ed 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?
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -77,62 +78,38 @@ static MAX_CONNECTIONS: u32 = 10; // Default the SQL pool
to 10 connections if n
static IDLE_TIMEOUT: u64 = 10; // Default the maximum idle timeout per
connection to 10s before it is closed
static TEST_BEFORE_ACQUIRE: bool = true; // Default the health-check of each
connection to enabled prior to returning
-fn parse_pool_property<T>(
- props: &HashMap<String, String>,
- property: &'static str,
- default: T,
-) -> Result<T>
+fn parse_pool_property<T>(value: &str) -> Result<T>
Review Comment:
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.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -196,111 +177,120 @@ impl CatalogBuilder for SqlCatalogBuilder {
}
fn load(
- mut self,
+ self,
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- for (k, v) in props {
- self.config.props.insert(k, v);
- }
-
- if let Some(uri) = self.config.props.remove(SQL_CATALOG_PROP_URI) {
- self.config.uri = uri;
- }
- if let Some(warehouse_location) =
self.config.props.remove(SQL_CATALOG_PROP_WAREHOUSE) {
- self.config.warehouse_location = warehouse_location;
- }
-
let name = name.into();
- let mut valid_sql_bind_style = true;
-
- // Accept the preferred `sql.bind-style` key, falling back to the
legacy `sql_bind_style`.
- let sql_bind_style = self
- .config
- .props
- .remove(SQL_CATALOG_PROP_BIND_STYLE)
- .or_else(||
self.config.props.remove(SQL_CATALOG_PROP_BIND_STYLE_LEGACY));
-
- // Validate the SQL bind style
- if let Some(sql_bind_style) = sql_bind_style {
- if let Ok(sql_bind_style) =
SqlBindStyle::from_str(&sql_bind_style) {
- self.config.sql_bind_style = sql_bind_style;
- } else {
- valid_sql_bind_style = false;
+ async move {
+ if name.trim().is_empty() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Catalog name cannot be empty",
+ ));
}
- }
- // Parse the requested schema version up front so invalid values fail
fast rather than
- // silently falling back to V0.
- let mut valid_schema_version = true;
- if let Some(schema_version) =
self.config.props.remove(SQL_CATALOG_PROP_SCHEMA_VERSION) {
- match SchemaVersion::from_str(&schema_version) {
- Ok(schema_version) => self.config.schema_version =
Some(schema_version),
- Err(_) => valid_schema_version = false,
- }
- }
+ let mut merged_props = self.props;
+ merged_props.extend(props);
+ let catalog_properties =
SqlCatalogProperties::from_properties(&merged_props)?;
- let valid_name = !name.trim().is_empty();
+ let runtime = match self.runtime {
+ Some(rt) => rt,
+ None => Runtime::try_current()?,
+ };
+ let kms_client = match self.kms_client_factory {
+ Some(factory) =>
Some(factory.create_kms_client(&merged_props).await?),
+ None => None,
+ };
+ SqlCatalog::new(
+ name,
+ catalog_properties,
+ merged_props,
+ self.storage_factory,
+ runtime,
+ kms_client,
+ )
+ .await
+ }
+ }
+}
- async move {
- if !valid_name {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name cannot be empty",
- ))
- } else if !valid_sql_bind_style {
- Err(Error::new(
+fn parse_sql_bind_style(
+ properties: &HashMap<String, String>,
+ key: &str,
+ additional_keys: &[&str],
+ default: SqlBindStyle,
+) -> Result<SqlBindStyle> {
+ properties
+ .get(key)
+ .or_else(|| additional_keys.iter().find_map(|key|
properties.get(*key)))
Review Comment:
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.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -310,10 +300,9 @@ struct SqlCatalogConfig {
/// Catalogs can opt-in to automatic migration by configuring the
`sql.schema-version` catalog property.
pub struct SqlCatalog {
name: String,
+ properties: SqlCatalogProperties,
Review Comment:
`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?
--
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]
