wgtmac commented on code in PR #444:
URL: https://github.com/apache/iceberg-cpp/pull/444#discussion_r2650378494
##########
src/iceberg/snapshot.h:
##########
@@ -250,14 +254,57 @@ struct ICEBERG_EXPORT Snapshot {
/// unknown.
std::optional<std::string_view> operation() const;
+ /// \brief Returns all ManifestFile instances for either data or delete
manifests
+ /// in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \param content The content type of the manifests to return
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> Manifests(std::shared_ptr<FileIO> file_io)
const;
+
+ /// \brief Returns a ManifestFile for each data manifest in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> DataManifests(std::shared_ptr<FileIO>
file_io) const;
+
+ /// \brief Returns a ManifestFile for each delete manifest in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> DeleteManifests(std::shared_ptr<FileIO>
file_io) const;
+
/// \brief Compare two snapshots for equality.
friend bool operator==(const Snapshot& lhs, const Snapshot& rhs) {
return lhs.Equals(rhs);
}
+ /// \brief Default constructor
+ Snapshot();
Review Comment:
If we have to add ctor to it, what about removing the default ctor and
adding ctor for all member variables and `Snapshot::Make` for validated object
creation? The Java `BaseSnapshot` has two ctors for v1 and others respectively.
##########
src/iceberg/snapshot.h:
##########
@@ -250,14 +254,57 @@ struct ICEBERG_EXPORT Snapshot {
/// unknown.
std::optional<std::string_view> operation() const;
+ /// \brief Returns all ManifestFile instances for either data or delete
manifests
+ /// in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \param content The content type of the manifests to return
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> Manifests(std::shared_ptr<FileIO> file_io)
const;
+
+ /// \brief Returns a ManifestFile for each data manifest in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> DataManifests(std::shared_ptr<FileIO>
file_io) const;
+
+ /// \brief Returns a ManifestFile for each delete manifest in this snapshot.
+ ///
+ /// \param file_io The FileIO instance to use for reading the manifest list
+ /// \return A span of ManifestFile instances, or an error
+ Result<std::span<ManifestFile>> DeleteManifests(std::shared_ptr<FileIO>
file_io) const;
+
/// \brief Compare two snapshots for equality.
friend bool operator==(const Snapshot& lhs, const Snapshot& rhs) {
return lhs.Equals(rhs);
}
+ /// \brief Default constructor
+ Snapshot();
+
+ /// \brief Destructor
+ ~Snapshot();
+
private:
/// \brief Compare two snapshots for equality.
bool Equals(const Snapshot& other) const;
+
+ /// \brief Cache structure for storing loaded manifests
+ struct ManifestsCache {
+ std::vector<ManifestFile> all_manifests;
Review Comment:
Can we avoid caching 2X manifest files here? As we return std::span for
different manifests, perhaps we can use a single vector to store data manifests
in the head and then delete manifests in the tail, and then use the split point
to construct different spans to return.
--
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]
