Skip to content
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.

Sign up for GitHub

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

Spec: Update partition stats for V3 #12098

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+25 −19
Open

Spec: Update partition stats for V3 #12098

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb8cbf2
Spec: Update partition stats for V3
aokolnychyi Jan 24, 2025
6a663bd
Simplify
aokolnychyi Jan 24, 2025
b57dcb1
Cover table above
aokolnychyi Feb 1, 2025
72fcdc8
Clarify
aokolnychyi Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions format/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,11 +914,11 @@ Partition statistics file must be registered in the table metadata file to be co

`partition-statistics` field of table metadata is an optional list of structs with the following fields:

| v1 | v2 | Field name | Type | Description |
|----|----|------------|------|-------------|
| _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg table's snapshot the partition statistics file is associated with. |
| _required_ | _required_ | **`statistics-path`** | `string` | Path of the partition statistics file. See [Partition statistics file](#partition-statistics-file). |
| _required_ | _required_ | **`file-size-in-bytes`** | `long` | Size of the partition statistics file. |
| v1 | v2 | v3 | Field name | Type | Description |
|----|----|----|------------|------|-------------|
| _required_ | _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg table's snapshot the partition statistics file is associated with. |
| _required_ | _required_ | _required_ | **`statistics-path`** | `string` | Path of the partition statistics file. See [Partition statistics file](#partition-statistics-file). |
| _required_ | _required_ | _required_ | **`file-size-in-bytes`** | `long` | Size of the partition statistics file. |

##### Partition Statistics File

Expand All @@ -927,20 +927,21 @@ These rows must be sorted (in ascending manner with NULL FIRST) by `partition` f

The schema of the partition statistics file is as follows:

| v1 | v2 | Field id, name | Type | Description |
|----|----|----------------|------|-------------|
| _required_ | _required_ | **`1 partition`** | `struct<..>` | Partition data tuple, schema based on the unified partition type considering all specs in a table |
| _required_ | _required_ | **`2 spec_id`** | `int` | Partition spec id |
| _required_ | _required_ | **`3 data_record_count`** | `long` | Count of records in data files |
| _required_ | _required_ | **`4 data_file_count`** | `int` | Count of data files |
| _required_ | _required_ | **`5 total_data_file_size_in_bytes`** | `long` | Total size of data files in bytes |
| _optional_ | _optional_ | **`6 position_delete_record_count`** | `long` | Count of records in position delete files |
| _optional_ | _optional_ | **`7 position_delete_file_count`** | `int` | Count of position delete files |
| _optional_ | _optional_ | **`8 equality_delete_record_count`** | `long` | Count of records in equality delete files |
| _optional_ | _optional_ | **`9 equality_delete_file_count`** | `int` | Count of equality delete files |
| _optional_ | _optional_ | **`10 total_record_count`** | `long` | Accurate count of records in a partition after applying the delete files if any |
| _optional_ | _optional_ | **`11 last_updated_at`** | `long` | Timestamp in milliseconds from the unix epoch when the partition was last updated |
| _optional_ | _optional_ | **`12 last_updated_snapshot_id`** | `long` | ID of snapshot that last updated this partition |
| v1 | v2 | v3 | Field id, name | Type | Description |
aokolnychyi marked this conversation as resolved.
Show resolved Hide resolved
|----|----|----|----------------|------|-------------|
| _required_ | _required_ | _required_ | **`1 partition`** | `struct<..>` | Partition data tuple, schema based on the unified partition type considering all specs in a table |
| _required_ | _required_ | _required_ | **`2 spec_id`** | `int` | Partition spec id |
| _required_ | _required_ | _required_ | **`3 data_record_count`** | `long` | Count of records in data files |
| _required_ | _required_ | _required_ | **`4 data_file_count`** | `int` | Count of data files |
| _required_ | _required_ | _required_ | **`5 total_data_file_size_in_bytes`** | `long` | Total size of data files in bytes |
| _optional_ | _optional_ | _required_ | **`6 position_delete_record_count`** | `long` | Count of position deletes across position delete files and deletion vectors |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale behind combining the DV and position delete record count in the position_delete_record_count ? Better to keep a new record counter for DVs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've elaborated here. Let me know what you think, @ajantha-bhat!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

| _optional_ | _optional_ | _required_ | **`7 position_delete_file_count`** | `int` | Count of position delete files ignoring deletion vectors |
Copy link
Member

@ajantha-bhat ajantha-bhat Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why the delete stats are required field in v3? IIRC, Tables may not have delete operations. So, it is was made optional in V2. So, it should be optional for v3 also?

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, there's value in having these be required because having null in these fields isn't really that informative since it just means that the state is unknown and needs to be derived using other metadata. Populating these fields accurately isn't that much of a burden for writers (so if there are no deletes, it'll just be 0) and eliminates any special casing around having to deal with null values here. I'd imagine the potential extra storage from these fields is also pretty negligible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, having this as optional is ambiguous. In fact, we should clarify that NULL means 0 in v1 and v2. What do you think about this, @ajantha-bhat? If you agree, can you start a discussion on the dev list as you know this part best?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for starting the discussions.

| | | _required_ | **`13 dv_count`** | `int` | Count of deletion vectors |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using dv in the key to match what we did for snapshot summaries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should append this line to the end of this table, which reduces the code change and matches the field id in ascending order.

Copy link
Member

@ajantha-bhat ajantha-bhat Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, keeping in the end maybe useful for choosing next schema field id easily. Else user might have to go through each field to see whether next field id is reused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@advancedxy @ajantha-bhat Agreed, I think the original intention was to group it with similar fields but I feel like it's better to optimize this table for future evolution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I OK either way. Adding it to the end won't reduce the amount of changes, though. We still need to add the V3 column.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aokolnychyi do we need a new field for dv_record_count?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevenzwu, I don't think so but can be convinced otherwise. From the spec perspective, the content of DVs is position deletes. That's what we store in manifests. Also, we combine the number of position deletes across the old-style position delete files and DVs in snapshot summaries. Therefore, the decision to reuse the existing field for the position delete record count is consistent.

Are there use cases that would benefit if we had dv_record_count as an independent field?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking about future deprecation. When position delete file is deprecated in the future, position_delete_record_count and position_delete_file_count fields can be deprecated and removed.

Copy link
Contributor Author

@aokolnychyi aokolnychyi Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original thinking was to keep position_delete_record_count beyond V3 similar to snapshot summary stats, primarily because I consider DV content as position deletes. In V3, this field will contain a sum of record counts across old-style position delete files and DVs. In V4, we will only contain DVs but the field will be the same.

We may deprecate position_delete_file_count in V4 but that would mean that V4 prohibits any existing V2 delete files. That's something unclear at the moment.

How do you see this, @stevenzwu?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I consider DV content as position deletes

I can understand this reasoning.

Is there any value to show separate position_delete_record_count and dv_record_count to help users understand the state of the table in row-level deletes. Maybe the information can help users to make a decision if it makes sense to convert position delete files to delete vectors?

It also feels more symmetric to have position_delete_record_count and position_delete_file_count (vs) dv_record_count and dv_count.

if we do decide to deprecate position_delete_file_count in the future, we would have left with position_delete_record_count and dv_count, which seems a bit inconsistent in naming.

| _optional_ | _optional_ | _required_ | **`8 equality_delete_record_count`** | `long` | Count of records in equality delete files |
| _optional_ | _optional_ | _required_ | **`9 equality_delete_file_count`** | `int` | Count of equality delete files |
| _optional_ | _optional_ | _optional_ | **`10 total_record_count`** | `long` | Accurate count of records in a partition after applying deletes if any |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be required aswell for v3? Since we decided to keep it same as other counters during implementation.
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/PartitionStats.java#L36

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can make this required and I am afraid we have to fix the implementation. If there are equality deletes, determining this would be very expensive. If we don't have equality deletes and have only DVs, then we can populate this value without an expensive computation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether we compute or not is optional and I agree.

But we decided that all the counters must be of same behavior (zero initialized if not computed or not present). So, making it null in the implementation is not a good idea? It can be required and initialized to zero always if we don't compute it?

If not, I am even thinking to remove this field as it is hard to compute. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added more context to this PR around total_record_count. I think it differs from other counters and DVs actually enable cheap and precise computation of it without reading data!

| _optional_ | _optional_ | _optional_ | **`11 last_updated_at`** | `long` | Timestamp in milliseconds from the unix epoch when the partition was last updated |
| _optional_ | _optional_ | _optional_ | **`12 last_updated_snapshot_id`** | `long` | ID of snapshot that last updated this partition |

Note that partition data tuple's schema is based on the partition spec output using partition field ids for the struct field ids.
The unified partition type is a struct containing all fields that have ever been a part of any spec in the table
Expand All @@ -957,6 +958,11 @@ The unified partition type looks like `Struct<field#1, field#2, field#3>`.
and then the table has evolved into `spec#1` which has just one field `{field#2}`.
The unified partition type looks like `Struct<field#1, field#2>`.

When a v2 table is upgraded to v3 or later, the `position_delete_record_count` field must account for all position deletes, including those from remaining v2 position delete files and any deletion vectors added after the upgrade.

Calculating `total_record_count` for a table with equality deletes or v2 position delete files requires reading data. In such cases, implementations may omit this field and must write `NULL`, indicating that the exact record count in a partition is unknown.
If a table has no deletes or only deletion vectors, implementations are encouraged to populate `total_record_count` using metadata in manifests.

### Commit Conflict Resolution and Retry

When two commits happen at the same time and are based on the same version, only one commit will succeed. In most cases, the failed commit can be applied to the new current version of table metadata and retried. Updates verify the conditions under which they can be applied to a new version and retry if those conditions are met.
Expand Down