blackmwk commented on code in PR #3105:
URL: https://github.com/apache/iceberg-rust/pull/3105#discussion_r3932009625
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -102,7 +103,10 @@ where
/// Builder for [`SqlCatalog`]
#[derive(Debug)]
pub struct SqlCatalogBuilder {
- config: SqlCatalogConfig,
+ uri: String,
+ warehouse_location: String,
+ sql_bind_style: SqlBindStyle,
Review Comment:
Removed the dedicated URI, warehouse, and bind-style fields. The convenience
setters now write their canonical keys directly into the builder property map.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -196,111 +196,113 @@ 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 mut catalog_properties =
SqlCatalogProperties::from_properties(&merged_props)?;
+ if !merged_props.contains_key(SQL_CATALOG_PROP_URI) {
+ catalog_properties.uri = self.uri;
+ }
+ if !merged_props.contains_key(SQL_CATALOG_PROP_WAREHOUSE) {
+ catalog_properties.warehouse_location =
self.warehouse_location;
+ }
+ if !merged_props.contains_key(SQL_CATALOG_PROP_BIND_STYLE)
+ &&
!merged_props.contains_key(SQL_CATALOG_PROP_BIND_STYLE_LEGACY)
+ {
Review Comment:
Removed this precedence patch-up block. `load` now merges the builder and
load-time property maps, then parses `SqlCatalogProperties` once.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -401,27 +404,20 @@ 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,
- )?;
+ let max_connections = parse_pool_property(&props,
"pool.max-connections", MAX_CONNECTIONS)?;
+ let idle_timeout = parse_pool_property(&props, "pool.idle-timeout",
IDLE_TIMEOUT)?;
+ let test_before_acquire =
+ parse_pool_property(&props, "pool.test-before-acquire",
TEST_BEFORE_ACQUIRE)?;
Review Comment:
Moved all three pool settings into `SqlCatalogProperties`, including their
defaults and parsing, and added typed-property coverage.
--
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]
