wgtmac commented on code in PR #770:
URL: https://github.com/apache/iceberg-cpp/pull/770#discussion_r3465667124
##########
src/iceberg/table_scan.h:
##########
@@ -302,6 +304,12 @@ class ICEBERG_TEMPLATE_CLASS_EXPORT TableScanBuilder :
public ErrorCollector {
/// \param num_rows The minimum number of rows requested
TableScanBuilder& MinRowsRequested(int64_t num_rows);
+ /// \brief Configure an executor for manifest planning.
+ ///
+ /// \param executor Executor to use while planning manifests.
+ /// \return Reference to this for method chaining.
+ TableScanBuilder& PlanWith(Executor& executor);
Review Comment:
Please document the executor lifetime here too. The built scan stores this
by reference and may use it later in `PlanFiles()`.
##########
src/iceberg/update/expire_snapshots.h:
##########
@@ -122,6 +122,12 @@ class ICEBERG_EXPORT ExpireSnapshots : public
PendingUpdate {
/// \return Reference to this for method chaining.
ExpireSnapshots& DeleteWith(std::function<void(const std::string&)>
delete_func);
+ /// \brief Configure an executor for planning expired snapshot metadata.
+ ///
+ /// \param executor Executor to use while planning expired snapshot metadata.
+ /// \return Reference to this for method chaining.
+ ExpireSnapshots& PlanWith(Executor& executor);
Review Comment:
Please add the same lifetime note as `ExecuteDeleteWith`. This stores only a
reference, and planning can run later during `Apply()` or `Finalize()`.
##########
src/iceberg/update/snapshot_update.h:
##########
@@ -81,6 +83,15 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
return self;
}
+ /// \brief Configure an executor for manifest planning work.
+ ///
+ /// \param executor Executor to use while planning manifests.
+ /// \return Reference to this for method chaining.
+ auto& ScanManifestsWith(this auto& self, Executor& executor) {
Review Comment:
Please document the executor lifetime here too. This stores only a reference
and `Apply()` may use it later.
##########
src/iceberg/delete_file_index.h:
##########
@@ -356,6 +357,12 @@ class ICEBERG_EXPORT DeleteFileIndex::Builder : public
ErrorCollector {
/// \brief Ignore residual expressions after partition filtering.
Builder& IgnoreResiduals();
+ /// \brief Configure an optional executor for reading delete manifests.
+ ///
+ /// \param executor Executor to use, or std::nullopt to read manifests
serially.
+ /// \return Reference to this for method chaining.
+ Builder& PlanWith(OptionalExecutor executor);
Review 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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
