JingsongLi commented on code in PR #274:
URL: https://github.com/apache/paimon-cpp/pull/274#discussion_r4061091516
##########
src/paimon/rest/rest_catalog.cpp:
##########
@@ -438,6 +460,34 @@ std::shared_ptr<FileSystem> RestCatalog::GetFileSystem()
const {
return fs_;
}
+Result<std::shared_ptr<FileSystem>> RestCatalog::GetTableFileSystem(
+ const Identifier& identifier) const {
+ // A file system the caller supplied to `Catalog::Create` authenticates
its own accesses
+ // and is used as-is, so it is handed out even when the server issues data
tokens:
+ // rebuilding a delegate from the options would discard it together with
the
+ // `CredentialProvider` it signs with, and could send a custom scheme to
the default
+ // backend. Without data tokens the catalog-wide file system serves the
data as well.
+ if (!data_token_enabled_ || fs_explicitly_supplied_) {
+ return fs_;
+ }
+ // The credentials are issued for the data table, so a system table shares
those of
+ // the table it belongs to.
+ PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier,
ToLoadIdentifier(identifier));
+ // Building the file system asks the server for nothing, the credentials
are loaded on
+ // the first access, so nothing is keyed by the table here: the file
systems built from
+ // the credentials are what a cache reuses and bounds. Keying an instance
by its table
+ // would keep serving the credentials of a dropped table to a table
recreated at another
+ // location.
+ //
+ // The shared cache is keyed by the credentials alone and holds the file
systems built
+ // from the catalog options, which every table agrees on, so a rotation
rebuilds them.
+ const std::map<std::string, std::string>& catalog_options =
api_->GetMergedOptions();
+ std::shared_ptr<RestCredentialProvider> provider =
+ std::make_shared<RestCredentialProvider>(api_, catalog_options,
load_identifier);
+ return std::make_shared<RestTokenFileSystem>(std::move(provider),
catalog_options,
+ token_fs_cache_,
fs_scheme_to_identifier_map_);
Review Comment:
**[P2] Keep external tables on their catalog-configured filesystem**
Please preserve the external-table distinction before constructing this
wrapper. Java's
[CatalogUtils.loadTable()](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java#L343-L347)
selects `externalFileIO` for external tables, and
[RESTCatalog.getTable()](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-core/src/main/java/org/apache/paimon/rest/RESTCatalog.java#L379-L389)
supplies the catalog-configured filesystem for that path. Here
`GetTableResponse::IsExternal()` is never consulted, so an external native
table opened through `WithCatalog()` with `data-token.enabled=true` requests
`/token` before accessing storage. If the server only issues tokens for
internal tables, the read fails even though the catalog storage credentials can
access the external location. Please retain the external flag when resolving
table IO and return the configured filesystem
for external tables; a test whose external-table token endpoint rejects
requests would cover this difference.
##########
src/paimon/core/operation/read_context.cpp:
##########
@@ -306,6 +318,11 @@ Result<std::unique_ptr<ReadContext>>
ReadContextBuilder::Finish() {
if (impl_->format_table_ != nullptr) {
// The table already answers each of these, and from a source this
cannot see behind, so a
// second answer is refused rather than silently dropped.
+ if (impl_->catalog_ != nullptr) {
+ return Status::Invalid(
+ "a format table carries the file system it was loaded through,
so WithCatalog() "
+ "cannot be used with one");
Review Comment:
**[P2] Attach the token filesystem when loading internal format tables**
Please also wire the table-scoped filesystem into
`RestCatalog::LoadFormatTable()`. That method still calls
`FormatTable::Create(fs_, ...)`, and format-table scans, reads and writes use
that stored catalog-wide filesystem. With `data-token.enabled=true` and storage
access available only through the table token, the documented
`ReadContextBuilder(format_table)` / `ScanContextBuilder(format_table)` paths
therefore fail without ever requesting a token; adding `WithCatalog()` is
rejected here. Java [selects the internal/external FileIO before dispatching to
the format-table
loader](https://github.com/apache/paimon/blob/d77efe0d2e3d15e33df0eb8211f833035e0959b8/paimon-core/src/main/java/org/apache/paimon/catalog/CatalogUtils.java#L347-L364),
so internal format tables receive the refreshing token filesystem as well.
Please select and store the appropriate filesystem during format-table loading,
respecting the external-table distinction, and cover an internal format table
whose storage re
quires its token.
--
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]
