You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fqaiser94 (via GitHub)" <gi...@apache.org> on 2024/07/23 15:11:36 UTC
[PR] Add in-memory catalog implementation [iceberg-rust]
fqaiser94 opened a new pull request, #475:
URL: https://github.com/apache/iceberg-rust/pull/475
Fixes: https://github.com/apache/iceberg-rust/issues/412
This PR adds an in-memory implementation of catalog.
Given that this is in-memory, it's not suitable for use in production environments. I expect that this will be used mostly for unit testing and maybe demonstration purposes.
With this in mind, the implementation prioritizes consistency over performance. For example, all catalog state is stored inside a `Mutex` and each catalog method acquires a lock before performing any operations. This makes all catalog methods thread-safe at the cost of performance. I could get more fancy and look into things like `RWLock` or `Atomic*` but I didn't feel this was necessary given the expected use cases. I am open to feedback though.
Lastly, please note that although the PR looks big, 1300 lines are just tests 🥲
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691637775
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Based on my current understanding of the catalog API: If users want to configure the `location`, they should specify it in `TableCreate`. Otherwise, the location generation logic is the implementation detail.
I'm fine if all files been located under `<namespaces>/<uuids>`. If I want to control the `root`, I can setup it in `FileIO` directly. For example, I can configure an fs `FileIO` with `root = "/tmp"`. So all generated tables will locate at `/tmp/<namespaces>/<uuids>`.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690027985
##########
crates/catalog/inmemory/README.md:
##########
@@ -0,0 +1,27 @@
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+-->
+
+# Apache Iceberg In-Memory Catalog Official Native Rust Implementation
+
+[](https://crates.io/crates/iceberg-catalog-inmemory)
+[](https://docs.rs/iceberg/latest/iceberg-catalog-inmemory/)
Review Comment:
```suggestion
[](https://docs.rs/iceberg/latest/iceberg-catalog-inmemory/)
```
##########
crates/catalog/inmemory/Cargo.toml:
##########
@@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[package]
+name = "iceberg-catalog-inmemory"
Review Comment:
nit: is using `memory` a good idea? cc @Fokko @liurenjie1024
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
Review Comment:
We can accept a `FileIO` as input for the storage they want to connect. Also, we can provide a `Default` implementation which uses in-memory file_io.
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
+ let root_namespace_state = NamespaceState::new();
+ let file_io = FileIOBuilder::new_fs_io().build()?;
Review Comment:
We can use an in-memory filesystem by default, eliminating the need to change the environment.
I will build one for file_io.
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
Review Comment:
Hi, is there a specific reason we're using `async_lock`? Could we switch to `futures::lock::Mutex` since we already depend on `futures`?
##########
crates/catalog/inmemory/src/lib.rs:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Iceberg in-memory Catalog API implementation.
+
+#![deny(missing_docs)]
+#![feature(map_try_insert)]
Review Comment:
All crates built by iceberg should be available in stable rust. So it's better to not enable nightly features.
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
+ let root_namespace_state = NamespaceState::new();
+ let file_io = FileIOBuilder::new_fs_io().build()?;
+ let inmemory_catalog = Self {
+ root_namespace_state: Mutex::new(root_namespace_state),
+ file_io,
+ };
+
+ Ok(inmemory_catalog)
+ }
+}
+
+/// Create metadata location from `location` and `version`
+fn create_metadata_location(location: impl AsRef<str>, version: i32) -> Result<String> {
+ if version < 0 {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Table metadata version: '{}' must be a non-negative integer",
+ version
+ ),
+ ))
+ } else {
+ let version = format!("{:0>5}", version);
+ let id = Uuid::new_v4();
+ let metadata_location = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ location.as_ref(),
+ version,
+ id
+ );
+
+ Ok(metadata_location)
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ 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()),
+ Some(parent_namespace_ident) => {
+ let namespaces = root_namespace_state
+ .list_namespaces_under(parent_namespace_ident)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ 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 tmp_dir = TempDir::new()?;
Review Comment:
I personally believe we should not depend on `TempDir`. As mentioned in the previous comment, the `file_io` could be memory or s3.
If `location` is empty, we can generate one for user.
##########
crates/catalog/inmemory/README.md:
##########
@@ -0,0 +1,27 @@
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+-->
+
+# Apache Iceberg In-Memory Catalog Official Native Rust Implementation
+
+[](https://crates.io/crates/iceberg-catalog-inmemory)
Review Comment:
```suggestion
[](https://crates.io/crates/iceberg-catalog-inmemory)
```
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691464398
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Apologies, I'm not understanding, could you be more specific here for me please?
Where should I be getting this `default_table_root_location` value from when I'm inside the `InMemoryCatalog.create_table` method?
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693080874
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
+ "{}/{}",
+ 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 = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ &location,
+ 0,
+ Uuid::new_v4()
+ );
+
+ 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.
+ async fn load_table(&self, table_ident: &TableIdent) -> Result<Table> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let metadata_location = root_namespace_state.get_existing_table_location(table_ident)?;
+ let input_file = self.file_io.new_input(metadata_location)?;
+ let metadata_content = input_file.read().await?;
+ let metadata = serde_json::from_slice::<TableMetadata>(&metadata_content)?;
+ let table = Table::builder()
+ .file_io(self.file_io.clone())
+ .metadata_location(metadata_location.clone())
+ .metadata(metadata)
+ .identifier(table_ident.clone())
+ .build();
+
+ Ok(table)
+ }
+
+ /// Drop a table from the catalog.
+ async fn drop_table(&self, table_ident: &TableIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_table(table_ident)
+ }
+
+ /// Check if a table exists in the catalog.
+ async fn table_exists(&self, table_ident: &TableIdent) -> Result<bool> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.table_exists(table_ident)
+ }
+
+ /// Rename a table in the catalog.
+ async fn rename_table(
+ &self,
+ src_table_ident: &TableIdent,
+ dst_table_ident: &TableIdent,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let mut new_root_namespace_state = root_namespace_state.clone();
Review Comment:
Great question!
The cloning is deliberate. The `rename_table` method requires us to perform 2 separate (i.e. non-atomic) modifications to the catalog state:
1. Remove the src table
2. Add the dst table
Either of these modifications could fail but I'm particularly concerned about the case where the second modification fails.
If we were to use a modify in-place approach, and the second modification fails, we would leave the catalog in a bad state because we would have removed the src table already.
To avoid this scenario, I'm using a "immutable-atomic-swap" style approach that modifies a copy of the catalog state first, and then swaps the pointer to this new copy if everything works successfully.
Let me know if that makes sense.
There are other ways to do this but I found this approach to be the "cleanest."
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693082893
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> How about starting a new thread instead. I think it's not a block of merging current PR.
Sounds reasonable to me.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1689983750
##########
crates/catalog/inmemory/src/lib.rs:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Iceberg in-memory Catalog API implementation.
+
+#![deny(missing_docs)]
+#![feature(map_try_insert)]
Review Comment:
We use stable rust to build this library so that downstream users will not be forced to use unstable rust.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691830743
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> For example, I can configure an fs FileIO with root = "/tmp".
CMIIW but I don't there is a way to do that today.
I had to make a bunch of changes to enable that (for unit testing) in the [latest commit](https://github.com/apache/iceberg-rust/pull/475/commits/807dd4cf649b5c367f25afc59f99341d6995c337). Please take another look.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691637775
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Based on my current understanding of the catalog API: If users want to configure the `location`, they should specify it in `TableCreate`. Otherwise, the location generation logic is the implementation detail.
I'm fine if all files been located under `<namespaces>/<uuids>`. If I want to control the `root`, I can setup it in `FileIO` directly. For example, I can configure a fs `FileIO` with `root = "/tmp"`. So all generated tables will locate at `/tmp/<namespaces>/<uuids>`.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691550143
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Ah okay, I see what you're saying now.
CMIIW though isn't that going to be a bit problematic for non-memory-FileIO usecases?
You'll end up with files at `namespace/.*` of your file-system/s3-bucket/etc.?
As opposed to with the current approach where you can specify `default_table_root_location="/tmp"` and then have tables created at`/tmp/namespace/.*` for example.
Let me know your thoughts.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691909039
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Fair, [reverted](https://github.com/apache/iceberg-rust/pull/475/commits/1d010721ee417cc38f56d7580871c2fcbf07b889).
I've also removed `default_table_root_location` from `InMemoryCatalog` and refactored the unit tests to always explicitly provide a temporary table location (i also had to remove the test that exercised the no-location-provided behaviour). If your memory-file-io PR goes in before this one, i'll just switch to that instead.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691852790
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> CMIIW but I don't see a way to do that today.
> I had to make some changes to enable that (for the unit tests) in the latest commit.
Hi, it's better not to include unrelated changes in this PR. I mean the new prop part of fs file io. The configuration should agree with all iceberg implementations.
We can discuss about the default root in another issue.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693069969
##########
crates/catalog/inmemory/src/namespace_state.rs:
##########
@@ -0,0 +1,306 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use iceberg::{Error, ErrorKind, NamespaceIdent, Result, TableIdent};
+use itertools::Itertools;
+use std::collections::{hash_map, HashMap};
+
+// Represents the state of a namespace
+#[derive(Debug, Clone, Default)]
+pub(crate) struct NamespaceState {
+ // Properties of this namespace
+ properties: HashMap<String, String>,
+ // Namespaces nested inside this namespace
+ namespaces: HashMap<String, NamespaceState>,
+ // Mapping of tables to metadata locations in this namespace
+ table_metadata_locations: HashMap<String, String>,
+}
+
+fn no_such_namespace_err<T>(namespace_ident: &NamespaceIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such namespace: {:?}", namespace_ident),
+ ))
+}
+
+fn no_such_table_err<T>(table_ident: &TableIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such table: {:?}", table_ident),
+ ))
+}
+
+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 {
Review Comment:
I understood this comment as remove the `NamespaceState::new()` method and just use `NamespaceState::default()` instead. Let me know if I misunderstood.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693113401
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
+ "{}/{}",
+ 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 = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ &location,
+ 0,
+ Uuid::new_v4()
+ );
+
+ 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.
+ async fn load_table(&self, table_ident: &TableIdent) -> Result<Table> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let metadata_location = root_namespace_state.get_existing_table_location(table_ident)?;
+ let input_file = self.file_io.new_input(metadata_location)?;
+ let metadata_content = input_file.read().await?;
+ let metadata = serde_json::from_slice::<TableMetadata>(&metadata_content)?;
+ let table = Table::builder()
+ .file_io(self.file_io.clone())
+ .metadata_location(metadata_location.clone())
+ .metadata(metadata)
+ .identifier(table_ident.clone())
+ .build();
+
+ Ok(table)
+ }
+
+ /// Drop a table from the catalog.
+ async fn drop_table(&self, table_ident: &TableIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_table(table_ident)
+ }
+
+ /// Check if a table exists in the catalog.
+ async fn table_exists(&self, table_ident: &TableIdent) -> Result<bool> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.table_exists(table_ident)
+ }
+
+ /// Rename a table in the catalog.
+ async fn rename_table(
+ &self,
+ src_table_ident: &TableIdent,
+ dst_table_ident: &TableIdent,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let mut new_root_namespace_state = root_namespace_state.clone();
Review Comment:
Thanks for this explaination, it makes sense to me! The only concern with me is that cloning maybe quite slow for large catalog, but given this memory catalog is mostly used for test and demo, I think this is acceptable.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691909039
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Fair, [reverted](https://github.com/apache/iceberg-rust/pull/475/commits/1d010721ee417cc38f56d7580871c2fcbf07b889).
I've also removed `default_table_root_location` from `InMemoryCatalog` and refactored the unit tests to always explicitly provide a temporary table location.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691830743
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> For example, I can configure an fs FileIO with root = "/tmp".
CMIIW but I don't see a way to do that today.
I had to make some changes to enable that (for the unit tests) in the [latest commit](https://github.com/apache/iceberg-rust/pull/475/commits/807dd4cf649b5c367f25afc59f99341d6995c337). Please take another look.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691550143
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Ah okay, I see what you're saying now.
CMIIW though isn't that going to be a bit problematic?
You'll end up with files at `namespace/.*` of your file-system/s3-bucket/etc.?
As opposed to with the current approach where you can specify `/tmp/namespace/.*` for example.
I'm also thinking about unit-tests here, I don't want random files littering my hard drive.
Let me know your thoughts.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693080828
##########
crates/catalog/inmemory/src/namespace_state.rs:
##########
@@ -0,0 +1,306 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use iceberg::{Error, ErrorKind, NamespaceIdent, Result, TableIdent};
+use itertools::Itertools;
+use std::collections::{hash_map, HashMap};
+
+// Represents the state of a namespace
+#[derive(Debug, Clone, Default)]
+pub(crate) struct NamespaceState {
+ // Properties of this namespace
+ properties: HashMap<String, String>,
+ // Namespaces nested inside this namespace
+ namespaces: HashMap<String, NamespaceState>,
+ // Mapping of tables to metadata locations in this namespace
+ table_metadata_locations: HashMap<String, String>,
+}
+
+fn no_such_namespace_err<T>(namespace_ident: &NamespaceIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such namespace: {:?}", namespace_ident),
+ ))
+}
+
+fn no_such_table_err<T>(table_ident: &TableIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such table: {:?}", table_ident),
+ ))
+}
+
+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 {
Review Comment:
Yes, exactly! Thanks!
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1689983750
##########
crates/catalog/inmemory/src/lib.rs:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Iceberg in-memory Catalog API implementation.
+
+#![deny(missing_docs)]
+#![feature(map_try_insert)]
Review Comment:
We use stable rust to build this library so that downstream users will not be forced to use unstable rust.
##########
crates/catalog/inmemory/src/namespace_state.rs:
##########
@@ -0,0 +1,306 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use iceberg::{Error, ErrorKind, NamespaceIdent, Result, TableIdent};
+use itertools::Itertools;
+use std::collections::{hash_map, HashMap};
+
+// Represents the state of a namespace
+#[derive(Debug, Clone, Default)]
+pub(crate) struct NamespaceState {
+ // Properties of this namespace
+ properties: HashMap<String, String>,
+ // Namespaces nested inside this namespace
+ namespaces: HashMap<String, NamespaceState>,
+ // Mapping of tables to metadata locations in this namespace
+ table_metadata_locations: HashMap<String, String>,
+}
+
+fn no_such_namespace_err<T>(namespace_ident: &NamespaceIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such namespace: {:?}", namespace_ident),
+ ))
+}
+
+fn no_such_table_err<T>(table_ident: &TableIdent) -> Result<T> {
+ Err(Error::new(
+ ErrorKind::Unexpected,
+ format!("No such table: {:?}", table_ident),
+ ))
+}
+
+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 {
Review Comment:
nit: We can implement `Default` for it.
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
Review Comment:
```suggestion
/// List namespaces inside the catalog.
```
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -35,7 +35,7 @@ use uuid::Uuid;
/// The catalog API for Iceberg Rust.
#[async_trait]
pub trait Catalog: Debug + Sync + Send {
- /// List namespaces from table.
+ /// List namespaces inside the Catalog.
Review Comment:
```suggestion
/// List namespaces inside the catalog.
```
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
Review Comment:
I think as other catalogs, we should allow user to config a path as root of warehouse, and we create tables under this root path?
##########
crates/catalog/inmemory/Cargo.toml:
##########
@@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[package]
+name = "iceberg-catalog-inmemory"
Review Comment:
+1
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
+ "{}/{}",
+ 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 = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ &location,
+ 0,
+ Uuid::new_v4()
+ );
+
+ 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.
+ async fn load_table(&self, table_ident: &TableIdent) -> Result<Table> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let metadata_location = root_namespace_state.get_existing_table_location(table_ident)?;
+ let input_file = self.file_io.new_input(metadata_location)?;
+ let metadata_content = input_file.read().await?;
+ let metadata = serde_json::from_slice::<TableMetadata>(&metadata_content)?;
+ let table = Table::builder()
+ .file_io(self.file_io.clone())
+ .metadata_location(metadata_location.clone())
+ .metadata(metadata)
+ .identifier(table_ident.clone())
+ .build();
+
+ Ok(table)
+ }
+
+ /// Drop a table from the catalog.
+ async fn drop_table(&self, table_ident: &TableIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_table(table_ident)
+ }
+
+ /// Check if a table exists in the catalog.
+ async fn table_exists(&self, table_ident: &TableIdent) -> Result<bool> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.table_exists(table_ident)
+ }
+
+ /// Rename a table in the catalog.
+ async fn rename_table(
+ &self,
+ src_table_ident: &TableIdent,
+ dst_table_ident: &TableIdent,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let mut new_root_namespace_state = root_namespace_state.clone();
Review Comment:
Why we need to clone this? Why not just modify it in place?
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690620132
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
Review Comment:
I wasn't aware of `futures`/`futures::lock::Mutex` 🙂
The only difference I can see between the two is that `async_lock::Mutex` [docs](https://docs.rs/async-lock/latest/async_lock/struct.Mutex.html) state that:
```
The locking mechanism uses eventual fairness to ensure locking will be fair on average without sacrificing performance. This is done by forcing a fair lock whenever a lock operation is starved for longer than 0.5 milliseconds.
```
Whereas `futures::lock::Mutex` [docs](https://docs.rs/futures/latest/futures/lock/struct.Mutex.html#fairness) state that:
```
This mutex provides no fairness guarantees. Tasks may not acquire the mutex in the order that they requested the lock, and it’s possible for a single task which repeatedly takes the lock to starve other tasks, which may be left waiting indefinitely.
```
Regardless, I've switched to `futures::lock::Mutex` since I don't think this should be an issue (and we can always re-evaluate in the future if it does become one).
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690627063
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
+ let root_namespace_state = NamespaceState::new();
+ let file_io = FileIOBuilder::new_fs_io().build()?;
Review Comment:
Sounds good, I see you already filed a ticket [here](https://github.com/apache/iceberg-rust/issues/477) and I don't think that should be a blocker 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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691852790
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> CMIIW but I don't see a way to do that today.
> I had to make some changes to enable that (for the unit tests) in the latest commit.
Hi, it's better not to include unrelated changes in this PR. I mean the new prop part of fs file io. The configuration should agree with all iceberg implementations.
We can discuss about the default root configuration in another issue.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691464398
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Apologies, I'm struggling to implement your suggestion, could you be more specific please?
Where should I be getting this `default_table_root_location` value from when I'm inside the `InMemoryCatalog.create_table` method?
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693073031
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
I have a concern that `root` is not a standard configuration of `FileIO` in java/python implementations?
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693080874
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
+ "{}/{}",
+ 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 = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ &location,
+ 0,
+ Uuid::new_v4()
+ );
+
+ 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.
+ async fn load_table(&self, table_ident: &TableIdent) -> Result<Table> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let metadata_location = root_namespace_state.get_existing_table_location(table_ident)?;
+ let input_file = self.file_io.new_input(metadata_location)?;
+ let metadata_content = input_file.read().await?;
+ let metadata = serde_json::from_slice::<TableMetadata>(&metadata_content)?;
+ let table = Table::builder()
+ .file_io(self.file_io.clone())
+ .metadata_location(metadata_location.clone())
+ .metadata(metadata)
+ .identifier(table_ident.clone())
+ .build();
+
+ Ok(table)
+ }
+
+ /// Drop a table from the catalog.
+ async fn drop_table(&self, table_ident: &TableIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_table(table_ident)
+ }
+
+ /// Check if a table exists in the catalog.
+ async fn table_exists(&self, table_ident: &TableIdent) -> Result<bool> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.table_exists(table_ident)
+ }
+
+ /// Rename a table in the catalog.
+ async fn rename_table(
+ &self,
+ src_table_ident: &TableIdent,
+ dst_table_ident: &TableIdent,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let mut new_root_namespace_state = root_namespace_state.clone();
Review Comment:
Great question!
The cloning is deliberate. The `rename_table` method requires us to perform 2 separate (i.e. non-atomic) modifications to the catalog state:
1. Remove the src table
2. Add the dst table
Either of these modifications could fail but I'm particularly concerned about the case where the second modification fails.
If we were to use a modify in-place approach, and the second modification fails, we would leave the catalog in a bad state because we would have removed the src table already.
To avoid this scenario, I'm using an "atomic-swap" style approach: modify a copy of the catalog state first, and then swap the pointer to point to this new copy if everything works successfully.
Let me know if that makes sense.
There are other ways to do this but I found this approach to be the "cleanest."
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690026333
##########
crates/catalog/inmemory/Cargo.toml:
##########
@@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[package]
+name = "iceberg-catalog-inmemory"
Review Comment:
nit: is using `iceberg-catalog-memory` a good idea? cc @Fokko @liurenjie1024
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691932105
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Thanks a lot! I think this PR can merge as is and I'm willing to do the changes.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691852790
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> CMIIW but I don't see a way to do that today.
> I had to make some changes to enable that (for the unit tests) in the latest commit.
Hi, it's better not to include unrelated changes in this PR. We can discuss about the default root in another issue.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#issuecomment-2249346483
Request review from @Fokko and @liurenjie1024 for another eyes.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693068061
##########
crates/catalog/inmemory/Cargo.toml:
##########
@@ -0,0 +1,41 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[package]
+name = "iceberg-catalog-inmemory"
Review Comment:
Changed all "in-memory" references to just "memory"
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693048009
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
Review Comment:
We have a discussion thread at https://github.com/apache/iceberg-rust/pull/475#discussion_r1690861564.
Maybe we can move this part into file io so that catalog don't need to handle root everytime?
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690621507
##########
crates/catalog/inmemory/src/lib.rs:
##########
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Iceberg in-memory Catalog API implementation.
+
+#![deny(missing_docs)]
+#![feature(map_try_insert)]
Review Comment:
Fixed.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690616688
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
+ let root_namespace_state = NamespaceState::new();
+ let file_io = FileIOBuilder::new_fs_io().build()?;
+ let inmemory_catalog = Self {
+ root_namespace_state: Mutex::new(root_namespace_state),
+ file_io,
+ };
+
+ Ok(inmemory_catalog)
+ }
+}
+
+/// Create metadata location from `location` and `version`
+fn create_metadata_location(location: impl AsRef<str>, version: i32) -> Result<String> {
+ if version < 0 {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Table metadata version: '{}' must be a non-negative integer",
+ version
+ ),
+ ))
+ } else {
+ let version = format!("{:0>5}", version);
+ let id = Uuid::new_v4();
+ let metadata_location = format!(
+ "{}/metadata/{}-{}.metadata.json",
+ location.as_ref(),
+ version,
+ id
+ );
+
+ Ok(metadata_location)
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ 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()),
+ Some(parent_namespace_ident) => {
+ let namespaces = root_namespace_state
+ .list_namespaces_under(parent_namespace_ident)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ 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 tmp_dir = TempDir::new()?;
Review Comment:
Agreed; if we're going to accept arbitrary `FileIO` instances, it doesn't make sense to use `TempDir` anymore here. I've refactored the code so that you can configure a default table location when you initialize the `InMemoryCatalog` that will be used if no location is provided in `TableCreation` instances.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691483403
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
I think we don't need to put `default_table_root_location` in `create_table`. Just create with given namespaces like:
```diff
let location = format!(
- "{}/{}/{}",
- self.default_table_root_location,
+ "{}/{}",
table_ident.namespace().join("/"),
table_ident.name()
);
```
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691550143
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Ah okay, I see what you're saying now.
CMIIW though isn't that going to be a bit problematic for non-memory-FileIO usecases?
You'll end up with files at `namespace/.*` of your file-system/s3-bucket/etc.?
As opposed to with the current approach where you can specify `/tmp/namespace/.*` for example.
Let me know your thoughts.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691909039
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Fair, [reverted](https://github.com/apache/iceberg-rust/pull/475/commits/1d010721ee417cc38f56d7580871c2fcbf07b889).
I've also removed `default_table_root_location` from `InMemoryCatalog` and refactored the unit tests to always explicitly provide a temporary table location. If your memory-file-io PR goes in before this one, i'll just switch to that instead.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691637775
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Based on my current understanding of the catalog API: If users want to configure the `location`, they should specify it in `TableCreate`. Otherwise, the location generation logic is the implementation detail.
I'm fine if all files been located under `<namespaces>/<uuids>`. If I want to control the `root`, I can setup it in `FileIO` directly.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691550143
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Ah okay, I see what you're saying now.
CMIIW though isn't that going to be a bit problematic?
You'll end up with files at `namespace/.*` of your file-system/s3-bucket/etc.?
As opposed to with the current approach where you can specify `/tmp/namespace/.*` for example.
I'm also thinking about unit-tests here, I don't want random files littering my hard drive (unless I write a specialized FileIO that only writes to `/tmp` but that only solves the problem for unit tests).
Let me know your thoughts.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690861564
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
`default_table_root_location` is part of `file_io` config, so we don't need to set this at catalog level.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691550143
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Ah okay, I see what you're saying now.
CMIIW though isn't that going to be a bit problematic?
You'll end up with files at `namespace/.*` of your file-system/s3-bucket/etc.?
As opposed to with the current approach where you can specify `/tmp/namespace/.*` for example.
I'm also thinking about unit-tests here, I don't want random files littering my hard drive (unless I write a specialized `FileIO` that only writes to `/tmp` but that only solves the problem for unit tests).
Let me know your thoughts.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691830743
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
> For example, I can configure an fs FileIO with root = "/tmp".
CMIIW but I don't there is a way to do that today.
I had to make a bunch of changes to enable that in the [latest commit](https://github.com/apache/iceberg-rust/pull/475/commits/807dd4cf649b5c367f25afc59f99341d6995c337). Please take another look.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1691991685
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
Sounds good.
For posterity's sake, it would basically be just this [commit](https://github.com/apache/iceberg-rust/pull/475/commits/d778682361af6bd4fc2dcd3827736f1504fb5e54) to revert.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693069613
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1466 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new(file_io: FileIO) -> Self {
+ Self {
+ root_namespace_state: Mutex::new(NamespaceState::new()),
+ file_io,
+ }
+ }
+}
+
+#[async_trait]
+impl Catalog for InMemoryCatalog {
+ /// List namespaces inside the Catalog.
+ async fn list_namespaces(
+ &self,
+ maybe_parent: Option<&NamespaceIdent>,
+ ) -> Result<Vec<NamespaceIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ match maybe_parent {
+ 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)?
+ .into_iter()
+ .map(|name| NamespaceIdent::new(name.to_string()))
+ .collect_vec();
+
+ Ok(namespaces)
+ }
+ }
+ }
+
+ /// Create a new namespace inside the catalog.
+ async fn create_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<Namespace> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.insert_new_namespace(namespace_ident, properties.clone())?;
+ let namespace = Namespace::with_properties(namespace_ident.clone(), properties);
+
+ Ok(namespace)
+ }
+
+ /// Get a namespace information from the catalog.
+ async fn get_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<Namespace> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let namespace = Namespace::with_properties(
+ namespace_ident.clone(),
+ root_namespace_state
+ .get_properties(namespace_ident)?
+ .clone(),
+ );
+
+ Ok(namespace)
+ }
+
+ /// Check if namespace exists in catalog.
+ async fn namespace_exists(&self, namespace_ident: &NamespaceIdent) -> Result<bool> {
+ let guarded_namespaces = self.root_namespace_state.lock().await;
+
+ Ok(guarded_namespaces.namespace_exists(namespace_ident))
+ }
+
+ /// Update a namespace inside the catalog.
+ ///
+ /// # Behavior
+ ///
+ /// The properties must be the full set of namespace.
+ async fn update_namespace(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ properties: HashMap<String, String>,
+ ) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.replace_properties(namespace_ident, properties)
+ }
+
+ /// Drop a namespace from the catalog.
+ async fn drop_namespace(&self, namespace_ident: &NamespaceIdent) -> Result<()> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ root_namespace_state.remove_existing_namespace(namespace_ident)
+ }
+
+ /// List tables from namespace.
+ async fn list_tables(&self, namespace_ident: &NamespaceIdent) -> Result<Vec<TableIdent>> {
+ let root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_names = root_namespace_state.list_tables(namespace_ident)?;
+ let table_idents = table_names
+ .into_iter()
+ .map(|table_name| TableIdent::new(namespace_ident.clone(), table_name.clone()))
+ .collect_vec();
+
+ Ok(table_idents)
+ }
+
+ /// Create a new table inside the namespace.
+ async fn create_table(
+ &self,
+ namespace_ident: &NamespaceIdent,
+ table_creation: TableCreation,
+ ) -> Result<Table> {
+ let mut root_namespace_state = self.root_namespace_state.lock().await;
+
+ let table_name = table_creation.name.clone();
+ let table_ident = TableIdent::new(namespace_ident.clone(), table_name);
+
+ let (table_creation, location) = match table_creation.location.clone() {
+ Some(location) => (table_creation, location),
+ None => {
+ let location = format!(
Review Comment:
Let's continue to discuss in that thread.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1693076550
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
How about starting a new thread instead. I think it's not a block of merge current PR.
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1496 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use futures::lock::Mutex;
+use iceberg::io::FileIO;
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+ default_table_root_location: String,
Review Comment:
How about starting a new thread instead. I think it's not a block of merging current 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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690614275
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
+use iceberg::io::{FileIO, FileIOBuilder};
+use iceberg::spec::{TableMetadata, TableMetadataBuilder};
+use itertools::Itertools;
+use std::collections::HashMap;
+use uuid::Uuid;
+
+use async_trait::async_trait;
+
+use iceberg::table::Table;
+use iceberg::Result;
+use iceberg::{
+ Catalog, Error, ErrorKind, Namespace, NamespaceIdent, TableCommit, TableCreation, TableIdent,
+};
+use tempfile::TempDir;
+
+use crate::namespace_state::NamespaceState;
+
+/// In-memory catalog implementation.
+#[derive(Debug)]
+pub struct InMemoryCatalog {
+ root_namespace_state: Mutex<NamespaceState>,
+ file_io: FileIO,
+}
+
+impl InMemoryCatalog {
+ /// Creates an in-memory catalog.
+ pub fn new() -> Result<Self> {
Review Comment:
Added a parameter to accept a `FileIO`.
I don't think I can make this `#[derive(Default)]` until the in-memory `FileIO` implementation becomes available.
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on code in PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#discussion_r1690620132
##########
crates/catalog/inmemory/src/catalog.rs:
##########
@@ -0,0 +1,1511 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! This module contains in-memory catalog implementation.
+
+use async_lock::Mutex;
Review Comment:
I wasn't aware of `futures`/`futures::lock::Mutex` 🙂
The only difference I can see between the two is that `async_lock::Mutex` docs state that:
```
The locking mechanism uses eventual fairness to ensure locking will be fair on average without sacrificing performance. This is done by forcing a fair lock whenever a lock operation is starved for longer than 0.5 milliseconds.
```
Whereas `futures::lock::Mutex` docs state that:
```
This mutex provides no fairness guarantees. Tasks may not acquire the mutex in the order that they requested the lock, and it’s possible for a single task which repeatedly takes the lock to starve other tasks, which may be left waiting indefinitely.
```
Regardless, I've switched to `futures::lock::Mutex` since I don't think this should be an issue (and we can always re-evaluate in the future if it does become one).
--
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] Add in-memory catalog implementation [iceberg-rust]
Posted by "fqaiser94 (via GitHub)" <gi...@apache.org>.
fqaiser94 commented on PR #475:
URL: https://github.com/apache/iceberg-rust/pull/475#issuecomment-2249126369
@Xuanwo just FYI, all comments besides the [package-naming](https://github.com/apache/iceberg-rust/pull/475#discussion_r1690026333) discussion should be addressed now
--
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