feat: add partition summary and write added/existing/deleted entries to manifest writer #317

zhjwpku · 2025-11-14T01:23:57Z

including AddEntry, AddExistingEntry, AddDeleteEntry

zhjwpku · 2025-11-17T02:08:00Z

Some unclear points while porting Java's TestManifestWriterVersions:

Should we update the ManifestFile's sequence number when writing to ManifestList?
Should we update ManifestEntry's status to Delete when performing AddDeleteEntry?

I've temporarily commented out some failing assertions, but the PR seems ready for review, just pay special attention to the above two questions.

src/iceberg/partition_summary_internal.h

src/iceberg/partition_summary_internal.cc

src/iceberg/metrics.h

src/iceberg/manifest_writer.h

src/iceberg/manifest_writer.cc

src/iceberg/manifest_adapter.h

wgtmac · 2025-11-19T06:15:03Z

src/iceberg/manifest_writer.h

  /// \param entry Manifest entry to write.
  /// \return Status::OK() if entry was written successfully
-  Status Add(const ManifestEntry& entry);
+  Status AddEntry(const ManifestEntry& entry);


What about adding following functions to deal with different use cases:

// Write the entry that all its fields are populated correctly. Status WriteEntry(const ManifestEntry& entry); // Write the input as a new entry to add. Status WriteAddedEntry(std::shared_ptr<DataFile> file, std::optional<int64_t> data_sequence_number); Status WriteAddedEntry(const ManifestEntry& entry); // Write the input as an existing entry. Status WriteExistingEntry(std::shared_ptr<DataFile> file, int64_t file_snapshot_id, int64_t data_sequence_number, std::optional<int64_t> file_sequence_number); Status WriteExistingEntry(const ManifestEntry& entry); // Write the input as a deleted entry. Status WriteDeletedEntry(std::shared_ptr<DataFile> file, int64_t data_sequence_number, std::optional<int64_t> file_sequence_number); Status WriteDeletedEntry(const ManifestEntry& entry);

WriteEntry can be the delegate function to accept writes from other functions. Inside each WriteXXXEntry function, we can just create a new entry and the copy should be cheap.

Good idea, I tried this, please take a look again.

including AddEntry, AddExistingEntry, AddDeleteEntry

Jinchul81 · 2025-11-23T10:56:46Z

src/iceberg/manifest_adapter.cc

+  entry.snapshot_id = snapshot_id_;
+  if (entry.sequence_number.has_value() &&
+      entry.sequence_number.value() < TableMetadata::kInitialSequenceNumber) {
+    entry.sequence_number = std::nullopt;


Please add a comment why sequence_number should be initialized here.

This logic has been removed since GetSequenceNumber handle this, the reason for it been null is because for newly added data, its sequence number is inherited from manifest metadata, see [1].

[1] https://iceberg.apache.org/spec/#sequence-number-inheritance

src/iceberg/manifest_adapter.cc

Jinchul81 · 2025-11-23T11:04:01Z

src/iceberg/manifest_adapter.cc

+  manifest_file.added_rows_count = add_rows_count_;
+  manifest_file.existing_rows_count = existing_rows_count_;
+  manifest_file.deleted_rows_count = delete_rows_count_;
+  manifest_file.partitions = std::move(partition_summary_->Summaries());


Please clarify the ownership movement. Please add a comment why it is required.

This has been changed to:

ICEBERG_ASSIGN_OR_RAISE(auto partition_summary, partition_summary_->Summaries()); manifest_file.partitions = std::move(partition_summary);

I agree at the version of you comment, the move is not necessary, I guess meson build would warn under warning_level=2. But after changing, the move can avoid an extra copy.

After seeing Gang's comment, I change it to the following:

ICEBERG_ASSIGN_OR_RAISE(manifest_file.partitions, partition_summary_->Summaries());

src/iceberg/manifest_list.h

Jinchul81 · 2025-11-23T11:17:30Z

src/iceberg/manifest_list.h


 /// \brief Get the relative manifest content type from name
-ICEBERG_EXPORT constexpr Result<ManifestFile::Content> ManifestFileContentFromString(
+ICEBERG_EXPORT constexpr Result<ManifestContent> ManifestContentFromString(


Please make it a static function.

I'm a bit confused here, are you worried that this function will be included in multiple translation units?
If so, constexpr implies inline, so this should be fine. We already use this pattern in several other places, correct me if I'm wrong.

Jinchul81 · 2025-11-23T11:38:24Z

src/iceberg/partition_summary_internal.h

+
+  Status Update(const Literal& value);
+
+  PartitionFieldSummary Finish() const;


Please add a comment.

Jinchul81 · 2025-11-23T11:39:29Z

src/iceberg/partition_summary_internal.h

+
+ private:
+  /// \brief Create a PartitionSummary with the given field stats.
+  explicit PartitionSummary(std::vector<PartitionFieldStats> field_stats)


Why should we hide the ctor? Please add a comment for reasoning.

The reason was there was a Make method, but now I remove the Make and bring the ctor to public.

src/iceberg/test/manifest_reader_writer_test.cc

src/iceberg/v2_metadata.cc

Jinchul81 · 2025-11-23T11:43:41Z

src/iceberg/v3_metadata.cc


-Status ManifestFileAdapterV3::Append(const ManifestFile& file) {
+Status ManifestFileAdapterV3::Append(ManifestFile& file) {
+  // TODO(zhjwpku): Should we set sequence_number/first_row_id here?


Please file an issue and use the tracking id instead of your id. I know other code already used personal id, but it's not helpful to follow what to do.

These TODOs were meant to attract reviewers' closer attention. I've removed them now.

wgtmac

Thanks for the update! I think it generally looks good now. I've left some inline comments.

src/iceberg/manifest_list.h

src/iceberg/manifest_writer.cc

src/iceberg/manifest_writer.h

src/iceberg/manifest_adapter.cc

src/iceberg/manifest_writer.cc

src/iceberg/v2_metadata.cc

src/iceberg/manifest_adapter.cc

src/iceberg/metrics.h

- unify partition summary collection - fix manifest reader to inherit metadata - fix first row id round trip

wgtmac · 2025-11-25T07:25:17Z

Thanks @zhjwpku for working on this and adding sufficient test cases! I've pushed some changes to fix first_row_id and sequence_number inheritance.

Thanks for your review, @Jinchul81! Do you have any other comment?

zhjwpku force-pushed the manifest_writer_with_add_entry branch 5 times, most recently from 138cee5 to 7a4c48e Compare November 16, 2025 13:15

zhjwpku marked this pull request as ready for review November 16, 2025 14:25

zhjwpku requested a review from wgtmac November 17, 2025 02:08

wgtmac requested changes Nov 19, 2025

View reviewed changes

zhjwpku added 3 commits November 21, 2025 22:18

feat: adopt partition summary & add AddEntry variants

637a55a

including AddEntry, AddExistingEntry, AddDeleteEntry

fix: review comments

0f40163

fix: resolve a rebase conflict

e0d03e6

zhjwpku force-pushed the manifest_writer_with_add_entry branch from 7a4c48e to e0d03e6 Compare November 21, 2025 14:24

zhjwpku requested a review from wgtmac November 21, 2025 14:53

Jinchul81 reviewed Nov 23, 2025

View reviewed changes

wgtmac reviewed Nov 24, 2025

View reviewed changes

zhjwpku and others added 2 commits November 25, 2025 00:10

fix: review comments

77c86cf

various fixes:

2e2f49c

- unify partition summary collection - fix manifest reader to inherit metadata - fix first row id round trip

wgtmac changed the title ~~feat: adopt partition summary & add AddEntry variants~~ feat: add partition summary and write added/existing/deleted entries to manifest writer Nov 25, 2025

wgtmac approved these changes Nov 25, 2025

View reviewed changes

wgtmac merged commit c142ef9 into apache:main Nov 26, 2025
10 checks passed

zhjwpku deleted the manifest_writer_with_add_entry branch November 30, 2025 10:46


		Status Update(const Literal& value);

		PartitionFieldSummary Finish() const;

feat: add partition summary and write added/existing/deleted entries to manifest writer #317

feat: add partition summary and write added/existing/deleted entries to manifest writer #317

Uh oh!

Conversation

zhjwpku commented Nov 14, 2025

Uh oh!

zhjwpku commented Nov 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac commented Nov 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants