feat: parallelize update and scan processing#770
Conversation
HuaHuaY
force-pushed
the
parallel_optimize
branch
from
dbe9af7 to
ac14be2
Compare
HuaHuaY
force-pushed
the
parallel_optimize
branch
from
ac14be2 to
e5da646
Compare
| plan_executor_, manifests_to_delete, | ||
| [this, &metadata]( | ||
| const ManifestFile& manifest) -> Result<std::unordered_set<std::string>> { | ||
| auto result = ReadLiveDataFilePaths(metadata, manifest); |
There was a problem hiding this comment.
Can we handle delete manifests here too? ReadLiveDataFilePaths rejects them, so reachable cleanup removes the delete manifest but leaves the delete file behind. Java uses ManifestFiles.readPaths, which reads both data and delete manifests.
There was a problem hiding this comment.
This isn't a problem introduced by this PR. It can be fixed in a future PR.
| /// | ||
| /// \param executor Executor to use while planning expired snapshot metadata. | ||
| /// \return Reference to this for method chaining. | ||
| ExpireSnapshots& PlanWith(Executor& executor); |
There was a problem hiding this comment.
Please add the same lifetime note as ExecuteDeleteWith. This stores only a reference, and planning can run later during Apply() or Finalize().
There was a problem hiding this comment.
I removed the comment from ExecuteDeleteWith. The existence of the executor is self-evident, guaranteed by the caller.
| /// | ||
| /// \param executor Executor to use while planning manifests. | ||
| /// \return Reference to this for method chaining. | ||
| TableScanBuilder& PlanWith(Executor& executor); |
There was a problem hiding this comment.
Please document the executor lifetime here too. The built scan stores this by reference and may use it later in PlanFiles().
| /// | ||
| /// \param executor Executor to use while planning manifests. | ||
| /// \return Reference to this for method chaining. | ||
| auto& ScanManifestsWith(this auto& self, Executor& executor) { |
There was a problem hiding this comment.
Please document the executor lifetime here too. This stores only a reference and Apply() may use it later.
| /// | ||
| /// \param executor Executor to use, or std::nullopt to read manifests serially. | ||
| /// \return Reference to this for method chaining. | ||
| Builder& PlanWith(OptionalExecutor executor); |
There was a problem hiding this comment.
Can this public API also take Executor&? If OptionalExecutor is only internal plumbing, it should live in an _internal.h header so users do not depend on it.
There was a problem hiding this comment.
We can rename the file in future PR.
wgtmac
changed the title
2 participants
No description provided.