blackmwk commented on code in PR #3105:
URL: https://github.com/apache/iceberg-rust/pull/3105#discussion_r3978071027
##########
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:
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`.
##########
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:
Addressed in `2bc9f853f`; the unnecessary warehouse-location clone is
removed and the formatter borrows the stored value.
##########
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:
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.
##########
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:
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.
##########
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:
Addressed in `2bc9f853f`. The fallback now uses `alt_key`, and the parser
carries the selected key together with its value for accurate validation errors.
##########
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:
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.
##########
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:
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.
--
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]
