wgtmac commented on code in PR #80:
URL: https://github.com/apache/iceberg-cpp/pull/80#discussion_r2059433981
##########
src/iceberg/CMakeLists.txt:
##########
@@ -19,6 +19,7 @@ set(ICEBERG_INCLUDES
"$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src>"
"$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>")
set(ICEBERG_SOURCES
arrow_c_data_internal.cc
+ catalog/memory_catalog.cc
Review Comment:
Rename it to `in_memory_catalog.cc`
##########
src/iceberg/catalog/memory_catalog.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <mutex>
+#include <optional>
+#include <unordered_map>
+
+#include "iceberg/catalog.h"
+
+namespace iceberg {
+
+class InMemoryNamespace;
+
+class ICEBERG_EXPORT InMemoryCatalog : public Catalog {
+ public:
+ InMemoryCatalog(std::shared_ptr<FileIO> file_io, std::string
warehouse_location);
+
+ void Initialize(
Review Comment:
The Java impl provides a default constructor to facilitate creating instance
via reflection. That's why it has a `initialize` method. Perhaps we can remove
it and move these parameters to the constructor?
##########
src/iceberg/catalog.h:
##########
@@ -90,8 +90,11 @@ class ICEBERG_EXPORT Catalog {
/// \brief Check whether table exists
///
/// \param identifier a table identifier
- /// \return true if the table exists, false otherwise
- virtual bool TableExists(const TableIdentifier& identifier) const = 0;
+ /// \return Status indicating success or failure.
+ /// - On success, the table existence was successfully checked
(actual existence
+ /// may be inferred elsewhere).
+ /// - On failure, contains error information.
+ virtual Status TableExists(const TableIdentifier& identifier) const = 0;
Review Comment:
What about `Result<bool>`? I don't think it is an error when table does not
exist. Same for below.
##########
src/iceberg/catalog/memory_catalog.h:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <mutex>
+#include <optional>
+#include <unordered_map>
+
+#include "iceberg/catalog.h"
+
+namespace iceberg {
+
+class InMemoryNamespace;
+
+class ICEBERG_EXPORT InMemoryCatalog : public Catalog {
+ public:
+ InMemoryCatalog(std::shared_ptr<FileIO> file_io, std::string
warehouse_location);
+
+ void Initialize(
+ const std::string& name,
+ const std::unordered_map<std::string, std::string>& properties) override;
+
+ std::string_view name() const override;
+
+ Result<std::vector<TableIdentifier>> ListTables(const Namespace& ns) const
override;
+
+ Result<std::unique_ptr<Table>> CreateTable(
+ const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
+ const std::string& location,
+ const std::unordered_map<std::string, std::string>& properties) override;
+
+ Result<std::unique_ptr<Table>> UpdateTable(
+ const TableIdentifier& identifier,
+ const std::vector<std::unique_ptr<UpdateRequirement>>& requirements,
+ const std::vector<std::unique_ptr<MetadataUpdate>>& updates) override;
+
+ Result<std::shared_ptr<Transaction>> StageCreateTable(
+ const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
+ const std::string& location,
+ const std::unordered_map<std::string, std::string>& properties) override;
+
+ Status TableExists(const TableIdentifier& identifier) const override;
+
+ Status DropTable(const TableIdentifier& identifier, bool purge) override;
+
+ Result<std::shared_ptr<Table>> LoadTable(
+ const TableIdentifier& identifier) const override;
+
+ Result<std::shared_ptr<Table>> RegisterTable(
+ const TableIdentifier& identifier,
+ const std::string& metadata_file_location) override;
+
+ std::unique_ptr<iceberg::TableBuilder> BuildTable(const TableIdentifier&
identifier,
+ const Schema& schema)
const override;
+
+ private:
Review Comment:
nit: what about using `pimpl` idiom to hide the implementation detail like
`InMemoryNamespace` and reduce compile time?
```
class ICEBERG_EXPORT InMemoryCatalog : public Catalog {
public:
...
private:
std::unique_ptr<class InMemoryCatalogImpl> impl_;
};
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
