Minor clean up

apache · Jul 25, 2024 · 9258ddf · 9258ddf
1 parent cedf7cc
commit 9258ddf
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 70 deletions.
diff --git a/crates/catalog/inmemory/src/catalog.rs b/crates/catalog/inmemory/src/catalog.rs
@@ -87,11 +87,15 @@ impl Catalog for InMemoryCatalog {
         let root_namespace_state = self.root_namespace_state.lock().await;
 
         match maybe_parent {
-            None => Ok(root_namespace_state
-                .list_top_level_namespaces()
-                .into_iter()
-                .map(|str| NamespaceIdent::new(str.to_string()))
-                .collect_vec()),
+            None => {
+                let namespaces = root_namespace_state
+                    .list_top_level_namespaces()
+                    .into_iter()
+                    .map(|str| NamespaceIdent::new(str.to_string()))
+                    .collect_vec();
+
+                Ok(namespaces)
+            }
             Some(parent_namespace_ident) => {
                 let namespaces = root_namespace_state
                     .list_namespaces_under(parent_namespace_ident)?
@@ -184,55 +188,44 @@ impl Catalog for InMemoryCatalog {
 
         let table_name = table_creation.name.clone();
         let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
-        let table_exists = root_namespace_state.table_exists(&table_ident)?;
-
-        if table_exists {
-            Err(Error::new(
-                ErrorKind::Unexpected,
-                format!(
-                    "Cannot create table {:?}. Table already exists",
-                    &table_ident
-                ),
-            ))
-        } else {
-            let (table_creation, location) = match table_creation.location.clone() {
-                Some(location) => (table_creation, location),
-                None => {
-                    let location = format!(
-                        "{}/{}/{}",
-                        self.default_table_root_location,
-                        table_ident.namespace().join("/"),
-                        table_ident.name()
-                    );
-
-                    let new_table_creation = TableCreation {
-                        location: Some(location.clone()),
-                        ..table_creation
-                    };
-
-                    (new_table_creation, location)
-                }
-            };
-
-            let metadata = TableMetadataBuilder::from_table_creation(table_creation)?.build()?;
-            let metadata_location = create_metadata_location(&location, 0)?;
-
-            self.file_io
-                .new_output(&metadata_location)?
-                .write(serde_json::to_vec(&metadata)?.into())
-                .await?;
-
-            root_namespace_state.insert_new_table(&table_ident, metadata_location.clone())?;
-
-            let table = Table::builder()
-                .file_io(self.file_io.clone())
-                .metadata_location(metadata_location)
-                .metadata(metadata)
-                .identifier(table_ident)
-                .build();
-
-            Ok(table)
-        }
+
+        let (table_creation, location) = match table_creation.location.clone() {
+            Some(location) => (table_creation, location),
+            None => {
+                let location = format!(
+                    "{}/{}/{}",
+                    self.default_table_root_location,
+                    table_ident.namespace().join("/"),
+                    table_ident.name()
+                );
+
+                let new_table_creation = TableCreation {
+                    location: Some(location.clone()),
+                    ..table_creation
+                };
+
+                (new_table_creation, location)
+            }
+        };
+
+        let metadata = TableMetadataBuilder::from_table_creation(table_creation)?.build()?;
+        let metadata_location = create_metadata_location(&location, 0)?;
+
+        self.file_io
+            .new_output(&metadata_location)?
+            .write(serde_json::to_vec(&metadata)?.into())
+            .await?;
+
+        root_namespace_state.insert_new_table(&table_ident, metadata_location.clone())?;
+
+        let table = Table::builder()
+            .file_io(self.file_io.clone())
+            .metadata_location(metadata_location)
+            .metadata(metadata)
+            .identifier(table_ident)
+            .build();
+
+        Ok(table)
     }
 
     /// Load table from the catalog.
@@ -282,6 +275,7 @@ impl Catalog for InMemoryCatalog {
         new_root_namespace_state.remove_existing_table(src_table_ident)?;
         new_root_namespace_state.insert_new_table(dst_table_ident, metadata_location)?;
         *root_namespace_state = new_root_namespace_state;
+
         Ok(())
     }
 
@@ -1092,7 +1086,7 @@ mod tests {
                 .unwrap_err()
                 .to_string(),
             format!(
-                "Unexpected => Cannot create table {:?}. Table already exists",
+                "Unexpected => Cannot create table {:?}. Table already exists.",
                 &table_ident
             )
         );
@@ -1513,7 +1507,7 @@ mod tests {
                 .unwrap_err()
                 .to_string(),
             format!(
-                "Unexpected => Cannot insert table {:? }. Table already exists.",
+                "Unexpected => Cannot create table {:? }. Table already exists.",
                 &dst_table_ident
             ),
         );

diff --git a/crates/catalog/inmemory/src/namespace_state.rs b/crates/catalog/inmemory/src/namespace_state.rs
@@ -44,6 +44,26 @@ fn no_such_table_err<T>(table_ident: &TableIdent) -> Result<T> {
     ))
 }
 
+fn namespace_already_exists_err<T>(namespace_ident: &NamespaceIdent) -> Result<T> {
+    Err(Error::new(
+        ErrorKind::Unexpected,
+        format!(
+            "Cannot create namespace {:?}. Namespace already exists",
+            namespace_ident
+        ),
+    ))
+}
+
+fn table_already_exists_err<T>(table_ident: &TableIdent) -> Result<T> {
+    Err(Error::new(
+        ErrorKind::Unexpected,
+        format!(
+            "Cannot create table {:?}. Table already exists.",
+            table_ident
+        ),
+    ))
+}
+
 impl NamespaceState {
     // Creates a new namespace state
     pub(crate) fn new() -> Self {
@@ -157,19 +177,14 @@ impl NamespaceState {
             .namespaces
             .entry(child_namespace_name)
         {
-            hash_map::Entry::Occupied(_) => Err(Error::new(
-                ErrorKind::Unexpected,
-                format!(
-                    "Cannot create namespace {:?}. Namespace already exists",
-                    namespace_ident
-                ),
-            )),
+            hash_map::Entry::Occupied(_) => namespace_already_exists_err(namespace_ident),
             hash_map::Entry::Vacant(entry) => {
                 let _ = entry.insert(NamespaceState {
                     properties,
                     namespaces: HashMap::new(),
                     table_metadata_locations: HashMap::new(),
                 });
+
                 Ok(())
             }
         }
@@ -220,6 +235,7 @@ impl NamespaceState {
     ) -> Result<()> {
         let properties = self.get_mut_properties(namespace_ident)?;
         *properties = new_properties;
+
         Ok(())
     }
 
@@ -266,15 +282,10 @@ impl NamespaceState {
             .table_metadata_locations
             .entry(table_ident.name().to_string())
         {
-            hash_map::Entry::Occupied(_) => Err(Error::new(
-                ErrorKind::Unexpected,
-                format!(
-                    "Cannot insert table {:?}. Table already exists.",
-                    table_ident
-                ),
-            )),
+            hash_map::Entry::Occupied(_) => table_already_exists_err(table_ident),
             hash_map::Entry::Vacant(entry) => {
                 let _ = entry.insert(metadata_location);
+
                 Ok(())
             }
         }