Repository navigation
Conversation
f93aa25 to
7b35032
Compare
There was a problem hiding this comment.
@lucasfang, I left some comments for this pull request. PTAL.
There was a problem hiding this comment.
@lucasfang , one additional correctness issue remains in the direct system-table read path.
zjw1111
left a comment
There was a problem hiding this comment.
Two correctness issues remain in the table-first write/commit path at 13fba3780476ce4177d2197e0d7f62816bdd3047.
zjw1111
left a comment
There was a problem hiding this comment.
One branch identity issue remains in SwitchToBranch() at 13fba3780476ce4177d2197e0d7f62816bdd3047.
There was a problem hiding this comment.
@lucasfang, thanks for updates. Only one fallback-read correctness issue comment. PTAL.
| if (options_differ) { | ||
| dynamic_options = options; | ||
| dynamic_options.erase(Options::TYPE); | ||
| dynamic_options.erase(Options::BRANCH); |
There was a problem hiding this comment.
Preserve explicit options that match the main schema. When every context option already matches the main schema, options_differ stays false and dynamic_options remains empty. Those explicit options are then lost when constructing the fallback reader.
For example, if the main schema has key-value.sequence_number.enabled=true and scan.fallback-branch=fb, the fallback schema omits the sequence-number setting, and the read context explicitly sets it to true while projecting _SEQUENCE_NUMBER, TableRead::Create() fails when the fallback treats that column as an unknown schema field. Previously, context options were merged into both branches.
Please preserve the supplied dynamic options even when the main schema needs no merge, and add a regression test for this matching-value case.
Purpose
Linked issue: close #459
The single
Tableclass dispatched managed, format, and system tables throughifbranches in one 1400-linetable.cpp; every operator call re-merged an options map and re-parsedCoreOptions(Java'sAbstractFileStoreparses 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/Copyare virtual, the constructor is protected. Implemented byFileStoreTable(withAppendOnlyFileStoreTable/PrimaryKeyFileStoreTablesubclasses,src/paimon/core/table/), byFormatTable(now deriving fromTable, owning its own assembly —format_table_loaderis absorbed), and by theSystemTablefamily.FileStore(src/paimon/core/table/file_store.h/.cpp), the counterpart of Java'sAbstractFileStore: materializesCoreOptionsonce through astatic 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.FileStoreTableholds 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-appliestypeorbranch;SwitchToBranch()derives a table of another branch.TableScan::Create(ctx)etc.) merges them over the schema's throughTable::Copy()— a table is the product of its load — whileNewXxxon a held table refuses a contradicting option (option 'X' is 'A' in the context but 'B' on the table, values redacted throughSensitiveConfigUtils::RedactValue). The four context builders document the semantics.RestCatalog(src/paimon/rest/rest_catalog.cpp):GetTableserves the global system tables of thesysdatabase locally (they previously reached the managed-table load, which statically cast their read-only schema — undefined behavior), binds per-table system tables toGetTableFileSystem()so data-token credentials reach$snapshotsand friends, andFileStoreTable::Createrefuses a non-managed schema instead of casting it.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,Copykeeping that branch when handed abranchoption (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, thesys-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 theTableAPI.PAIMON_ENABLE_REST=ON):paimon-core-test2492/2492;paimon-rest-test152/152;ctest -j 9632/33 — the only failure ispaimon-write-and-read-inte-testwith its 2("parquet", "jindo")jindo-credential environment cases, unchanged from the baseline;paimon-data-evolution-table-test306/306 andpaimon-scan-and-read-inte-test511/511;pre-commit run --files <changed>passed (clang-format applied);git diff --checkclean.API and Format
No storage format or protocol change. Breaking public API changes:
Tableis now abstract (constructor protected,Name/FullName/NewXxx/Copyvirtual,Table::CurrentBranch()replaced byFileStoreTable::CurrentBranch());FormatTablederives fromTable(staticFormatTable::Copy(table, options)becomes the virtualTable::Copy(options),Location()becomesGetRootPath());FileStoreTable::SwitchToBranchis 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.rstfollows theFormatTableAPI rename.Generative AI tooling
Generated-by: Qoder