HeartLinked commented on code in PR #176:
URL: https://github.com/apache/iceberg-cpp/pull/176#discussion_r2281191236
##########
src/iceberg/manifest_writer.h:
##########
@@ -35,14 +35,24 @@ namespace iceberg {
class ICEBERG_EXPORT ManifestWriter {
public:
virtual ~ManifestWriter() = default;
- virtual Status WriteManifestEntries(
- const std::vector<ManifestEntry>& entries) const = 0;
+
+ /// \brief Write manifest entry to file
Review Comment:
```suggestion
/// \brief Write manifest entry to file.
```
##########
src/iceberg/manifest_writer.h:
##########
@@ -51,13 +61,27 @@ class ICEBERG_EXPORT ManifestWriter {
class ICEBERG_EXPORT ManifestListWriter {
public:
virtual ~ManifestListWriter() = default;
- virtual Status WriteManifestFiles(const std::vector<ManifestFile>& files)
const = 0;
+
+ /// \brief Write manifest file list to manifest list file.
Review Comment:
The comment "Write manifest file list" here is inconsistent with the
single-add function.
##########
src/iceberg/manifest_writer.h:
##########
@@ -51,13 +61,27 @@ class ICEBERG_EXPORT ManifestWriter {
class ICEBERG_EXPORT ManifestListWriter {
public:
virtual ~ManifestListWriter() = default;
- virtual Status WriteManifestFiles(const std::vector<ManifestFile>& files)
const = 0;
+
+ /// \brief Write manifest file list to manifest list file.
Review Comment:
The comment "Write manifest file list" here is inconsistent with the
single-add function.
##########
src/iceberg/manifest_writer.h:
##########
@@ -35,14 +35,24 @@ namespace iceberg {
class ICEBERG_EXPORT ManifestWriter {
public:
virtual ~ManifestWriter() = default;
- virtual Status WriteManifestEntries(
- const std::vector<ManifestEntry>& entries) const = 0;
+
+ /// \brief Write manifest entry to file
+ /// \param entry Manifest entry to write.
+ /// \return Status::OK() if all entry was written successfully
+ virtual Status WriteManifestEntry(const ManifestEntry& entry) const = 0;
+
+ /// \brief Close writer and flush to storage.
+ virtual Status Close() = 0;
/// \brief Creates a writer for a manifest file.
+ /// \param format_version Format version of the manifest.
+ /// \param snapshot_id ID of the snapshot.
+ /// \param first_row_id First row ID of the snapshot.
/// \param manifest_location Path to the manifest file.
/// \param file_io File IO implementation to use.
/// \return A Result containing the writer or an error.
static Result<std::unique_ptr<ManifestWriter>> MakeWriter(
+ int32_t format_version, int64_t snapshot_id, int64_t first_row_id,
Review Comment:
> Should we make some parameters optional, like first_row_id? Same question
for ManifestListWriter.
I agree, in the Java implementation, different versions from v1 to v4 are
achieved by having subclasses inherit from a base writer class, which
conveniently allows different construction parameters for each version. I find
this approach more intuitive. If we don't implement it this way, should we at
least create multiple constructors with different parameters to explicitly
distinguish between versions?
--
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]
