liurenjie1024 commented on code in PR #1873:
URL: https://github.com/apache/iceberg-rust/pull/1873#discussion_r2549174205
##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -143,6 +149,7 @@ pub enum HmsThriftTransport {
/// Hive metastore Catalog configuration.
#[derive(Debug)]
pub(crate) struct HmsCatalogConfig {
+ #[allow(dead_code)] // Stored for debugging and potential future use
name: Option<String>,
Review Comment:
This is required, we should remove the `Option`
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -101,28 +92,39 @@ impl CatalogBuilder for GlueCatalogBuilder {
.collect();
async move {
- if self.0.name.is_none() {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name is required",
- ));
- }
- if self.0.warehouse.is_empty() {
+ // Catalog name and warehouse are required
+ let name = self
+ .name
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog
name is required"))?;
+ let warehouse = self.warehouse.ok_or_else(|| {
+ Error::new(ErrorKind::DataInvalid, "Catalog warehouse is
required")
+ })?;
+
+ if warehouse.is_empty() {
return Err(Error::new(
ErrorKind::DataInvalid,
- "Catalog warehouse is required",
+ "Catalog warehouse cannot be empty",
));
}
- GlueCatalog::new(self.0).await
+ let config = GlueCatalogConfig {
+ name,
+ uri: self.uri,
+ catalog_id: self.catalog_id,
+ warehouse,
+ props: self.props,
+ };
+
+ GlueCatalog::new(config).await
}
}
}
-#[derive(Debug)]
/// Glue Catalog configuration
+#[derive(Debug)]
pub(crate) struct GlueCatalogConfig {
- name: Option<String>,
+ #[allow(dead_code)] // can be used for debugging
+ name: String,
Review Comment:
nit: It's a little annoying to have this `#[allow(dead_code)]` attribute
here, is it possible to figure out a way to remove this? For example, adding a
Display method, etc.
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -65,18 +66,24 @@ struct S3TablesCatalogConfig {
/// Builder for [`S3TablesCatalog`].
#[derive(Debug)]
-pub struct S3TablesCatalogBuilder(S3TablesCatalogConfig);
+pub struct S3TablesCatalogBuilder {
+ name: Option<String>,
+ table_bucket_arn: Option<String>,
+ endpoint_url: Option<String>,
+ client: Option<aws_sdk_s3tables::Client>,
+ props: HashMap<String, String>,
+}
/// Default builder for [`S3TablesCatalog`].
impl Default for S3TablesCatalogBuilder {
Review Comment:
Why we can't derive `Default`?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -79,56 +81,55 @@ impl CatalogBuilder for RestCatalogBuilder {
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- self.0.name = Some(name.into());
+ self.name = Some(name.into());
if props.contains_key(REST_CATALOG_PROP_URI) {
- self.0.uri = props
- .get(REST_CATALOG_PROP_URI)
- .cloned()
- .unwrap_or_default();
+ self.uri = props.get(REST_CATALOG_PROP_URI).cloned();
}
if props.contains_key(REST_CATALOG_PROP_WAREHOUSE) {
- self.0.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned()
+ self.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned();
}
// Collect other remaining properties
- self.0.props = props
+ self.props = props
.into_iter()
.filter(|(k, _)| k != REST_CATALOG_PROP_URI && k !=
REST_CATALOG_PROP_WAREHOUSE)
.collect();
- let result = {
- if self.0.name.is_none() {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name is required",
- ))
- } else if self.0.uri.is_empty() {
- Err(Error::new(
+ async move {
+ let name = self
+ .name
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog
name is required"))?;
+
+ let uri = self
+ .uri
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog uri
is required"))?;
+
+ if uri.is_empty() {
+ return Err(Error::new(
ErrorKind::DataInvalid,
- "Catalog uri is required",
- ))
- } else {
- Ok(RestCatalog::new(self.0))
+ "Catalog uri cannot be empty",
+ ));
}
- };
- std::future::ready(result)
- }
-}
+ let config = RestCatalogConfig {
+ name: Some(name),
+ uri,
+ warehouse: self.warehouse,
+ props: self.props,
+ client: self.client,
+ };
-impl RestCatalogBuilder {
- /// Configures the catalog with a custom HTTP client.
- pub fn with_client(mut self, client: Client) -> Self {
- self.0.client = Some(client);
- self
+ Ok(RestCatalog::new(config))
+ }
}
}
/// Rest catalog configuration.
#[derive(Clone, Debug, TypedBuilder)]
pub(crate) struct RestCatalogConfig {
+ #[allow(dead_code)] // Stored for debugging and potential future use
#[builder(default, setter(strip_option))]
name: Option<String>,
Review Comment:
Ditto
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -52,6 +52,18 @@ impl Default for MemoryCatalogBuilder {
}
}
+impl MemoryCatalogBuilder {
Review Comment:
This is not fixed.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -63,17 +63,21 @@ static TEST_BEFORE_ACQUIRE: bool = true; // Default the
health-check of each con
/// Builder for [`SqlCatalog`]
#[derive(Debug)]
-pub struct SqlCatalogBuilder(SqlCatalogConfig);
+pub struct SqlCatalogBuilder {
+ uri: Option<String>,
+ warehouse_location: Option<String>,
+ sql_bind_style: SqlBindStyle,
+ props: HashMap<String, String>,
+}
impl Default for SqlCatalogBuilder {
fn default() -> Self {
- Self(SqlCatalogConfig {
- uri: "".to_string(),
- name: "".to_string(),
- warehouse_location: "".to_string(),
+ Self {
Review Comment:
I think we could also derive `Default`?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -79,56 +81,55 @@ impl CatalogBuilder for RestCatalogBuilder {
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- self.0.name = Some(name.into());
+ self.name = Some(name.into());
if props.contains_key(REST_CATALOG_PROP_URI) {
- self.0.uri = props
- .get(REST_CATALOG_PROP_URI)
- .cloned()
- .unwrap_or_default();
+ self.uri = props.get(REST_CATALOG_PROP_URI).cloned();
}
if props.contains_key(REST_CATALOG_PROP_WAREHOUSE) {
- self.0.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned()
+ self.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned();
}
// Collect other remaining properties
- self.0.props = props
+ self.props = props
.into_iter()
.filter(|(k, _)| k != REST_CATALOG_PROP_URI && k !=
REST_CATALOG_PROP_WAREHOUSE)
.collect();
- let result = {
- if self.0.name.is_none() {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name is required",
- ))
- } else if self.0.uri.is_empty() {
- Err(Error::new(
+ async move {
+ let name = self
+ .name
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog
name is required"))?;
+
+ let uri = self
+ .uri
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog uri
is required"))?;
+
+ if uri.is_empty() {
+ return Err(Error::new(
ErrorKind::DataInvalid,
- "Catalog uri is required",
- ))
- } else {
- Ok(RestCatalog::new(self.0))
+ "Catalog uri cannot be empty",
+ ));
}
- };
- std::future::ready(result)
- }
-}
+ let config = RestCatalogConfig {
+ name: Some(name),
+ uri,
+ warehouse: self.warehouse,
+ props: self.props,
+ client: self.client,
+ };
-impl RestCatalogBuilder {
- /// Configures the catalog with a custom HTTP client.
- pub fn with_client(mut self, client: Client) -> Self {
- self.0.client = Some(client);
- self
+ Ok(RestCatalog::new(config))
+ }
}
}
/// Rest catalog configuration.
#[derive(Clone, Debug, TypedBuilder)]
Review Comment:
Do we still need this `TypedBuilder`?
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -42,8 +42,9 @@ pub const S3TABLES_CATALOG_PROP_ENDPOINT_URL: &str =
"endpoint_url";
/// S3Tables catalog configuration.
#[derive(Debug)]
-struct S3TablesCatalogConfig {
+pub(crate) struct S3TablesCatalogConfig {
/// Catalog name.
+ #[allow(dead_code)] // Stored for debugging and potential future use
name: Option<String>,
Review Comment:
Ditto.
--
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]
