feat: implement initial MemoryCatalog functionality with namespace and table support #80

gty404 · 2025-04-17T07:03:06Z

Added NamespaceContainer class to manage namespace hierarchy and table metadata
Implemented MemoryCatalog methods: ListTables, TableExists, DropTable, RegisterTable

src/iceberg/catalog/memory_catalog.cc

src/iceberg/catalog/memory_catalog.h

wgtmac · 2025-04-18T03:37:14Z

src/iceberg/catalog/memory_catalog.h

+   *                                the children of the root are returned.
+   * @return A vector of child namespace names.
+   */
+  std::vector<std::string> ListChildrenNamespaces(


Why not return std::vector<Namespace>? Does it return next level or all levels?

It is returning the namespace of the next level, returning std::string instead of Namespace, I think this makes it easier to get the name.

src/iceberg/catalog/memory_catalog.h

src/iceberg/catalog/memory_catalog.cc

Fokko · 2025-04-20T19:08:12Z

I know that you already took the time and effort to write this, but I did want to put this out here. In PyIceberg we've removed the MemoryCatalog from the codebase, mostly because the SQLCatalog (JDBCCatalog in Java) was proven to be much better. The SQLCatalog also allows running in-memory using a SQLite in-memory instance, but also provides the option to easily keep the state after the process terminates. Next to that, it also provides locking out of the box.

wgtmac · 2025-04-21T02:28:22Z

@Fokko AFAIK, the C++ ecosystem does not yet have a good ORM framework so it might be challenging to implement a SqlCatalog at the moment. @gty404 is an expert on this and he may have insight in it.

gty404 · 2025-04-21T14:20:17Z

@Fokko When implementing JDBCCatalog, ORM and SQLite need to be introduced, as @wgtmac also mentioned. It's difficult to find a suitable ORM framework in the C++ system, which might be due to the license or ease of use.

src/iceberg/catalog/memory_catalog.h

Fokko · 2025-04-23T04:46:39Z

@gty404 @wgtmac I see, got it. Thanks for considering it, then the InMemoryCatalog is the next best option 👍

gty404 · 2025-04-24T11:00:22Z

@yingcai-cy @wgtmac @lidavidm @zhjwpku Could you help me review it again?

zhjwpku

I have two minor review comments, other than that, LGTM.

src/iceberg/catalog/memory_catalog.cc

src/iceberg/catalog/memory_catalog.h

src/iceberg/CMakeLists.txt

src/iceberg/catalog.h

wgtmac · 2025-04-25T01:49:55Z

src/iceberg/catalog/memory_catalog.h

+  std::unique_ptr<iceberg::TableBuilder> BuildTable(const TableIdentifier& identifier,
+                                                    const Schema& schema) const override;
+
+ private:


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_; };

How about placing InMemoryCatalogImpl and InMemoryNamespace in another header file? Their unit tests are also easy to add.

Why unit tests are required to have access to InMemoryCatalogImpl? Isn't the Catalog interface enough to test?

Don't InMemoryCatalogImpl and InMemoryNamespace require corresponding unit tests? The interfaces provided by these two may not be completely covered by the InMemoryCatalog test

Yes, that's reasonable.

src/iceberg/catalog/memory_catalog.h

wgtmac · 2025-04-25T06:53:53Z

src/iceberg/catalog/memory_catalog.h

+   *                                the children of the root are returned.
+   * \return A vector of child namespace names.
+   */
+  Result<std::vector<std::string>> ListChildrenNamespaces(


nit: use std::span

The returned Namespace list is a temporary variable.

wgtmac · 2025-04-25T07:10:11Z

src/iceberg/catalog/memory_catalog.h

+ * tables, and child namespaces. This structure enables a tree-like representation
+ * of nested namespaces.
+ */
+class ICEBERG_EXPORT InMemoryNamespace {


I was simply mirroring the Java impl when I added the Catalog without notice of SupportsNamespace: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java. Does it make sense to add all interfaces from SupportsNamespace to our Catalog API? iceberg-python did the same thing: https://github.com/apache/iceberg-python/blob/main/pyiceberg/catalog/__init__.py#L588 This aligns with the rest catalog api.

@Fokko Do you have an opinion?

Hey @wgtmac I think it makes sense to add all of them to the Catalog API. Keep in mind that some implementations (Rest, SQL) support nested namespaces, and some don't (Hive, Glue etc)

wgtmac · 2025-05-19T03:44:37Z

src/iceberg/catalog/in_memory_catalog.h

+
+namespace iceberg {
+
+class ICEBERG_EXPORT InMemoryCatalog : public Catalog {


Add some comments to describe its purpose. Especially to clarify that it is not supposed for production use.

wgtmac · 2025-05-19T03:48:59Z

src/iceberg/catalog/in_memory_catalog.cc

+ * tables, and child namespaces. This structure enables a tree-like representation
+ * of nested namespaces.
+ */
+class ICEBERG_EXPORT InMemoryNamespace {


As discussed in #80 (comment), we need to move some of these functions (which are included in the rest catalog spec) as a part of the public Catalog API.

wgtmac · 2025-05-19T03:49:59Z

src/iceberg/catalog/in_memory_catalog.cc

+   * \param[in] namespace_ident The namespace to check.
+   * \return Status indicating success or failure.
+   */
+  Status NamespaceExists(const Namespace& namespace_ident) const;


Use Result<bool>

wgtmac · 2025-05-19T03:50:55Z

src/iceberg/catalog/in_memory_catalog.cc

+   *                                the children of the root are returned.
+   * \return A vector of child namespace names.
+   */
+  Result<std::vector<std::string>> ListChildrenNamespaces(


If parent can be optional, is it better to rename it to ListNamespaces?

…d table support

wgtmac

Thanks for updating it! I've left some minor comments.

wgtmac · 2025-05-22T06:11:03Z

src/iceberg/catalog.h

+  /// \return Status::OK if updated successfully;
+  ///         ErrorKind::kNoSuchNamespace if the namespace does not exist;
+  ///         ErrorKind::kNotSupported if the operation is not supported
+  virtual Status SetNamespaceProperties(


What about merging SetNamespaceProperties and RemoveNamespaceProperties into a single UpdateNamespaceProperties(updates, removals)? This is a single call in the rest catalog spec.

I am referring to the implementation in java implementations, which has two interfaces. Should we follow the REST catalog spec?

@Fokko WDYT?

wgtmac · 2025-05-22T06:12:13Z

src/iceberg/catalog/in_memory_catalog.h

+/**
+ * @brief An in-memory implementation of the Iceberg Catalog interface.
+ *
+ * This catalog stores all metadata purely in memory, with no persistence to disk
+ * or external systems. It is primarily intended for unit tests, prototyping, or
+ * demonstration purposes.
+ *
+ * @note This class is **not** suitable for production use.
+ *       All data will be lost when the process exits.
+ */


Suggested change

/**

* @brief An in-memory implementation of the Iceberg Catalog interface.

*

* This catalog stores all metadata purely in memory, with no persistence to disk

* or external systems. It is primarily intended for unit tests, prototyping, or

* demonstration purposes.

*

* @note This class is **not** suitable for production use.

* All data will be lost when the process exits.

*/

/// \brief An in-memory implementation of the Iceberg Catalog interface.

///

/// This catalog stores all metadata purely in memory, with no persistence to disk

/// or external systems. It is primarily intended for unit tests, prototyping, or

/// demonstration purposes.

///

/// \note This class is **not** suitable for production use.

/// All data will be lost when the process exits.

…ies"

gty404 · 2025-05-26T11:59:46Z

@Xuanwo @Fokko Could you help review this? Thanks!

Fokko · 2025-05-27T19:50:22Z

Yes, looks good @gty404 thanks!

gty404 force-pushed the memory-catalog branch 3 times, most recently from 2774dc9 to e47caa0 Compare April 17, 2025 09:04

zhjwpku reviewed Apr 17, 2025

View reviewed changes

src/iceberg/catalog/memory_catalog.cc Outdated Show resolved Hide resolved

src/iceberg/catalog/memory_catalog.h Outdated Show resolved Hide resolved

src/iceberg/catalog/memory_catalog.h Outdated Show resolved Hide resolved

zhjwpku mentioned this pull request Apr 18, 2025

refactor: add expression subdirectory #81

Merged

wgtmac reviewed Apr 18, 2025

View reviewed changes

gty404 force-pushed the memory-catalog branch from e47caa0 to 44b0d28 Compare April 19, 2025 07:27

yingcai-cy reviewed Apr 23, 2025

View reviewed changes

src/iceberg/catalog/memory_catalog.h Outdated Show resolved Hide resolved

yingcai-cy reviewed Apr 23, 2025

View reviewed changes

src/iceberg/catalog/memory_catalog.h Outdated Show resolved Hide resolved

gty404 force-pushed the memory-catalog branch from 44b0d28 to 9bb7ee9 Compare April 24, 2025 01:06

zhjwpku approved these changes Apr 25, 2025

View reviewed changes

src/iceberg/catalog/memory_catalog.cc Outdated Show resolved Hide resolved

src/iceberg/catalog/memory_catalog.h Outdated Show resolved Hide resolved

wgtmac reviewed Apr 25, 2025

View reviewed changes

gty404 force-pushed the memory-catalog branch from f6bfb55 to 52e0c5b Compare May 10, 2025 06:02

gty404 force-pushed the memory-catalog branch from 52e0c5b to 59ec124 Compare May 17, 2025 06:09

wgtmac reviewed May 19, 2025

View reviewed changes

gty404 added 8 commits May 22, 2025 09:25

feat: implement initial MemoryCatalog functionality with namespace an…

87d747f

…d table support

fix: some review suggestions

33c02c3

fix: Merge the latest commits from the main branch

0a58823

Change the return result type of the method to use Result/Status

38b85c0

fix clang-format

7f31760

rename memory catalog

17a6d9f

Move the implementation details of memory catalog to cc

a7a680f

fix comments

1831078

gty404 force-pushed the memory-catalog branch from 59ec124 to 1831078 Compare May 22, 2025 05:30

wgtmac approved these changes May 22, 2025

View reviewed changes

Change "set/remove namespace properties" to "update namespace propert…

99675e4

…ies"

wgtmac approved these changes May 26, 2025

View reviewed changes

Fokko approved these changes May 27, 2025

View reviewed changes

Fokko merged commit 5aec271 into apache:main May 27, 2025
6 checks passed

gty404 deleted the memory-catalog branch May 28, 2025 01:55


		namespace iceberg {

		class ICEBERG_EXPORT InMemoryCatalog : public Catalog {

feat: implement initial MemoryCatalog functionality with namespace and table support #80

feat: implement initial MemoryCatalog functionality with namespace and table support #80

Uh oh!

Conversation

gty404 commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fokko commented Apr 20, 2025

Uh oh!

wgtmac commented Apr 21, 2025

Uh oh!

gty404 commented Apr 21, 2025

Uh oh!

Uh oh!

Uh oh!

Fokko commented Apr 23, 2025

Uh oh!

gty404 commented Apr 24, 2025

Uh oh!

zhjwpku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gty404 commented May 26, 2025

Uh oh!

Uh oh!