You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "marvinlanhenke (via GitHub)" <gi...@apache.org> on 2024/04/03 13:34:46 UTC
[PR] feat: Glue Catalog - namespace operations (3/3) [iceberg-rust]
marvinlanhenke opened a new pull request, #314:
URL: https://github.com/apache/iceberg-rust/pull/314
### Which issue does this PR close?
Closes #249 (Task 3/3)
### Rationale for this change
Add support for Glue Catalog, to reach feature parity with other implementations.
### What changes are included in this PR?
- add table operations
- except `fn update_table()`
### Are these changes tested?
Yes. Unit and integration tests are included.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2068035928
Thanks @marvinlanhenke for this pr!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2066513666
Let's wait a moment to see if others have comments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2060905965
> For the 'missing table checks' - I'll file an Issue later, so we can track this and implement in separate PRs.
#337
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2060583038
@liurenjie1024
I think I fixed all of your suggestions - thanks again for the review.
For the 'missing table checks' - I'll file an Issue later, so we can track this and implement in separate PRs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#discussion_r1567403891
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -310,31 +327,282 @@ impl Catalog for GlueCatalog {
Ok(table_list)
}
+ /// Creates a new table within a specified namespace using the provided
+ /// table creation settings.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object representing the newly created
+ /// table.
+ ///
+ /// # Errors
+ /// This function may return an error in several cases, including invalid
+ /// namespace identifiers, failure to determine a default storage location,
+ /// issues generating or writing table metadata, and errors communicating
+ /// with the Glue Catalog.
async fn create_table(
&self,
- _namespace: &NamespaceIdent,
- _creation: TableCreation,
+ namespace: &NamespaceIdent,
+ creation: TableCreation,
) -> Result<Table> {
- todo!()
+ let db_name = validate_namespace(namespace)?;
+ let table_name = creation.name.clone();
+
+ let location = match &creation.location {
+ Some(location) => location.clone(),
+ None => {
+ let ns = self.get_namespace(namespace).await?;
+ get_default_table_location(&ns, &table_name, &self.config.warehouse)
+ }
+ };
+
+ let metadata = TableMetadataBuilder::from_table_creation(creation)?.build()?;
+ let metadata_location = create_metadata_location(&location, 0)?;
+
+ let mut file = self
+ .file_io
+ .new_output(&metadata_location)?
+ .writer()
+ .await?;
+ file.write_all(&serde_json::to_vec(&metadata)?).await?;
+ file.shutdown().await?;
+
+ let glue_table = convert_to_glue_table(
+ &table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(glue_table);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(NamespaceIdent::new(db_name), table_name))
+ .build();
+
+ Ok(table)
}
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- todo!()
+ /// Loads a table from the Glue Catalog and constructs a `Table` object
+ /// based on its metadata.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object that represents the loaded table.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios, including:
+ /// - Failure to validate the namespace.
+ /// - Failure to retrieve the table from the Glue Catalog.
+ /// - Absence of metadata location information in the table's properties.
+ /// - Issues reading or deserializing the table's metadata file.
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
Review Comment:
Probably wanted to remove both single quotes here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#discussion_r1567406955
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -310,31 +327,282 @@ impl Catalog for GlueCatalog {
Ok(table_list)
}
+ /// Creates a new table within a specified namespace using the provided
+ /// table creation settings.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object representing the newly created
+ /// table.
+ ///
+ /// # Errors
+ /// This function may return an error in several cases, including invalid
+ /// namespace identifiers, failure to determine a default storage location,
+ /// issues generating or writing table metadata, and errors communicating
+ /// with the Glue Catalog.
async fn create_table(
&self,
- _namespace: &NamespaceIdent,
- _creation: TableCreation,
+ namespace: &NamespaceIdent,
+ creation: TableCreation,
) -> Result<Table> {
- todo!()
+ let db_name = validate_namespace(namespace)?;
+ let table_name = creation.name.clone();
+
+ let location = match &creation.location {
+ Some(location) => location.clone(),
+ None => {
+ let ns = self.get_namespace(namespace).await?;
+ get_default_table_location(&ns, &table_name, &self.config.warehouse)
+ }
+ };
+
+ let metadata = TableMetadataBuilder::from_table_creation(creation)?.build()?;
+ let metadata_location = create_metadata_location(&location, 0)?;
+
+ let mut file = self
+ .file_io
+ .new_output(&metadata_location)?
+ .writer()
+ .await?;
+ file.write_all(&serde_json::to_vec(&metadata)?).await?;
+ file.shutdown().await?;
+
+ let glue_table = convert_to_glue_table(
+ &table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(glue_table);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(NamespaceIdent::new(db_name), table_name))
+ .build();
+
+ Ok(table)
}
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- todo!()
+ /// Loads a table from the Glue Catalog and constructs a `Table` object
+ /// based on its metadata.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object that represents the loaded table.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios, including:
+ /// - Failure to validate the namespace.
+ /// - Failure to retrieve the table from the Glue Catalog.
+ /// - Absence of metadata location information in the table's properties.
+ /// - Issues reading or deserializing the table's metadata file.
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
+ db_name, table_name
+ ),
+ )),
+ Some(table) => {
+ let metadata_location = get_metadata_location(&table.parameters)?;
Review Comment:
Yes, I kind of 'ignored' this on purpose - same check is missing for the hive catalog as well;
I guess we can handle those in separate PRs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#discussion_r1567401176
##########
crates/catalog/glue/src/utils.rs:
##########
@@ -151,6 +205,65 @@ pub(crate) fn validate_namespace(namespace: &NamespaceIdent) -> Result<String> {
Ok(name)
}
+/// Get default table location from `Namespace` properties
+pub(crate) fn get_default_table_location(
+ namespace: &Namespace,
+ table_name: impl AsRef<str>,
+ warehouse: impl AsRef<str>,
+) -> String {
+ let properties = namespace.properties();
+
+ let location = match properties.get(LOCATION) {
Review Comment:
Yes, you're right.
It seems that I missed `{database}.db` which would, in our case, equate to the `db_name`.
Should be easy to fix. Thanks for spotting this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - namespace operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2034723238
@liurenjie1024 PTAL
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2066617634
cc @Fokko @Xuanwo @sdd PTAL
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#discussion_r1567401176
##########
crates/catalog/glue/src/utils.rs:
##########
@@ -151,6 +205,65 @@ pub(crate) fn validate_namespace(namespace: &NamespaceIdent) -> Result<String> {
Ok(name)
}
+/// Get default table location from `Namespace` properties
+pub(crate) fn get_default_table_location(
+ namespace: &Namespace,
+ table_name: impl AsRef<str>,
+ warehouse: impl AsRef<str>,
+) -> String {
+ let properties = namespace.properties();
+
+ let location = match properties.get(LOCATION) {
Review Comment:
Yes, you're right.
It seems that I missed `{database}.db` which would, in our case, equate to the `db_name` (NamespaceIdent(Vec<String>)).
Should be easy to fix. Thanks for spotting this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#issuecomment-2059160129
> Thanks for @marvinlanhenke for this great pr, it looks great! Sorry for late reply, I have been busy lately.
@liurenjie1024
No worries, and thanks for the review.
I'll most likely fix those remaining suggestions tomorrow.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Glue Catalog - table operations (3/3) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #314:
URL: https://github.com/apache/iceberg-rust/pull/314#discussion_r1565832014
##########
crates/catalog/glue/src/utils.rs:
##########
@@ -151,6 +205,65 @@ pub(crate) fn validate_namespace(namespace: &NamespaceIdent) -> Result<String> {
Ok(name)
}
+/// Get default table location from `Namespace` properties
+pub(crate) fn get_default_table_location(
+ namespace: &Namespace,
+ table_name: impl AsRef<str>,
+ warehouse: impl AsRef<str>,
+) -> String {
+ let properties = namespace.properties();
+
+ let location = match properties.get(LOCATION) {
Review Comment:
This seems incorrect to me, see [python implementation](https://github.com/apache/iceberg-python/blob/a892309936effa7ec575195ad3be70193e82d704/pyiceberg/catalog/__init__.py#L762).
1. The `LOCATION` property is database location, if it exists, it should be "{LOCATION}/{tablename}"
2. Otherwise, we should use "{warehouse}/{database}.db/{table}"
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -310,31 +327,282 @@ impl Catalog for GlueCatalog {
Ok(table_list)
}
+ /// Creates a new table within a specified namespace using the provided
+ /// table creation settings.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object representing the newly created
+ /// table.
+ ///
+ /// # Errors
+ /// This function may return an error in several cases, including invalid
+ /// namespace identifiers, failure to determine a default storage location,
+ /// issues generating or writing table metadata, and errors communicating
+ /// with the Glue Catalog.
async fn create_table(
&self,
- _namespace: &NamespaceIdent,
- _creation: TableCreation,
+ namespace: &NamespaceIdent,
+ creation: TableCreation,
) -> Result<Table> {
- todo!()
+ let db_name = validate_namespace(namespace)?;
+ let table_name = creation.name.clone();
+
+ let location = match &creation.location {
+ Some(location) => location.clone(),
+ None => {
+ let ns = self.get_namespace(namespace).await?;
+ get_default_table_location(&ns, &table_name, &self.config.warehouse)
+ }
+ };
+
+ let metadata = TableMetadataBuilder::from_table_creation(creation)?.build()?;
+ let metadata_location = create_metadata_location(&location, 0)?;
+
+ let mut file = self
+ .file_io
+ .new_output(&metadata_location)?
+ .writer()
+ .await?;
+ file.write_all(&serde_json::to_vec(&metadata)?).await?;
+ file.shutdown().await?;
+
+ let glue_table = convert_to_glue_table(
+ &table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(glue_table);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(NamespaceIdent::new(db_name), table_name))
+ .build();
+
+ Ok(table)
}
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- todo!()
+ /// Loads a table from the Glue Catalog and constructs a `Table` object
+ /// based on its metadata.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object that represents the loaded table.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios, including:
+ /// - Failure to validate the namespace.
+ /// - Failure to retrieve the table from the Glue Catalog.
+ /// - Absence of metadata location information in the table's properties.
+ /// - Issues reading or deserializing the table's metadata file.
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
+ db_name, table_name
+ ),
+ )),
+ Some(table) => {
+ let metadata_location = get_metadata_location(&table.parameters)?;
Review Comment:
Python implementation have some check about glue table: https://github.com/apache/iceberg-python/blob/07442cc00125ac00a91f717d9832e5479f4ff6dd/pyiceberg/catalog/glue.py#L299
But I think this is not a blocker.
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -310,31 +327,282 @@ impl Catalog for GlueCatalog {
Ok(table_list)
}
+ /// Creates a new table within a specified namespace using the provided
+ /// table creation settings.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object representing the newly created
+ /// table.
+ ///
+ /// # Errors
+ /// This function may return an error in several cases, including invalid
+ /// namespace identifiers, failure to determine a default storage location,
+ /// issues generating or writing table metadata, and errors communicating
+ /// with the Glue Catalog.
async fn create_table(
&self,
- _namespace: &NamespaceIdent,
- _creation: TableCreation,
+ namespace: &NamespaceIdent,
+ creation: TableCreation,
) -> Result<Table> {
- todo!()
+ let db_name = validate_namespace(namespace)?;
+ let table_name = creation.name.clone();
+
+ let location = match &creation.location {
+ Some(location) => location.clone(),
+ None => {
+ let ns = self.get_namespace(namespace).await?;
+ get_default_table_location(&ns, &table_name, &self.config.warehouse)
+ }
+ };
+
+ let metadata = TableMetadataBuilder::from_table_creation(creation)?.build()?;
+ let metadata_location = create_metadata_location(&location, 0)?;
+
+ let mut file = self
+ .file_io
+ .new_output(&metadata_location)?
+ .writer()
+ .await?;
+ file.write_all(&serde_json::to_vec(&metadata)?).await?;
+ file.shutdown().await?;
+
+ let glue_table = convert_to_glue_table(
+ &table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(glue_table);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(NamespaceIdent::new(db_name), table_name))
+ .build();
+
+ Ok(table)
}
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- todo!()
+ /// Loads a table from the Glue Catalog and constructs a `Table` object
+ /// based on its metadata.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object that represents the loaded table.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios, including:
+ /// - Failure to validate the namespace.
+ /// - Failure to retrieve the table from the Glue Catalog.
+ /// - Absence of metadata location information in the table's properties.
+ /// - Issues reading or deserializing the table's metadata file.
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
+ db_name, table_name
+ ),
+ )),
+ Some(table) => {
+ let metadata_location = get_metadata_location(&table.parameters)?;
+
+ let mut reader = self.file_io.new_input(&metadata_location)?.reader().await?;
+ let mut metadata_str = String::new();
+ reader.read_to_string(&mut metadata_str).await?;
+ let metadata = serde_json::from_str::<TableMetadata>(&metadata_str)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(
+ NamespaceIdent::new(db_name),
+ table_name.to_owned(),
+ ))
+ .build();
+
+ Ok(table)
+ }
+ }
}
- async fn drop_table(&self, _table: &TableIdent) -> Result<()> {
- todo!()
+ /// Asynchronously drops a table from the database.
+ ///
+ /// # Errors
+ /// Returns an error if:
+ /// - The namespace provided in `table` cannot be validated
+ /// or does not exist.
+ /// - The underlying database client encounters an error while
+ /// attempting to drop the table. This includes scenarios where
+ /// the table does not exist.
+ /// - Any network or communication error occurs with the database backend.
+ async fn drop_table(&self, table: &TableIdent) -> Result<()> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .delete_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ Ok(())
}
- async fn table_exists(&self, _table: &TableIdent) -> Result<bool> {
- todo!()
+ /// Asynchronously checks the existence of a specified table
+ /// in the database.
+ ///
+ /// # Returns
+ /// - `Ok(true)` if the table exists in the database.
+ /// - `Ok(false)` if the table does not exist in the database.
+ /// - `Err(...)` if an error occurs during the process
+ async fn table_exists(&self, table: &TableIdent) -> Result<bool> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let resp = builder.send().await;
+
+ match resp {
+ Ok(_) => Ok(true),
+ Err(err) => {
+ if err
+ .as_service_error()
+ .map(|e| e.is_entity_not_found_exception())
+ == Some(true)
+ {
+ return Ok(false);
+ }
+ Err(from_aws_sdk_error(err))
+ }
+ }
}
- async fn rename_table(&self, _src: &TableIdent, _dest: &TableIdent) -> Result<()> {
- todo!()
+ /// Asynchronously renames a table within the database
+ /// or moves it between namespaces (databases).
+ ///
+ /// # Returns
+ /// - `Ok(())` on successful rename or move of the table.
+ /// - `Err(...)` if an error occurs during the process.
+ async fn rename_table(&self, src: &TableIdent, dest: &TableIdent) -> Result<()> {
+ let src_db_name = validate_namespace(src.namespace())?;
+ let dest_db_name = validate_namespace(dest.namespace())?;
+
+ let src_table_name = src.name();
+ let dest_table_name = dest.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&src_db_name)
+ .name(src_table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
+ src_db_name, src_table_name
+ ),
+ )),
+ Some(table) => {
+ let rename_table_input = TableInput::builder()
+ .name(dest_table_name)
+ .set_parameters(table.parameters.clone())
+ .set_storage_descriptor(table.storage_descriptor.clone())
+ .set_table_type(table.table_type.clone())
+ .set_description(table.description.clone())
+ .build()
+ .map_err(from_aws_build_error)?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&dest_db_name)
+ .table_input(rename_table_input);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let drop_src_table_result = self.drop_table(src).await;
+
+ match drop_src_table_result {
Review Comment:
This looks great, thanks!
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -310,31 +327,282 @@ impl Catalog for GlueCatalog {
Ok(table_list)
}
+ /// Creates a new table within a specified namespace using the provided
+ /// table creation settings.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object representing the newly created
+ /// table.
+ ///
+ /// # Errors
+ /// This function may return an error in several cases, including invalid
+ /// namespace identifiers, failure to determine a default storage location,
+ /// issues generating or writing table metadata, and errors communicating
+ /// with the Glue Catalog.
async fn create_table(
&self,
- _namespace: &NamespaceIdent,
- _creation: TableCreation,
+ namespace: &NamespaceIdent,
+ creation: TableCreation,
) -> Result<Table> {
- todo!()
+ let db_name = validate_namespace(namespace)?;
+ let table_name = creation.name.clone();
+
+ let location = match &creation.location {
+ Some(location) => location.clone(),
+ None => {
+ let ns = self.get_namespace(namespace).await?;
+ get_default_table_location(&ns, &table_name, &self.config.warehouse)
+ }
+ };
+
+ let metadata = TableMetadataBuilder::from_table_creation(creation)?.build()?;
+ let metadata_location = create_metadata_location(&location, 0)?;
+
+ let mut file = self
+ .file_io
+ .new_output(&metadata_location)?
+ .writer()
+ .await?;
+ file.write_all(&serde_json::to_vec(&metadata)?).await?;
+ file.shutdown().await?;
+
+ let glue_table = convert_to_glue_table(
+ &table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(glue_table);
+ let builder = with_catalog_id!(builder, self.config);
+
+ builder.send().await.map_err(from_aws_sdk_error)?;
+
+ let table = Table::builder()
+ .file_io(self.file_io())
+ .metadata_location(metadata_location)
+ .metadata(metadata)
+ .identifier(TableIdent::new(NamespaceIdent::new(db_name), table_name))
+ .build();
+
+ Ok(table)
}
- async fn load_table(&self, _table: &TableIdent) -> Result<Table> {
- todo!()
+ /// Loads a table from the Glue Catalog and constructs a `Table` object
+ /// based on its metadata.
+ ///
+ /// # Returns
+ /// A `Result` wrapping a `Table` object that represents the loaded table.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios, including:
+ /// - Failure to validate the namespace.
+ /// - Failure to retrieve the table from the Glue Catalog.
+ /// - Absence of metadata location information in the table's properties.
+ /// - Issues reading or deserializing the table's metadata file.
+ async fn load_table(&self, table: &TableIdent) -> Result<Table> {
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let builder = self
+ .client
+ .0
+ .get_table()
+ .database_name(&db_name)
+ .name(table_name);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let glue_table_output = builder.send().await.map_err(from_aws_sdk_error)?;
+
+ match glue_table_output.table() {
+ None => Err(Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "'Table' object for database: {} and table: {} does not exist",
Review Comment:
```suggestion
"'Table object for database: {} and table: {} does not exist",
```
##########
crates/catalog/glue/tests/glue_catalog_test.rs:
##########
@@ -91,6 +103,166 @@ async fn set_test_namespace(fixture: &TestFixture, namespace: &NamespaceIdent) -
Ok(())
}
+fn set_table_creation(location: impl ToString, name: impl ToString) -> Result<TableCreation> {
+ let schema = Schema::builder()
+ .with_schema_id(0)
+ .with_fields(vec![
+ NestedField::required(1, "foo", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::required(2, "bar", Type::Primitive(PrimitiveType::String)).into(),
+ ])
+ .build()?;
+
+ let creation = TableCreation::builder()
+ .location(location.to_string())
+ .name(name.to_string())
+ .properties(HashMap::new())
+ .schema(schema)
+ .build();
+
+ Ok(creation)
+}
+
+#[tokio::test]
+async fn test_rename_table() -> Result<()> {
+ let fixture = set_test_fixture("test_rename_table").await;
+ let creation = set_table_creation("s3a://warehouse/hive", "my_table")?;
+ let namespace = Namespace::new(NamespaceIdent::new("my_database".into()));
+
+ fixture
+ .glue_catalog
+ .create_namespace(namespace.name(), HashMap::new())
+ .await?;
+
+ let table = fixture
+ .glue_catalog
+ .create_table(namespace.name(), creation)
+ .await?;
+
+ let dest = TableIdent::new(namespace.name().clone(), "my_table_rename".to_string());
+
+ fixture
+ .glue_catalog
+ .rename_table(table.identifier(), &dest)
+ .await?;
+
+ let table = fixture.glue_catalog.load_table(&dest).await?;
+ assert_eq!(table.identifier(), &dest);
Review Comment:
How about adding a check here to load old table name and verify that it doesn't exist?
##########
crates/catalog/glue/tests/glue_catalog_test.rs:
##########
@@ -91,6 +103,166 @@ async fn set_test_namespace(fixture: &TestFixture, namespace: &NamespaceIdent) -
Ok(())
}
+fn set_table_creation(location: impl ToString, name: impl ToString) -> Result<TableCreation> {
+ let schema = Schema::builder()
+ .with_schema_id(0)
+ .with_fields(vec![
+ NestedField::required(1, "foo", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::required(2, "bar", Type::Primitive(PrimitiveType::String)).into(),
+ ])
+ .build()?;
+
+ let creation = TableCreation::builder()
+ .location(location.to_string())
+ .name(name.to_string())
+ .properties(HashMap::new())
+ .schema(schema)
+ .build();
+
+ Ok(creation)
+}
+
+#[tokio::test]
+async fn test_rename_table() -> Result<()> {
+ let fixture = set_test_fixture("test_rename_table").await;
+ let creation = set_table_creation("s3a://warehouse/hive", "my_table")?;
+ let namespace = Namespace::new(NamespaceIdent::new("my_database".into()));
+
+ fixture
+ .glue_catalog
+ .create_namespace(namespace.name(), HashMap::new())
+ .await?;
+
+ let table = fixture
+ .glue_catalog
+ .create_table(namespace.name(), creation)
+ .await?;
+
+ let dest = TableIdent::new(namespace.name().clone(), "my_table_rename".to_string());
+
+ fixture
+ .glue_catalog
+ .rename_table(table.identifier(), &dest)
+ .await?;
+
+ let table = fixture.glue_catalog.load_table(&dest).await?;
+ assert_eq!(table.identifier(), &dest);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn test_table_exists() -> Result<()> {
+ let fixture = set_test_fixture("test_table_exists").await;
+ let creation = set_table_creation("s3a://warehouse/hive", "my_table")?;
+ let namespace = Namespace::new(NamespaceIdent::new("my_database".into()));
+
+ fixture
+ .glue_catalog
+ .create_namespace(namespace.name(), HashMap::new())
+ .await?;
Review Comment:
How about adding a check here to ensure that `table_exists` returns false?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org