refactor(table): split the table hierarchy and materialize the store a table serves by lucasfang · Pull Request #447 · apache/paimon-cpp

lucasfang · 2026-10-20T07:00:12Z

Purpose

Linked issue: close #459

The single Table class dispatched managed, format, and system tables through if branches in one 1400-line table.cpp; every operator call re-merged an options map and re-parsed CoreOptions (Java's AbstractFileStore parses once), and the served file system was resolved before the schema's access configuration was known. This splits the hierarchy the way Java splits it and gives context options one clear place to speak, fixing the defects that the ambiguity hid.

Changes:

Table (include/paimon/catalog/table.h) becomes an abstract base: Name/FullName/NewXxx/Copy are virtual, the constructor is protected. Implemented by FileStoreTable (with AppendOnlyFileStoreTable/PrimaryKeyFileStoreTable subclasses, src/paimon/core/table/), by FormatTable (now deriving from Table, owning its own assembly — format_table_loader is absorbed), and by the SystemTable family.
New FileStore (src/paimon/core/table/file_store.h/.cpp), the counterpart of Java's AbstractFileStore: materializes CoreOptions once through a static Create(), resolves the served file system after the options are known (schema-carried access configuration names its own backend; the caller's explicit file system wins over it), carries the file-system scheme map, and hosts the operator factories. FileStoreTable holds its schema/snapshot managers and its branch; Copy() hands the managers over unless a file-system option among the copy's changes the resolution, and never half-applies type or branch; SwitchToBranch() derives a table of another branch.
Context options speak at the load: a factory-path load (TableScan::Create(ctx) etc.) merges them over the schema's through Table::Copy() — a table is the product of its load — while NewXxx on a held table refuses a contradicting option (option 'X' is 'A' in the context but 'B' on the table, values redacted through SensitiveConfigUtils::RedactValue). The four context builders document the semantics.
RestCatalog (src/paimon/rest/rest_catalog.cpp): GetTable serves the global system tables of the sys database locally (they previously reached the managed-table load, which statically cast their read-only schema — undefined behavior), binds per-table system tables to GetTableFileSystem() so data-token credentials reach $snapshots and friends, and FileStoreTable::Create refuses a non-managed schema instead of casting it.
System table views (read_optimized_system_table.cpp, audit_log_system_table.cpp): the underlying scans and reads prefer the context's file system and fall back to the table's own only when the context names neither a file system nor a scheme map (the previous unconditional fallback was refused when a scheme map was present). The scan-fallback read context merges the load's dynamic options, so the fallback reader recognises a sequence-number column the main one does.

Tests

src/paimon/core/table/table_test.cpp (expanded): managers of the branch a table was loaded with, SwitchToBranch, Copy keeping that branch when handed a branch option (both directions, verified to fail before the fix), operators refusing an option that contradicts a held table, a factory-path override of a schema-carried key, cache injection, the sys-database REST open, and the per-table file system.
src/paimon/core/table/source/table_read_test.cpp (TestMergeOptions, TestDependencyReadWithoutCatalogUsesReadOptions): factory-path merging of context options into the served schema, and the dependency catalog context seeing the merged header value.
src/paimon/core/catalog/file_system_catalog_test.cpp, src/paimon/rest/rest_catalog_test.cpp, and the inte tests (scan_and_read, realtime_write, read, scan, write_and_read_inte_with_index) adapted to the Table API.
Ran (with PAIMON_ENABLE_REST=ON): paimon-core-test 2492/2492; paimon-rest-test 152/152; ctest -j 96 32/33 — the only failure is paimon-write-and-read-inte-test with its 2 ("parquet", "jindo") jindo-credential environment cases, unchanged from the baseline; paimon-data-evolution-table-test 306/306 and paimon-scan-and-read-inte-test 511/511; pre-commit run --files <changed> passed (clang-format applied); git diff --check clean.

API and Format

No storage format or protocol change. Breaking public API changes: Table is now abstract (constructor protected, Name/FullName/NewXxx/Copy virtual, Table::CurrentBranch() replaced by FileStoreTable::CurrentBranch()); FormatTable derives from Table (static FormatTable::Copy(table, options) becomes the virtual Table::Copy(options), Location() becomes GetRootPath()); FileStoreTable::SwitchToBranch is new. Observable semantics: schema options that do not parse fail at table-build time (previously at the first operator); a table without a schema-carried backend or a caller file system is served by the default local one instead of a null one; context options are merged at load time only.

Documentation

No new user-facing feature; the option semantics on the four context builders are documented in the same change, and docs/source/user_guide/format_table.rst follows the FormatTable API rename.

Generative AI tooling

Generated-by: Qoder

SteNicholas

@lucasfang, I left some comments for this pull request. PTAL.

SteNicholas · 2026-10-23T15:44:33Z

+        std::shared_ptr<FileStorePathFactory> path_factory,
+        store_->NewPathFactory(arrow_schema, core_options,
+                               core_options.GetFileFormat()->Identifier(), memory_pool));
+    return NewTableRead(path_factory, internal_context, memory_pool, executor);


@lucasfang, select the fallback reader from the fallback schema. CreateTableRead now dispatches through the main table's NewTableRead implementation, including when constructing the fallback reader. An append-only main table can have a primary-key fallback branch—the existing append_table_with_rt_branch fixture does. Such fallback splits now get AppendOnlyTableRead without a MergeFileSplitRead, so splits requiring merging or delete handling fail with create reader failed, not read match with split. The previous implementation selected the reader from internal_context->GetPrimaryKeys(). Please select the reader using the fallback schema's primary keys.

SteNicholas · 2026-10-23T15:44:33Z

+        GlobalSystemTableContext context;
+        context.catalog = this;
+        context.fs = fs_;
+        context.warehouse = warehouse_;
+        context.catalog_options = api_->GetMergedOptions();
+        return GlobalSystemTableLoader::Load(identifier.GetObjectName(), context);


@lucasfang, preserve the catalog lifetime for global system tables. The returned system table stores this as a non-owning catalog pointer. After GetTable returns, destroying the catalog leaves sys.tables, sys.partitions and sys.all_table_options with a dangling pointer; their lazy BuildRows calls dereference it during NextBatch. Neither the table nor its reader retains the catalog (InMemorySystemTableRead retains only the table and memory pool). Please retain the required catalog state for the returned table's lifetime.

SteNicholas · 2026-10-23T15:44:33Z

+    for (const auto& [key, value] : dynamic_options) {
+        if (key != Options::FILE_SYSTEM && key.rfind("fs.", 0) != 0) {
+            continue;


@lucasfang, recognize native S3 options when rebuilding managers. This check ignores supported options such as s3.endpoint, s3.access-key and s3.session.token. When the filesystem is resolved from options, Copy reparses these into a new filesystem but retains the old schema and snapshot managers, leaving metadata requests on the old endpoint or credentials while data requests use the new settings. Please include native S3 options in the invalidation check. An isolated check of this helper confirmed that changing s3.endpoint returns false, whereas changing fs.s3.endpoint returns true.

SteNicholas · 2026-10-23T15:44:34Z

+    for (const auto& [key, value] : context->GetOptions()) {
+        if (key != Options::TYPE) {
+            options[key] = value;
+        }
+    }


@lucasfang, carry copied table options into fallback reads. The fallback merges only context options, whereas the main reader uses the copied table's options. For a primary-key table copied with table-read.sequence-number.enabled=true and a fallback branch, reading _SEQUENCE_NUMBER with an otherwise empty context succeeds for the main reader but fails while constructing the fallback reader because that flag is lost. InternalReadContext then treats _SEQUENCE_NUMBER as a regular field missing from the fallback schema. Please propagate the table's effective dynamic overrides to the fallback as well.

zjw1111 · 2026-10-26T08:31:16Z

+
+    /// The schema manager the operators read through. It carries the schema cache of `root_path`
+    /// on `branch()`.
+    const std::shared_ptr<SchemaManager>& schema_manager() const {


why these function name is lower_case, not UpperCase?

zjw1111 · 2026-10-26T08:40:02Z

+        const std::string& uuid = "",
+        const std::string& branch = BranchManager::DEFAULT_MAIN_BRANCH,
+        const std::shared_ptr<SchemaManager>& schema_manager = nullptr,
+        const std::shared_ptr<SnapshotManager>& snapshot_manager = nullptr,
+        const std::map<std::string, std::string>& fs_scheme_map = {});


Do not use default values(also in some other files, total 10 functions used)

lxy-9602 · 2026-10-26T06:02:02Z

   PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<paimon::ScanContext> scan_context,
                          scan_builder.Finish());
   PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<paimon::TableScan> scan,
                          paimon::TableScan::Create(std::move(scan_context)));


I’m a bit confused—now that expect for a data table, shouldn’t all the other table types use the table->NewScan()-style interfaces?

lxy-9602 · 2026-10-26T08:03:24Z

 class PAIMON_EXPORT TableScan {
 public:
-    /// Create an instance of `TableScan`.
+    /// Create an instance of `TableScan`: the table the context names is loaded first, then the


"the table the context names" -> "the table created by the context"?

lxy-9602 · 2026-10-26T08:14:13Z

-    /// @param table The format table to read, as `Catalog::GetFormatTable()` hands it back.
+    /// @param table The format table to read, as `Catalog::GetTable()` hands it back.
    explicit ReadContextBuilder(const std::shared_ptr<FormatTable>& table);



I’m thinking about removing FormatTable from xxxContextBuilder. Wouldn’t it be better if all special tables also go through table (i.e., create the behavior via table)? Only the data table might be allowed to be created directly via the context.

Otherwise, the current design feels a bit too coupled between context and table.

lxy-9602 · 2026-10-26T08:53:25Z

+    ASSERT_TRUE(std::dynamic_pointer_cast<SystemTable>(system_table) != nullptr);
+    ASSERT_TRUE(std::dynamic_pointer_cast<FormatTable>(system_table) == nullptr);
+    ASSERT_EQ("options", system_table->Name());
+    ASSERT_EQ("options", system_table->FullName());


For system tables, should the full name and name be tbl1$options?

lxy-9602 · 2026-10-26T08:57:17Z

+    static Result<std::unique_ptr<InternalReadContext>> Create(
+        const std::shared_ptr<ReadContext>& read_context,
+        const std::shared_ptr<TableSchema>& table_schema, CoreOptions core_options,
+        const std::string* path = nullptr);


Prefer not to use default parameters in the production code if possible—otherwise we can’t be sure every call site has been updated. Also, why is CoreOptions not const &?
And adding an extra const std::string* path sounds a bit odd.

lxy-9602 · 2026-10-26T09:14:27Z

+        return Status::NotImplemented(
+            "a format table has no snapshots, so there is nothing a streaming write could "
+            "commit against");
+    }


Are these actually “NotImplemented”, or are some of them “Invalid”?

lxy-9602 · 2026-10-26T09:46:43Z

+        // default backend.
+        .WithFileSystem(context->GetSpecificFileSystem() != nullptr
+                            ? context->GetSpecificFileSystem()
+                            : GetFileSystem())


Why doesn’t this consider context->GetFileSystemSchemeToIdentifierMap()?
It seems a bit asymmetric with read.

lxy-9602 · 2026-10-26T09:59:27Z

+            // the base holds, so its own read-only schema is adopted before it leaves the
+            // loader.
+            PAIMON_RETURN_NOT_OK(system_table->AdoptOwnSchema());
+            return system_table;


This design is a bit coupled—similar to an init function.
Do we need SystemTable::GetSchema to override with its own logic?

lxy-9602 · 2026-10-26T10:11:12Z

+/// `Table` interface as every other kind of table. `Name()` - which each kind of system table
+/// answers for itself - is its full name as well: the identifier it is opened by belongs to the
+/// data table underneath and would name that one.
+class SystemTable : public Table, public std::enable_shared_from_this<SystemTable> {


Why is this designed this way here? In Java it just returns "tbl$options".

lxy-9602 · 2026-10-26T10:49:33Z

+    /// The shared ownership a returned table keeps of its catalog, so its lazy `BuildRows`
+    /// cannot read through one already destroyed. Empty where the table is transient - used
+    /// within the call, before the catalog can go away.
+    std::shared_ptr<const Catalog> catalog_retainer;


This design is a bit weird—why can’t we just use std::shared_ptr<Catalog> directly?

zjw1111 · 2026-10-26T09:11:22Z

+class ManifestList;
+class TableSchema;
+
+/// The storage a managed table is served through: the file system, root path, schema, branch,


There are so many sentence "XXX a XXX" in this PR, I think it's difficult to understand

and there are many long, complex sentences in the comments. Could you simplify them and express them in plain, easy-to-understand language?

lxy-9602 · 2026-10-26T11:00:39Z

+        // handed back has to keep it alive or the pointer dangles at the first `BuildRows`. A
+        // catalog not owned by a `std::shared_ptr` cannot be kept, so it is refused by name.
+        std::shared_ptr<const RestCatalog> self = weak_from_this().lock();
+        if (self == nullptr) {


Catalog::Create() returns std::unique_ptr<Catalog>. Converting that result to std::shared_ptr<Catalog> does not initialize enable_shared_from_this<RestCatalog>, because the constructor sees the static type Catalog, which has no such base. Consequently, weak_from_this().lock() remains empty and loading sys.tables fails even when the catalog is owned by a shared_ptr.

The new test converts unique_ptr<RestCatalog> directly to shared_ptr<RestCatalog>, so it does not cover this public factory path. Could we make catalog retention work through Catalog::Create() and add a regression test using that path?

Confirmed this still occurs at 13fba3780476ce4177d2197e0d7f62816bdd3047. A standalone C++17 reproduction of the same inheritance and ownership conversion leaves weak_from_this() empty after promoting unique_ptr<Catalog> to shared_ptr<Catalog>, while promoting unique_ptr<RestCatalog> initializes it. The current test exercises only the latter, so the public Catalog::Create() path still cannot load global system tables. Please cover that public factory path in the retention fix.

SteNicholas

One additional correctness issue remains in the direct system-table read path.

SteNicholas · 2026-10-27T04:50:24Z

+Status SystemTable::CheckNoSearch(const std::shared_ptr<ReadContext>& context) {
+    if (context->HasVectorOrFullTextSearch()) {
+        return Status::NotImplemented(
+            "VectorSearch or FullTextSearch does not support system-table reads yet");
+    }
+    return Status::OK();


[P2] Reject FullTextMatch on direct system-table reads

HasVectorOrFullTextSearch() only checks TopK requests; it does not detect FullTextMatch predicates supplied through ReadContextBuilder::SetPredicate(). Calling NewRead() on an in-memory system table (for example, t$options) therefore passes this guard and returns all rows without evaluating the predicate. The static TableRead::Create() path rejects the same request with NotImplemented.

Please also check PredicateUtils::ContainsFullTextMatch(context->GetPredicate()) here and add a regression test through the direct Table::NewRead() entry point.

lucasfang changed the title ~~refactor table~~ refactor: refactor table abstraction Oct 20, 2026

lucasfang marked this pull request as draft October 20, 2026 07:00

lucasfang force-pushed the dev30 branch from 820d019 to b309aad Compare October 22, 2026 09:16

lucasfang added 9 commits October 23, 2026 14:46

refactor table

7c6fec5

fix

3fbf1bb

tmp

a2ccafd

tmp

5cac87b

tmp

54367bb

tmp

09cb587

fix

472c9c5

fix

c4f9d46

fix

7b35032

lucasfang force-pushed the dev30 branch from f93aa25 to 7b35032 Compare October 23, 2026 07:06

fix

d6cbfe3

lucasfang changed the title ~~refactor: refactor table abstraction~~ refactor(table): split the table hierarchy and materialize the store a table serves Oct 23, 2026

lucasfang added 2 commits October 23, 2026 15:43

fix

81d9a5c

fix

231589e

lucasfang marked this pull request as ready for review October 23, 2026 09:32

update

043fbb6

SteNicholas reviewed Oct 23, 2026

View reviewed changes

zjw1111 reviewed Oct 26, 2026

View reviewed changes

fix

cc12dd9

lxy-9602 reviewed Oct 26, 2026

View reviewed changes

zjw1111 reviewed Oct 26, 2026

View reviewed changes

lxy-9602 reviewed Oct 26, 2026

View reviewed changes

fix

13fba37

SteNicholas reviewed Oct 27, 2026

View reviewed changes

Conversation

lucasfang commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Tests

API and Format

Documentation

Generative AI tooling

Uh oh!

SteNicholas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteNicholas Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteNicholas Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteNicholas Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteNicholas Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjw1111 Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjw1111 Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

SteNicholas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lucasfang commented Oct 20, 2026 •

edited

Loading

SteNicholas left a comment •

edited

Loading

SteNicholas Oct 23, 2026 •

edited

Loading

SteNicholas Oct 23, 2026 •

edited

Loading

SteNicholas Oct 23, 2026 •

edited

Loading

SteNicholas Oct 23, 2026 •

edited

Loading

zjw1111 Oct 26, 2026 •

edited

Loading

zjw1111 Oct 26, 2026 •

edited

Loading