lucasfang opened a new issue, #459: URL: https://github.com/apache/paimon-cpp/issues/459
## Search before asking - [x] I searched in the [issues](https://github.com/apache/paimon-cpp/issues) and found nothing similar. ## Motivation A single `Table` class dispatched everything: the differences between managed, format, and system tables lived in `if` branches inside one 1400-line `table.cpp`, every operator call (`NewScan/NewRead/NewWrite/NewCommit`) re-merged a fresh options map and re-parsed `CoreOptions` — the state Java Paimon's `AbstractFileStore` parses exactly once — and the file system a table is served by was resolved before the schema's access configuration was even known. The ambiguity produced real defects: - `RestCatalog::GetTable(Identifier("sys", "tables"))` returned the read-only schema of a global system table but still passed it to the managed-table load, which statically cast it to `TableSchema` and called `PrimaryKeys()` on it — undefined behavior. - The operator factory path built the file system from the context options alone; a schema carrying remote backend and credentials (delivered through `SetTableSchema()`) was therefore served by the default local backend, and reading its data failed. - The `$ro` and `$audit_log` views built their underlying scans and reads from the context's file system, ignoring the one their table was opened with; a context that also carried a file-system scheme map was then refused outright (`WithFileSystem()` and `WithFileSystemSchemeToIdentifierMap() cannot be used together`). - `FileStoreTable::Copy` half-applied a `branch` option: it was written into the copy's schema options but never moved the copy's branch, so the consistency check reported the branch actually served as a conflict and let a mismatched one through. - An option conflict between a context and a held table was reported with both raw values in the error message, leaking credentials into logs. - With REST data tokens enabled, the system tables a `GetTable()` call returned were bound to the catalog-wide file system, so `$snapshots` and friends read the data table's files without the table-level credentials and failed. ## Solution - Split the hierarchy: `Table` becomes an abstract base (`NewXxx` pure virtual, a new virtual `Copy(dynamic_options)`), implemented by `FileStoreTable` — with `AppendOnlyFileStoreTable`/`PrimaryKeyFileStoreTable` subclasses — by `FormatTable` (which now derives from `Table` and owns its own assembly, absorbing `format_table_loader`), and by the `SystemTable` family. - Introduce a `FileStore` (the counterpart of Java's `AbstractFileStore`) that materializes `CoreOptions` once at construction, 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 (`NewPathFactory`, `NewSnapshotManager`, `NewManifestList`, ...). `FileStoreTable` holds its schema/snapshot managers and its branch; `SwitchToBranch` derives a table of another branch. - Give context options one clear place to speak: the load. A factory-path load (`TableScan::Create(ctx)` etc.) merges the context options 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). `Copy` never half-applies `type` or `branch`; managers are handed over unless a file-system option among the copy's changes the resolution, in which case fresh ones are built. - Fix the surrounding defects: `RestCatalog::GetTable` serves `sys`-database global system tables locally and binds per-table system tables to `GetTableFileSystem()`; `$ro`/`$audit_log`/`$binlog` prefer the context's file system and fall back to the table's only when the context names neither a file system nor a scheme map; the scan-fallback read context merges the load's dynamic options so the fallback reader recognises what the main one does. ## Anything else? - Options a schema carries that do not parse now fail at table-build time rather than at the first operator; a table opened with neither a schema-carried backend nor a caller file system is served by the default local one instead of holding a null one. - No storage format or protocol change. Public API changes: `Table` is 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)`, `GetLocation()` becomes `GetRootPath()`), and `FileStoreTable::SwitchToBranch` is new. ## Are you willing to submit a PR? - [x] I'm willing to submit a 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
