lxy-9602 commented on code in PR #338:
URL: https://github.com/apache/paimon-cpp/pull/338#discussion_r4004470407
##########
src/paimon/core/manifest/manifest_file.cpp:
##########
@@ -90,7 +91,8 @@ Result<std::unique_ptr<ManifestFile>> ManifestFile::Create(
}
Status ManifestFile::ReadBucketEntries(const std::string& file_name, int32_t
bucket,
- std::vector<ManifestEntry>* entries)
const {
+ std::vector<ManifestEntry>* entries,
+ std::optional<int64_t> file_size) const
{
return ReadArrowBatches(
Review Comment:
Please place the output parameter at the end of the parameter list.
##########
src/paimon/core/utils/objects_file.h:
##########
@@ -63,11 +64,15 @@ class ObjectsFile {
virtual ~ObjectsFile() = default;
+ /// @param file_size Length of the file when planning already knows it,
which lets the read
+ /// skip the metadata request a bare `Open` issues on a
remote store. Leave it
+ /// unset when the length is not known; the read then
discovers it itself.
Status Read(const std::string& file_name, const
std::function<Result<bool>(const T&)>& filter,
- std::vector<T>* result) const;
+ std::vector<T>* result, std::optional<int64_t> file_size =
std::nullopt) const;
Status ReadIfFileExist(const std::string& file_name,
Review Comment:
Similarly, please avoid placing output parameters in the middle of the
parameter list, and avoid using default arguments.
##########
src/paimon/fs/jindo/jindo_file_system.cpp:
##########
@@ -173,11 +173,18 @@ Result<FileStatus> JindoFileSystem::GetFileStatus(const
std::string& path) const
Status JindoFileSystem::ListDir(const std::string& directory,
std::vector<BasicFileStatus>*
file_status_list) const {
- PAIMON_ASSIGN_OR_RAISE(bool exist, Exists(directory));
- if (!exist) {
- return Status::OK();
+ // One status call answers what Exists() followed by GetFileStatus() asked
the store twice:
+ // whether the path is there at all, and whether it is a directory.
PAIMON_RETURN_NOT_OK_FROM_
+ // JINDO maps the SDK's not-found to Status::NotExist, which is what tells
a missing directory
+ // (listed as empty, as the other file systems do) from a call that
genuinely failed.
+ Result<FileStatus> dir_status = GetFileStatus(directory);
+ if (!dir_status.ok()) {
+ if (dir_status.status().IsNotExist()) {
+ return Status::OK();
+ }
+ return dir_status.status();
Review Comment:
Could you confirm that Jindo’s `NotExist` status is correctly mapped to a
Paimon status, and that Jindo actually returns a `NotExist` status when the
target does not exist?
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -276,8 +276,33 @@ Status
FileStoreScan::ReadManifests(std::optional<Snapshot>* snapshot_ptr,
Status FileStoreScan::ReadManifestsWithSnapshot(const Snapshot& snapshot,
std::vector<ManifestFileMeta>*
manifests) const {
switch (scan_mode_) {
- case ScanMode::ALL:
- return manifest_list_->ReadDataManifests(snapshot, manifests);
+ case ScanMode::ALL: {
+ // The base and the delta manifest list are two independent files
and neither read
+ // depends on the other, so issue both together instead of paying
the two metadata
+ // round trips one after the other. The result keeps the
base-then-delta order that
+ // ReadDataManifests produced.
+ auto read_list = [this, &snapshot](bool base) ->
Result<std::vector<ManifestFileMeta>> {
+ std::vector<ManifestFileMeta> metas;
+ PAIMON_RETURN_NOT_OK(base ?
manifest_list_->ReadBaseManifests(snapshot, &metas)
+ :
manifest_list_->ReadDeltaManifests(snapshot, &metas));
+ return metas;
+ };
+ std::vector<std::future<Result<std::vector<ManifestFileMeta>>>>
futures;
+ futures.reserve(2);
+ futures.push_back(
+ Via(executor_.get(), [read_list]() { return
read_list(/*base=*/true); }));
+ futures.push_back(
+ Via(executor_.get(), [read_list]() { return
read_list(/*base=*/false); }));
+ for (auto& metas : CollectAll(futures)) {
+ if (!metas.ok()) {
+ return metas.status();
+ }
+ for (auto& meta : metas.value()) {
+ manifests->emplace_back(std::move(meta));
+ }
+ }
+ return Status::OK();
Review Comment:
There are only two files here in total, one base file and one delta file. Is
multithreading really necessary in this case?
##########
src/paimon/core/manifest/manifest_file.h:
##########
@@ -63,8 +64,12 @@ class ManifestFile : public ObjectsFile<ManifestEntry> {
Result<std::vector<ManifestFileMeta>> Write(const
std::vector<ManifestEntry>& entries);
/// Read a manifest file and deserialize only entries for the specified
bucket.
+ ///
+ /// @param file_size Length of the manifest when the caller already has it
from the manifest
+ /// list, which saves the read a metadata request on a
remote store.
Status ReadBucketEntries(const std::string& file_name, int32_t bucket,
- std::vector<ManifestEntry>* entries) const;
+ std::vector<ManifestEntry>* entries,
+ std::optional<int64_t> file_size = std::nullopt)
const;
Review Comment:
Please avoid using default arguments in production code.
--
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]
