SteNicholas commented on code in PR #447:
URL: https://github.com/apache/paimon-cpp/pull/447#discussion_r4361383239
##########
src/paimon/rest/rest_catalog.cpp:
##########
@@ -388,78 +390,110 @@ Result<std::shared_ptr<Schema>>
RestCatalog::LoadTableSchema(const Identifier& i
context.catalog_options = api_->GetMergedOptions();
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<SystemTable> system_table,
GlobalSystemTableLoader::Load(identifier.GetObjectName(), context));
- PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<arrow::Schema> arrow_schema,
- system_table->ArrowSchema());
- return std::make_shared<SystemTableSchema>(std::move(arrow_schema));
+ return system_table->LatestSchema();
}
PAIMON_ASSIGN_OR_RAISE(bool is_system_table, identifier.IsSystemTable());
PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> branch,
identifier.GetBranchName());
branch = NormalizeBranch(std::move(branch));
PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier,
ToLoadIdentifier(identifier));
if (is_system_table) {
- PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> system_table_name,
- identifier.GetSystemTableName());
- if (!system_table_name ||
!SystemTableLoader::IsSupported(system_table_name.value())) {
- return Status::NotExist(fmt::format("{} not exist",
identifier.ToString()));
- }
- std::string data_table_path;
- PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<TableSchema> latest_schema,
- LoadDataTableSchema(load_identifier, branch,
&data_table_path,
- /*table_id=*/nullptr));
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<SystemTable> system_table,
+ LoadSystemTable(identifier, load_identifier,
branch));
if (table_path != nullptr) {
- *table_path = data_table_path;
- }
- std::map<std::string, std::string> dynamic_options;
- if (branch) {
- dynamic_options[Options::BRANCH] = branch.value();
+ *table_path = system_table->GetRootPath();
}
- // The file system is only used to build the system table, whose
schema does not
- // read any file, so the catalog wide one is enough here. The
credentials of the
- // table are applied when its rows are read, through the file system
of the read or
- // scan context.
- PAIMON_ASSIGN_OR_RAISE(
- std::shared_ptr<SystemTable> system_table,
- SystemTableLoader::Load(system_table_name.value(), fs_,
data_table_path, latest_schema,
- dynamic_options));
- PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<arrow::Schema> arrow_schema,
- system_table->ArrowSchema());
- return std::make_shared<SystemTableSchema>(std::move(arrow_schema));
+ return system_table->LatestSchema();
}
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<TableSchema> schema,
LoadDataTableSchema(load_identifier, branch,
table_path, table_id));
return checked_pointer_cast<Schema>(schema);
}
-Result<std::shared_ptr<FormatTable>> RestCatalog::LoadFormatTable(
- const Identifier& identifier) const {
- // A system table is already refused by `Catalog::GetFormatTable()`, which
every caller goes
- // through, so this only has to load.
- PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> branch,
identifier.GetBranchName());
- branch = NormalizeBranch(std::move(branch));
- PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier,
ToLoadIdentifier(identifier));
- std::string location;
- PAIMON_ASSIGN_OR_RAISE(
- std::shared_ptr<TableSchema> schema,
- LoadDataTableSchema(load_identifier, branch, &location,
/*table_id=*/nullptr));
- // A rest catalog holds the schema itself, so everything below the
location is data.
- return FormatTable::Create(fs_, location, identifier,
checked_pointer_cast<DataSchema>(schema),
- /*location_carries_paimon_metadata=*/false,
- /*dynamic_options=*/{});
+Result<std::shared_ptr<SystemTable>> RestCatalog::LoadSystemTable(
+ const Identifier& identifier, const Identifier& load_identifier,
+ const std::optional<std::string>& branch) const {
+ PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> system_table_name,
+ identifier.GetSystemTableName());
+ if (!system_table_name ||
!SystemTableLoader::IsSupported(system_table_name.value())) {
+ return Status::NotExist(fmt::format("{} not exist",
identifier.ToString()));
+ }
+ std::string data_table_path;
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<TableSchema> latest_schema,
+ LoadDataTableSchema(load_identifier, branch,
&data_table_path,
+ /*table_id=*/nullptr));
+ std::map<std::string, std::string> dynamic_options;
+ if (branch) {
+ dynamic_options[Options::BRANCH] = branch.value();
+ }
+ // The system table reads the data table's own files - snapshots,
manifests, data - so it is
+ // served by the file system that table's credentials open, which with
data tokens is the
+ // only one that can: `GetTableFileSystem` folds the system identifier to
the data table it
+ // belongs to and hands the catalog-wide file system out when no tokens
are issued.
+ PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<FileSystem> table_file_system,
+ GetTableFileSystem(identifier));
+ return SystemTableLoader::Load(system_table_name.value(),
table_file_system, data_table_path,
+ latest_schema, dynamic_options);
}
Result<std::shared_ptr<Table>> RestCatalog::GetTable(const Identifier&
identifier) const {
+ // The global system tables of the "sys" database expose the catalog
itself rather than any
+ // table the server stands behind, so they are served locally - the way
`LoadTableSchema`
+ // serves them their schema - and never reach the managed-table load
below, whose schema
+ // cast a read-only system table schema would not survive.
+ if (CatalogUtils::IsSystemDatabase(identifier.GetDatabaseName())) {
+ PAIMON_ASSIGN_OR_RAISE(bool supported,
+
GlobalSystemTableLoader::IsSupported(identifier.GetObjectName(),
+
api_->GetMergedOptions()));
+ if (!supported) {
+ return Status::NotExist(fmt::format("{} not exist",
identifier.ToString()));
+ }
+ GlobalSystemTableContext context;
+ context.catalog = this;
+ context.fs = fs_;
+ context.warehouse = warehouse_;
+ context.catalog_options = api_->GetMergedOptions();
+ return GlobalSystemTableLoader::Load(identifier.GetObjectName(),
context);
Review Comment:
@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.
--
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]
