-
Notifications
You must be signed in to change notification settings - Fork 113
feat: write snapshot v3 row lineage fields at top level #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,6 +162,10 @@ std::optional<std::string_view> Snapshot::Operation() const { | |
| } | ||
|
|
||
| Result<std::optional<int64_t>> Snapshot::FirstRowId() const { | ||
| if (first_row_id.has_value()) { | ||
| return first_row_id; | ||
| } | ||
|
|
||
| auto it = summary.find(SnapshotSummaryFields::kFirstRowId); | ||
| if (it == summary.end()) { | ||
| return std::nullopt; | ||
|
|
@@ -171,6 +175,10 @@ Result<std::optional<int64_t>> Snapshot::FirstRowId() const { | |
| } | ||
|
|
||
| Result<std::optional<int64_t>> Snapshot::AddedRows() const { | ||
| if (added_rows.has_value()) { | ||
| return added_rows; | ||
| } | ||
|
|
||
| auto it = summary.find(SnapshotSummaryFields::kAddedRows); | ||
| if (it == summary.end()) { | ||
| return std::nullopt; | ||
|
|
@@ -186,7 +194,8 @@ bool Snapshot::Equals(const Snapshot& other) const { | |
| return snapshot_id == other.snapshot_id && | ||
| parent_snapshot_id == other.parent_snapshot_id && | ||
| sequence_number == other.sequence_number && timestamp_ms == other.timestamp_ms && | ||
| schema_id == other.schema_id; | ||
| schema_id == other.schema_id && first_row_id == other.first_row_id && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure these fields are required in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks odd if two snapshots with different row lineage attributes are deemed identical. I would rather think this is an oversight from apache/iceberg#11948. Both iceberg-rust and iceberg-python enforce all fields to be equal so I think it is fine to include them here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Opened apache/iceberg#17015 to fix at Java side. |
||
| added_rows == other.added_rows; | ||
| } | ||
|
|
||
| Result<std::unique_ptr<Snapshot>> Snapshot::Make( | ||
|
|
@@ -203,12 +212,6 @@ Result<std::unique_ptr<Snapshot>> Snapshot::Make( | |
| ICEBERG_PRECHECK(!first_row_id.has_value() || added_rows.has_value(), | ||
| "Missing added-rows when first-row-id is set"); | ||
| summary[SnapshotSummaryFields::kOperation] = operation; | ||
| if (first_row_id.has_value()) { | ||
| summary[SnapshotSummaryFields::kFirstRowId] = std::to_string(first_row_id.value()); | ||
| } | ||
| if (added_rows.has_value()) { | ||
| summary[SnapshotSummaryFields::kAddedRows] = std::to_string(added_rows.value()); | ||
| } | ||
| return std::make_unique<Snapshot>(Snapshot{ | ||
| .snapshot_id = snapshot_id, | ||
| .parent_snapshot_id = parent_snapshot_id, | ||
|
|
@@ -217,6 +220,8 @@ Result<std::unique_ptr<Snapshot>> Snapshot::Make( | |
| .manifest_list = std::move(manifest_list), | ||
| .summary = std::move(summary), | ||
| .schema_id = schema_id, | ||
| .first_row_id = first_row_id, | ||
| .added_rows = first_row_id.has_value() ? added_rows : std::nullopt, | ||
| }); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.