SteNicholas commented on code in PR #274:
URL: https://github.com/apache/paimon-cpp/pull/274#discussion_r4036417347
##########
src/paimon/core/table/source/table_scan.cpp:
##########
@@ -97,6 +99,15 @@ class TableScanImpl {
auto manifest_file_format = core_options.GetManifestFormat();
std::string branch =
BranchManager::NormalizeBranch(core_options.GetBranch());
auto snapshot_manager = std::make_shared<SnapshotManager>(fs,
context->GetPath(), branch);
+ if (VersionManagedCatalog* versioned =
AsVersionManaged(context->GetCatalog())) {
+ std::shared_ptr<Catalog> catalog = context->GetCatalog();
+ Identifier identifier = context->GetIdentifier().value();
+ // Keep the catalog alive while the scan uses its snapshot loader.
+ snapshot_manager->SetSnapshotLoader(
+ [catalog, versioned, identifier]() ->
Result<std::optional<Snapshot>> {
+ return versioned->LoadSnapshot(identifier);
+ });
Review Comment:
**[P1] Keep the catalog snapshot body when planning the initial scan**
Installing this loader does not make the default scan work when the latest
snapshot exists only in the catalog. `FullStartingScanner::Scan()` calls
`LatestSnapshotId()` (which now uses this loader), then calls
`SnapshotManager::LoadSnapshot(id)`, which unconditionally reads
`<table>/snapshot/snapshot-<id>` from the filesystem. Consequently both a
default batch scan and the initial full streaming scan fail with `NotExist`
even though the catalog returned a valid snapshot. The new
`TestCatalogScanReadsSnapshotFromCatalog` leaves the fixture's snapshot file in
place and therefore masks this failure. Please carry the returned `Snapshot`
into the initial plan, and cover a catalog-held snapshot with no corresponding
snapshot file.
##########
src/paimon/rest/rest_token_file_system.cpp:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "paimon/rest/rest_token_file_system.h"
+
+#include <utility>
+
+#include "paimon/core/core_options.h"
+
+namespace paimon {
+
+std::shared_ptr<RestTokenFileSystemCache>
RestTokenFileSystem::CreateFileSystemCache() {
+ RestTokenFileSystemCache::Options cache_options;
+ cache_options.max_weight = kMaxCachedFileSystems;
+ cache_options.expire_after_access_ms =
kFileSystemCacheExpireAfterAccessMillis;
+ return
std::make_shared<RestTokenFileSystemCache>(std::move(cache_options));
+}
+
+RestTokenFileSystem::RestTokenFileSystem(const std::shared_ptr<RestApi>& api,
+ const std::map<std::string,
std::string>& catalog_options,
+ const Identifier& identifier,
+
std::shared_ptr<RestTokenFileSystemCache> fs_cache,
+ Clock clock)
+ : catalog_options_(catalog_options),
+ identifier_(identifier),
+ fs_cache_(fs_cache != nullptr ? std::move(fs_cache) :
CreateFileSystemCache()),
+ provider_(std::make_shared<RestCredentialProvider>(api, catalog_options,
identifier,
+ std::move(clock))) {}
+
+Result<std::shared_ptr<FileSystem>> RestTokenFileSystem::BuildFileSystem(
+ const RestToken& token) const {
+ std::map<std::string, std::string> fs_options = catalog_options_;
+ for (const auto& [key, value] : token.token) {
+ fs_options[key] = value;
+ }
Review Comment:
**[P2] Prevent bucket credentials from overriding the issued token**
This merge only replaces identically named keys. For an OSS catalog
configured with `fs.oss.bucket.b.accessKeyId`/`accessKeySecret`, a token
containing `fs.oss.accessKeyId`/`accessKeySecret`/`securityToken` leaves the
bucket keys intact. `OssFileSystemFactory::FindOption()` prefers those bucket
keys, so access to `oss://b/...` is signed with the old catalog key pair and
the new STS token, which is an invalid credential combination; if a
bucket-scoped security token is configured too, the issued credentials are
ignored entirely. Please ensure the issued credentials take precedence after
bucket-option resolution, and add coverage with bucket-scoped catalog
credentials.
##########
src/paimon/rest/rest_catalog.cpp:
##########
@@ -438,6 +453,23 @@ std::shared_ptr<FileSystem> RestCatalog::GetFileSystem()
const {
return fs_;
}
+Result<std::shared_ptr<FileSystem>> RestCatalog::GetTableFileSystem(
+ const Identifier& identifier) const {
+ if (!data_token_enabled_) {
+ return fs_;
+ }
Review Comment:
**[P2] Preserve an explicitly supplied catalog filesystem**
When `Catalog::Create(..., custom_fs)` is combined with
`data-token.enabled=true` (including when enabled by `/v1/config`), this method
ignores `fs_` and always returns a new `RestTokenFileSystem`. The
read/write/commit paths using `WithCatalog` therefore discard the caller's
filesystem and its `CredentialProvider`: `BuildFileSystem()` reconstructs a
built-in delegate from options with `specified_file_system=nullptr`. This
breaks the newly documented `Catalog::Create` injection path, which promises to
use the supplied filesystem as-is, and can send custom-scheme accesses to the
default local backend. Please retain whether a filesystem was explicitly
supplied and preserve that override here, with a test that enables data tokens
while injecting a custom filesystem.
##########
docs/source/user_guide/catalog.rst:
##########
@@ -233,6 +252,51 @@ so its restored file references are visible to the
expiration operation.
A branch the catalog holds without that directory, or one created while
expiration runs, is not found: do not expire a table with such a branch, and
serialize branch creation and expiration through the upstream coordinator.
+=======
Review Comment:
**[P3] Remove the leftover merge-conflict separators**
There are standalone `=======` conflict markers here and at line 363,
followed by a duplicated trailing sentence at line 364. `git diff --check`
fails on both markers, so the repository's required diff check does not pass.
Please finish resolving these documentation hunks and remove the duplicated
text.
--
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]
