zjw1111 commented on code in PR #205: URL: https://github.com/apache/paimon-cpp/pull/205#discussion_r3811879591
########## src/paimon/fs/oss/oss_file_system.cpp: ########## @@ -0,0 +1,196 @@ +/* + * 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/fs/oss/oss_file_system.h" + +#include <curl/curl.h> + +#include <algorithm> +#include <cstdint> +#include <ctime> +#include <memory> +#include <utility> + +#include "alibabacloud/oss2/OSSClient.h" +#include "alibabacloud/oss2/Operation.h" +#include "alibabacloud/oss2/Types.h" +#include "alibabacloud/oss2/io/ByteWriter.h" +#include "alibabacloud/oss2/models/BucketBasic.h" +#include "alibabacloud/oss2/models/ObjectBasic.h" +#include "fmt/format.h" +#include "paimon/executor.h" + +namespace paimon::oss { +namespace { + +namespace oss2 = alibabacloud::oss2; + +bool IsNotFoundError(const oss2::OperationError& error) { + return error.getStatusCode() == 404 || error.getCode() == "NoSuchKey" || + error.getCode() == "NoSuchBucket" || error.getCode() == "NotFound"; +} + +Status ToPaimonStatus(const oss2::OperationError& error, const std::string& operation, + const ObjectStorePath& path) { + std::string message = fmt::format("OSS {} 'oss://{}/{}' failed: code={}, status={}, message={}", + operation, path.bucket, path.key, error.getCode(), + error.getStatusCode(), error.getMessage()); + if (!error.getRequestId().empty()) { + message += fmt::format(", request_id={}", error.getRequestId()); + } + if (IsNotFoundError(error)) { + return Status::NotExist(message); + } + if (error.getCode() == "RequestCanceled") { + return Status::Cancelled(message); + } + return Status::IOError(message); +} + +int64_t ParseTimeMillis(const std::string& value) { + time_t seconds = curl_getdate(value.c_str(), nullptr); Review Comment: `curl_getdate` cannot parse the timestamp format that `ListObjectsV2` returns, so this helper returns `0` for every listed object. `curl_getdate` only accepts formats carrying an English month name (RFC 822/1123, RFC 850, `asctime` and their loose variants), plus one compact `YYYYMMDD` form. In curl's `lib/parsedate.c`, `w->mon` is only ever set by `checkmonth()` on an alphabetic token or by the `num_digits == 8` branch in `datenum()`, and `datecheck()` fails immediately when `mon == -1`. `ListObjectsV2` returns ISO 8601 in the XML body and the SDK stores it verbatim in `ObjectSummary::lastModified`. For `2024-01-01T00:00:00.000Z` the parse actually fails even earlier: the tokens are `2024` (taken as year), `01` (taken as mday), and the third `01` has no field left to fill, so `datenum()` returns `PARSEDATE_FAIL`. `HeadObject` is unaffected because `HeadObjectResult::getLastModified()` reads the HTTP `Last-Modified` header, which is an HTTP-date and does contain a month name. That asymmetry makes this easy to miss. Downstream, every `FileStatus` produced by `ListDir` / `ListStatus` gets `modification_time == 0`, and `OrphanFilesCleanerImpl` then rejects it (`GetModificationTime() <= MIN_VALID_FILE_MODIFICATION_MS` returns `Status::Invalid("... is not in millisecond")`), so orphan file cleanup fails on OSS. Returning `0` also collides with `FileStatus::kUnknownModificationTime`, which is `-1`, so callers cannot distinguish "unknown" from the epoch. `s3_file_system.cpp` has the same defect in `ParseModificationTime` (S3's `ListObjectsV2` `LastModified` is ISO 8601 as well), and the two also differ in the failure check: S3 uses `seconds == static_cast<time_t>(-1)` while this uses `seconds < 0`, which is always false where `time_t` is unsigned. Would it be possible to extract one shared helper used by both file systems — handling ISO 8601 explicitly (`strptime("%Y-%m-%dT%H:%M:%S")` + `timegm`, plus the optional `.SSS` and the `Z` / `+hh:mm` suffix), falling back to `curl_getdate` for HTTP-date, and returning `kUnknownModificationTime` on failure? It would also help to assert a parsed value in the tests rather than only the failure path. `TestListObjects` already feeds `<LastModified>2024-01-01T00:00:00.000Z</LastModified>` but only checks `statuses.size()`, and `TestHeadObjectParsesMetadata` uses `not-a-timestamp` and asserts `0`, so this slipped through. ########## src/paimon/fs/oss/oss_file_system_factory.cpp: ########## @@ -0,0 +1,201 @@ +/* + * 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/fs/oss/oss_file_system_factory.h" + +#include <memory> +#include <optional> +#include <string> +#include <string_view> +#include <utility> + +#include "alibabacloud/oss2/ClientConfiguration.h" +#include "alibabacloud/oss2/OSSClient.h" +#include "alibabacloud/oss2/credentials/CredentialsProvider.h" +#include "fmt/format.h" +#include "paimon/common/utils/path_util.h" +#include "paimon/common/utils/string_utils.h" +#include "paimon/factories/factory.h" +#include "paimon/fs/oss/oss_file_system.h" + +namespace paimon::oss { +namespace { + +namespace oss2 = alibabacloud::oss2; + +constexpr std::string_view kOssOptionPrefix = "fs.oss."; + +std::string GetBucketOptionKey(const std::string& bucket, std::string_view option) { + return fmt::format("fs.oss.bucket.{}.{}", bucket, option.substr(kOssOptionPrefix.size())); +} + +const std::string* FindOption(const std::map<std::string, std::string>& options, + const std::string& bucket, std::string_view option, + std::string* option_key) { + std::string bucket_option_key = GetBucketOptionKey(bucket, option); + auto bucket_option = options.find(bucket_option_key); + if (bucket_option != options.end()) { + *option_key = std::move(bucket_option_key); + return &bucket_option->second; + } + option_key->assign(option); + auto global_option = options.find(*option_key); + return global_option == options.end() ? nullptr : &global_option->second; +} + +std::string GetOption(const std::map<std::string, std::string>& options, const std::string& bucket, + std::string_view option) { + std::string option_key; + const std::string* value = FindOption(options, bucket, option, &option_key); + return value == nullptr ? "" : *value; +} + +Result<std::string> GetRequiredOption(const std::map<std::string, std::string>& options, + const std::string& bucket, std::string_view option) { + std::string option_key; + const std::string* value = FindOption(options, bucket, option, &option_key); + if (value == nullptr || value->empty()) { + return Status::Invalid(fmt::format("OSS option '{}' must not be empty", option_key)); + } + return *value; +} + +Result<std::unique_ptr<Executor>> CreateExecutor(const std::map<std::string, std::string>& options, + const std::string& bucket) { + std::string option_key; + const std::string* value = + FindOption(options, bucket, kOssExecutorThreadCountOption, &option_key); + if (value == nullptr) { + return CreateDefaultExecutor(); + } + std::optional<uint32_t> thread_count = StringUtils::StringToValue<uint32_t>(*value); + if (!thread_count.has_value() || *thread_count == 0) { + return Status::Invalid(fmt::format( + "OSS executor thread count for option '{}' must be greater than 0", option_key)); + } + return CreateDefaultExecutor(*thread_count); +} + +std::string NormalizeEndpoint(std::string endpoint) { + if (!endpoint.empty() && endpoint.find("://") == std::string::npos) { + endpoint = "https://"; + endpoint; + } + return endpoint; +} + +std::string InferRegion(std::string endpoint) { + constexpr std::string_view kPrefix = "oss-"; + constexpr std::string_view kSuffix = ".aliyuncs.com"; + size_t scheme = endpoint.find("://"); + if (scheme != std::string::npos) { + endpoint.erase(0, scheme + 3); + } + size_t slash = endpoint.find('/'); + if (slash != std::string::npos) { + endpoint.erase(slash); + } + if (endpoint.rfind(kPrefix, 0) != 0) { + return ""; + } + size_t suffix = endpoint.find(kSuffix); + if (suffix == std::string::npos || suffix + kSuffix.size() != endpoint.size()) { + return ""; + } + std::string region = endpoint.substr(kPrefix.size(), suffix - kPrefix.size()); + constexpr std::string_view kInternal = "-internal"; + if (region.size() > kInternal.size() && + region.compare(region.size() - kInternal.size(), kInternal.size(), kInternal) == 0) { + region.erase(region.size() - kInternal.size()); + } + if (region == "accelerate" || Review Comment: Thanks for tightening this after the previous round — failing fast instead of silently guessing is the right direction. I think the deny-list still lets one real endpoint through. `oss-accelerate-overseas.aliyuncs.com` is a real endpoint (the SDK produces exactly this string for `EndpointType::Overseas` in `regionToEndpoint()`). It is neither equal to `"accelerate"` nor suffixed with `-dualstack`, so `InferRegion` returns `"accelerate-overseas"`, which is not a region. That value is assigned to `config.region`, and since the SDK defaults `signatureVersion` to `v4`, the region feeds the V4 signing scope and the derived signing key — so every request fails with `SignatureDoesNotMatch`, with an error mentioning nothing about endpoint or region configuration. Returning `""` here would instead surface the clear "OSS region must be configured ..." error at creation time. The `-dualstack` branch also looks unreachable: real dual-stack endpoints are `<region>.oss.aliyuncs.com` (again per `regionToEndpoint()`), which does not start with `oss-` and is already rejected by the prefix check above. `TestEndpointRegionValidation` uses `oss-cn-hangzhou-dualstack.aliyuncs.com`, a form OSS does not serve, so that case passes for the wrong reason. Would it be possible to invert this into an allow-list instead — accepting the inferred value only when it matches a region shape such as `^[a-z]{2,3}-[a-z]+(-[0-9]+)?$`? Then `accelerate`, `accelerate-overseas` and any future non-region endpoint all fall into the explicit-error path automatically, and the `-dualstack` branch could be dropped. It would be good to also change that test case to a real `cn-hangzhou.oss.aliyuncs.com` and add one for `oss-accelerate-overseas.aliyuncs.com`. One more small gap in the same function: an endpoint with an explicit port such as `oss-cn-hangzhou.aliyuncs.com:443` fails the `.aliyuncs.com`-at-the-end check and lands in the same "region must be configured" error, even though the endpoint itself is standard. Stripping a trailing `:<port>` after the path is removed would cover that. ########## src/paimon/fs/oss/oss_file_system_factory.cpp: ########## @@ -0,0 +1,201 @@ +/* + * 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/fs/oss/oss_file_system_factory.h" + +#include <memory> +#include <optional> +#include <string> +#include <string_view> +#include <utility> + +#include "alibabacloud/oss2/ClientConfiguration.h" +#include "alibabacloud/oss2/OSSClient.h" +#include "alibabacloud/oss2/credentials/CredentialsProvider.h" +#include "fmt/format.h" +#include "paimon/common/utils/path_util.h" +#include "paimon/common/utils/string_utils.h" +#include "paimon/factories/factory.h" +#include "paimon/fs/oss/oss_file_system.h" + +namespace paimon::oss { +namespace { + +namespace oss2 = alibabacloud::oss2; + +constexpr std::string_view kOssOptionPrefix = "fs.oss."; + +std::string GetBucketOptionKey(const std::string& bucket, std::string_view option) { + return fmt::format("fs.oss.bucket.{}.{}", bucket, option.substr(kOssOptionPrefix.size())); +} + +const std::string* FindOption(const std::map<std::string, std::string>& options, + const std::string& bucket, std::string_view option, + std::string* option_key) { + std::string bucket_option_key = GetBucketOptionKey(bucket, option); + auto bucket_option = options.find(bucket_option_key); + if (bucket_option != options.end()) { + *option_key = std::move(bucket_option_key); + return &bucket_option->second; + } + option_key->assign(option); + auto global_option = options.find(*option_key); + return global_option == options.end() ? nullptr : &global_option->second; +} + +std::string GetOption(const std::map<std::string, std::string>& options, const std::string& bucket, + std::string_view option) { + std::string option_key; + const std::string* value = FindOption(options, bucket, option, &option_key); + return value == nullptr ? "" : *value; +} + +Result<std::string> GetRequiredOption(const std::map<std::string, std::string>& options, + const std::string& bucket, std::string_view option) { + std::string option_key; + const std::string* value = FindOption(options, bucket, option, &option_key); + if (value == nullptr || value->empty()) { + return Status::Invalid(fmt::format("OSS option '{}' must not be empty", option_key)); + } + return *value; +} + +Result<std::unique_ptr<Executor>> CreateExecutor(const std::map<std::string, std::string>& options, + const std::string& bucket) { + std::string option_key; + const std::string* value = + FindOption(options, bucket, kOssExecutorThreadCountOption, &option_key); + if (value == nullptr) { + return CreateDefaultExecutor(); + } + std::optional<uint32_t> thread_count = StringUtils::StringToValue<uint32_t>(*value); + if (!thread_count.has_value() || *thread_count == 0) { + return Status::Invalid(fmt::format( + "OSS executor thread count for option '{}' must be greater than 0", option_key)); + } + return CreateDefaultExecutor(*thread_count); +} + +std::string NormalizeEndpoint(std::string endpoint) { + if (!endpoint.empty() && endpoint.find("://") == std::string::npos) { + endpoint = "https://"; + endpoint; + } + return endpoint; +} + +std::string InferRegion(std::string endpoint) { + constexpr std::string_view kPrefix = "oss-"; + constexpr std::string_view kSuffix = ".aliyuncs.com"; + size_t scheme = endpoint.find("://"); + if (scheme != std::string::npos) { + endpoint.erase(0, scheme + 3); + } + size_t slash = endpoint.find('/'); + if (slash != std::string::npos) { + endpoint.erase(slash); + } + if (endpoint.rfind(kPrefix, 0) != 0) { Review Comment: `StringUtils::StartsWith` / `EndsWith` already exist in `common/utils/string_utils.h`, and this file already includes it and uses `StringUtils::StringToValue` elsewhere. The four hand-written prefix/suffix checks in this function — this one, the `.aliyuncs.com` check below, the `-internal` strip, and the `-dualstack` check — would read better through them. One caveat if you do this: the helpers take `const std::string&`, while `kPrefix` / `kSuffix` / `kInternal` are `constexpr std::string_view`, which does not convert implicitly — so those constants would need to become `constexpr const char*` (or `StringUtils` would need `string_view` overloads). Also note the `.aliyuncs.com` check currently uses `find()` (first occurrence) plus an end-position check, so switching to `EndsWith` slightly changes behaviour for inputs like `oss-x.aliyuncs.com.aliyuncs.com`; that difference disappears if the allow-list suggestion above is adopted. ########## src/paimon/fs/oss/oss_file_system_test.cpp: ########## @@ -0,0 +1,320 @@ +/* + * 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/fs/oss/oss_file_system.h" + +#include <chrono> +#include <cstdint> +#include <future> +#include <map> +#include <memory> +#include <sstream> +#include <string> +#include <system_error> +#include <utility> +#include <vector> + +#include "alibabacloud/oss2/ClientConfiguration.h" +#include "alibabacloud/oss2/OSSClient.h" +#include "alibabacloud/oss2/credentials/CredentialsProvider.h" +#include "alibabacloud/oss2/io/ByteWriter.h" +#include "alibabacloud/oss2/transport/HttpTransport.h" +#include "gtest/gtest.h" +#include "paimon/fs/oss/oss_file_system_factory.h" +#include "paimon/testing/utils/testharness.h" + +namespace paimon::oss { +namespace { + +namespace oss2 = alibabacloud::oss2; + +class MockHttpTransport : public oss2::HttpTransport { + public: + oss2::ResponseResult send(std::unique_ptr<oss2::RequestMessage>& request, + const oss2::RequestOptions& options) override { + requests.emplace_back(std::make_unique<oss2::RequestMessage>(*request)); + if (responses.empty()) { + return oss2::TransportError{std::make_error_code(std::errc::no_message_available), "", + ""}; + } + std::unique_ptr<oss2::ResponseMessage> response = std::move(responses.front()); + responses.erase(responses.begin()); + if (response->statusCode / 100 == 2 && options.sinkFactory.has_value() && + response->body != nullptr) { + int64_t content_length = -1; + auto content_length_header = response->headers.find("Content-Length"); + if (content_length_header != response->headers.end()) { + content_length = std::stoll(content_length_header->second); + } + std::shared_ptr<oss2::ByteWriter> sink = + options.sinkFactory.value()(content_length, response->headers); + std::ostringstream body; + body << response->body->rdbuf(); + const std::string data = body.str(); + sink->write(reinterpret_cast<const uint8_t*>(data.data()), data.size()); + response->body.reset(); + } + return response; + } + + std::string getName() const override { + return "MockHttpTransport"; + } + + void AddResponse(int status_code, oss2::HeaderCollection headers, std::string body = "") { + std::shared_ptr<std::iostream> response_body; + if (!body.empty()) { + response_body = std::make_shared<std::stringstream>(std::move(body)); + } + responses.emplace_back(std::make_unique<oss2::ResponseMessage>(oss2::ResponseMessage{ + status_code, "", std::move(headers), std::move(response_body), nullptr})); + } + + std::vector<std::unique_ptr<oss2::ResponseMessage>> responses; Review Comment: Small naming nit: `MockHttpTransport` is a class, so Google style wants a trailing underscore on these two data members (`responses_` / `requests_`). `MockHttpClient::status_code_` in `s3_file_system_test.cpp` follows that already. `send` / `getName` above are SDK overrides, so keeping those names is of course correct. ########## src/paimon/fs/oss/oss_file_system.cpp: ########## @@ -0,0 +1,196 @@ +/* + * 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/fs/oss/oss_file_system.h" + +#include <curl/curl.h> + +#include <algorithm> +#include <cstdint> +#include <ctime> +#include <memory> +#include <utility> + +#include "alibabacloud/oss2/OSSClient.h" +#include "alibabacloud/oss2/Operation.h" +#include "alibabacloud/oss2/Types.h" +#include "alibabacloud/oss2/io/ByteWriter.h" +#include "alibabacloud/oss2/models/BucketBasic.h" +#include "alibabacloud/oss2/models/ObjectBasic.h" +#include "fmt/format.h" +#include "paimon/executor.h" + +namespace paimon::oss { +namespace { + +namespace oss2 = alibabacloud::oss2; + +bool IsNotFoundError(const oss2::OperationError& error) { + return error.getStatusCode() == 404 || error.getCode() == "NoSuchKey" || + error.getCode() == "NoSuchBucket" || error.getCode() == "NotFound"; +} + +Status ToPaimonStatus(const oss2::OperationError& error, const std::string& operation, + const ObjectStorePath& path) { + std::string message = fmt::format("OSS {} 'oss://{}/{}' failed: code={}, status={}, message={}", + operation, path.bucket, path.key, error.getCode(), + error.getStatusCode(), error.getMessage()); + if (!error.getRequestId().empty()) { + message += fmt::format(", request_id={}", error.getRequestId()); + } + if (IsNotFoundError(error)) { + return Status::NotExist(message); + } + if (error.getCode() == "RequestCanceled") { + return Status::Cancelled(message); + } + return Status::IOError(message); +} + +int64_t ParseTimeMillis(const std::string& value) { + time_t seconds = curl_getdate(value.c_str(), nullptr); + return seconds < 0 ? 0 : static_cast<int64_t>(seconds) * 1000; +} + +class OssObjectStoreClient : public ObjectStoreClient, + public std::enable_shared_from_this<OssObjectStoreClient> { + public: + OssObjectStoreClient(std::string bucket, std::shared_ptr<oss2::OSSClient> client, + std::unique_ptr<Executor> executor) + : bucket_(std::move(bucket)), client_(std::move(client)), executor_(std::move(executor)) {} + + Result<ObjectMetadata> HeadObject(const ObjectStorePath& path) const override { + PAIMON_RETURN_NOT_OK(ValidateBucket(path)); + oss2::models::HeadObjectRequest request; + request.setBucket(path.bucket).setKey(path.key); + oss2::HeadObjectOutcome outcome = client_->headObject(request); + if (!outcome.has_value()) { + return ToPaimonStatus(outcome.error(), "HeadObject", path); + } + const oss2::models::HeadObjectResult& result = outcome.value(); + if (result.getContentLength() < 0) { + return Status::IOError("OSS HeadObject response is missing Content-Length"); + } + return ObjectMetadata{path.key, result.getContentLength(), + ParseTimeMillis(result.getLastModified())}; + } + + Result<ListObjectsResult> ListObjects(const ObjectStorePath& path, + const std::string& continuation_token, + int32_t max_keys) const override { + PAIMON_RETURN_NOT_OK(ValidateBucket(path)); + oss2::models::ListObjectsV2Request request; + request.setBucket(path.bucket).setPrefix(path.key).setDelimiter("/"); + if (!continuation_token.empty()) { + request.setContinuationToken(continuation_token); + } + if (max_keys > 0) { + // OSS limits ListObjectsV2 requests to 1000 keys. + request.setMaxKeys(std::min(max_keys, 1000)); Review Comment: Minor: could `1000` become a named constant, e.g. `constexpr int32_t kMaxKeysPerRequest = 1000;`? The comment above explains where the limit comes from, but a named constant keeps it greppable if it is needed elsewhere. The same nit applies in `oss_file_system_factory.cpp`, where `"-dualstack"` is spelled inline three times within one expression while the neighbouring `-internal` is already a named `constexpr std::string_view kInternal`. That one goes away if the `InferRegion` allow-list suggestion is taken. ########## CMakeLists.txt: ########## @@ -75,8 +76,10 @@ endif() if(PAIMON_ENABLE_REST) add_definitions(-DPAIMON_ENABLE_REST) endif() -# libcurl backs the HTTP client shared by the S3 file system and the rest catalog. -if(PAIMON_ENABLE_S3 OR PAIMON_ENABLE_REST) +# libcurl backs the HTTP client shared by the object store file systems and the rest catalog. Review Comment: Now that `PAIMON_ENABLE_OSS` is part of this condition, the comment is no longer accurate. The shared HTTP client (`common/utils/http_client.cpp`) is still gated on `PAIMON_ENABLE_S3 OR PAIMON_ENABLE_REST` in `src/paimon/CMakeLists.txt`, so the OSS file system does not use it. OSS needs libcurl for two unrelated reasons: the OSS SDK's own curl transport, and the direct `curl_getdate` call in `oss_file_system.cpp`. Something like "libcurl is required by the REST catalog and the S3 HTTP client, and by the OSS SDK's curl transport" would describe the dependency more accurately. (If the shared timestamp helper suggested on `oss_file_system.cpp` lands in common code, the direct dependency from the OSS sources goes away as well.) -- 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]
