Copilot commented on code in PR #873:
URL: https://github.com/apache/iceberg-cpp/pull/873#discussion_r3795934394
##########
src/iceberg/manifest/manifest_reader.h:
##########
@@ -42,13 +43,24 @@ class ICEBERG_EXPORT ManifestReader {
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;
/// \brief Read only live (non-deleted) manifest entries.
virtual Result<std::vector<ManifestEntry>> LiveEntries() = 0;
+ /// \brief Lazily read manifest entries.
+ ///
+ /// The returned iterator reads and filters one underlying record batch at a
time. This
+ /// bounds memory use for large manifests. The iterator owns its reader
resources and
+ /// may outlive this ManifestReader.
+ virtual Result<std::unique_ptr<Iterator<ManifestEntry>>> EntriesIterator();
+
+ /// \brief Lazily read only live (non-deleted) manifest entries.
+ ///
+ /// The default implementation adapts LiveEntries() for compatibility with
custom reader
+ /// implementations. Built-in readers override this with a streaming
implementation.
+ virtual Result<std::unique_ptr<Iterator<ManifestEntry>>>
LiveEntriesIterator();
Review Comment:
Adding new virtual methods to an exported polymorphic base
(`ManifestReader`) is an ABI-breaking change for any out-of-tree/custom
implementations that were compiled against the previous header (even though
defaults exist). A more ABI-compatible approach is to keep these as non-virtual
helper methods on `ManifestReader` (implemented in terms of `Entries()` /
`LiveEntries()`), and introduce a separate optional extension interface (e.g.,
a new derived interface with the iterator virtuals) that built-in readers
implement; callers can `dynamic_cast` to the extension and otherwise fall back
to the helper/adaptor.
--
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]
