Add catalog test suite to unify catalog's behavior. #2131
Conversation
blackmwk
requested review from
CTTY,
kevinjqliu and
sdd
and removed request for
kevinjqliu
| let metadata_location = root_namespace_state.remove_existing_table(table_ident)?; | ||
| self.file_io.delete(&metadata_location).await | ||
| root_namespace_state.remove_existing_table(table_ident)?; | ||
| Ok(()) |
There was a problem hiding this comment.
I just realized we don't have a catalog API to purge table in iceberg-rust?
Probably need to add this API to better define the expected behavior
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_register_table() -> Result<()> { |
There was a problem hiding this comment.
I think we can remove this? This is already covered by table_register_suite
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_register_table() { |
There was a problem hiding this comment.
Same here, this is already covered by table_register_suite
| ) -> Result<Namespace> { | ||
| if self.namespace_exists(namespace).await? { | ||
| return Err(Error::new( | ||
| ErrorKind::DataInvalid, |
There was a problem hiding this comment.
This should be ErrorKind::NamespaceAlreadyExists
| ) -> Result<()> { | ||
| if !self.namespace_exists(namespace).await? { | ||
| return Err(Error::new( | ||
| ErrorKind::DataInvalid, |
| let db_name = validate_namespace(table.namespace())?; | ||
| if !self.namespace_exists(table.namespace()).await? { | ||
| return Err(Error::new( | ||
| ErrorKind::DataInvalid, |
| )); | ||
| } | ||
| if !self.table_exists(table).await? { | ||
| return Err(Error::new(ErrorKind::DataInvalid, "Table does not exist")); |
There was a problem hiding this comment.
This should be TableNotFound
|
|
||
| cleanup_namespace_dyn(catalog.as_ref(), &namespace).await; | ||
|
|
||
| assert!(catalog.get_namespace(&namespace).await.is_err()); |
There was a problem hiding this comment.
I think this is a good opportunity for us to enforce the correct error type across catalogs. Instead of assert_err, we can do something like assert_eq!(err, ErrorKind::xxx);
| catalog | ||
| .create_namespace(&namespace, HashMap::new()) | ||
| .await | ||
| .is_err() |
There was a problem hiding this comment.
Same here, we can assert exact error type
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_list_namespaces_returns_only_top_level_namespaces() { |
There was a problem hiding this comment.
Should we migrate these tests as well?
4 participants
Which issue does this PR close?
What changes are included in this PR?
In this pr we introduced catalog test suite in catalog-loader, which could unify the behavior of catalogs.
Are these changes tested?
Yes.