Copilot commented on code in PR #873:
URL: https://github.com/apache/iceberg-cpp/pull/873#discussion_r4015190678
##########
src/iceberg/manifest/manifest_group.h:
##########
@@ -134,7 +138,24 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector
{
ManifestGroup& WithScanMetrics(std::shared_ptr<ScanMetrics> scan_metrics);
/// \brief Plan scan tasks for all matching data files.
- Result<std::vector<std::shared_ptr<FileScanTask>>> PlanFiles();
+ ///
+ /// Consumes this group and collects PlanFilesStream() into a vector.
+ Result<std::vector<std::shared_ptr<FileScanTask>>> PlanFiles() &&;
Review Comment:
Adding the `&&` ref-qualifier breaks existing callers of the public eager
API that call `group->PlanFiles()` on an lvalue; the test changes had to add
`std::move` everywhere. This also conflicts with the stated promise that eager
`PlanFiles()` remains available. Retain an lvalue-compatible overload (and keep
the consuming stream overload separate).
##########
src/iceberg/manifest/manifest_reader.h:
##########
@@ -30,24 +30,46 @@
#include <vector>
#include "iceberg/iceberg_export.h"
+#include "iceberg/manifest/manifest_entry.h"
#include "iceberg/metrics/counter.h"
#include "iceberg/result.h"
#include "iceberg/type_fwd.h"
+#include "iceberg/util/stream.h"
namespace iceberg {
+/// \brief Stream of manifest entries.
+using ManifestEntryStream = Stream<ManifestEntry>;
+
+/// \brief Owning pointer to a manifest entry stream.
+using ManifestEntryStreamPtr = std::unique_ptr<ManifestEntryStream>;
+
/// \brief Read manifest entries from a manifest file.
class ICEBERG_EXPORT ManifestReader {
public:
virtual ~ManifestReader() = default;
/// \brief Read all manifest entries in the manifest file.
///
- /// TODO(gangwu): provide a lazy-evaluated iterator interface for better
performance.
- virtual Result<std::vector<ManifestEntry>> Entries() = 0;
+ /// Collects EntriesStream() into a vector.
+ Result<std::vector<ManifestEntry>> Entries();
/// \brief Read only live (non-deleted) manifest entries.
- virtual Result<std::vector<ManifestEntry>> LiveEntries() = 0;
+ ///
+ /// Collects LiveEntriesStream() into a vector.
+ Result<std::vector<ManifestEntry>> LiveEntries();
+
+ /// \brief Lazily read manifest entries.
+ ///
+ /// The returned stream is fallible and single-pass. It must own all
resources
+ /// required for consumption and must not depend on this reader remaining
alive.
+ virtual Result<ManifestEntryStreamPtr> EntriesStream() = 0;
Review Comment:
These new pure virtual stream methods replace the old virtual
`Entries()`/`LiveEntries()` extension points, so existing downstream
`ManifestReader` implementations no longer compile and cannot use the promised
eager fallback. Keep the old virtual methods and provide default stream
adapters that collect them, while the built-in reader overrides the lazy
methods.
--
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]
