Spec: Update partition stats for V3 #12098

aokolnychyi · 2025-01-24T19:53:34Z

This PR adapts our partition stats spec to accommodate DVs:

position_delete_record_count includes a sum of position deletes across position delete files and DVs
position_delete_file_count still counts only position delete files (ignoring deletion vectors)
dv_count counts the number of deletion vectors

aokolnychyi · 2025-01-24T21:17:32Z

format/spec.md

+| _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 |
+| _optional_ | _optional_ | _required_ | **`7 position_delete_file_count`** | `int` | Count of position delete files ignoring deletion vectors |
+|            |            | _required_ | **`13 dv_count`** | `int` | Count of deletion vectors |


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

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.

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.

@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

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.

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.

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?

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.

I spent a bit more time thinking about this and would still opt in for not introducing a new field to be consistent with how we handle these counts elsewhere (e.g. SnapshotSummary). DVs store position deletes so reusing the existing count is appropriate. My gut tells me it is unlikely the record count in DVs vs position delete files would matter based on our discussions around table maintenance.

@stevenzwu, what do you think? If we decide to add a separate field here, we will have to align all places.

I see. I agree it is not worthwhile if it would introduce pretty significant disruptions to many parts.

what about renaming dv_count to position_delete_vector_count? that would at least align the naming more consistently.

aokolnychyi · 2025-01-24T21:20:58Z

cc @ajantha-bhat @rdblue @nastra @Fokko @RussellSpitzer @huaxingao @amogh-jahagirdar

Let me know if there is any early feedback prior to starting the vote.

ajantha-bhat · 2025-01-25T17:23:35Z

format/spec.md

+| _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 |
+| _optional_ | _optional_ | _required_ | **`7 position_delete_file_count`** | `int` | Count of position delete files ignoring deletion vectors |


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?

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.

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?

thanks for starting the discussions.

format/spec.md

ajantha-bhat · 2025-02-03T12:29:45Z

format/spec.md

+|            |            | _required_ | **`13 dv_count`** | `int` | Count of deletion vectors |
+| _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 |


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

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.

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?

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!

ajantha-bhat · 2025-02-03T12:35:20Z

format/spec.md

+| _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 |


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?

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

Thanks for the explanation.

ajantha-bhat

LGTM.

Looks clear to me now.

stevenzwu

LGTM.

I will leave the decision to you if it makes sense to rename dv_count to position_delete_vector_count for align the naming with other 2 fields?

ajantha-bhat · 2025-03-07T10:41:47Z

Looks like we have everyone's approval. Can we please merge this?

github-actions · 2025-04-07T00:17:18Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2025-04-14T00:17:51Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

aokolnychyi · 2025-04-29T22:46:10Z

This became stale. I reopened it.

ajantha-bhat · 2025-04-30T02:39:43Z

@aokolnychyi: Looks like we had enough approvals and you can merge this?

aokolnychyi · 2025-05-05T20:59:49Z

@ajantha-bhat, let me rebase and notify the dev list.

aokolnychyi · 2025-05-05T21:22:09Z

Thanks everyone!

github-actions bot added the Specification Issues that may introduce spec changes. label Jan 24, 2025

aokolnychyi commented Jan 24, 2025

View reviewed changes

RussellSpitzer approved these changes Jan 24, 2025

View reviewed changes

huaxingao approved these changes Jan 24, 2025

View reviewed changes

ajantha-bhat reviewed Jan 25, 2025

View reviewed changes

format/spec.md Show resolved Hide resolved

amogh-jahagirdar approved these changes Feb 2, 2025

View reviewed changes

advancedxy approved these changes Feb 2, 2025

View reviewed changes

ajantha-bhat reviewed Feb 3, 2025

View reviewed changes

ajantha-bhat approved these changes Feb 6, 2025

View reviewed changes

This was referenced Feb 10, 2025

Data: Add partition stats writer and reader #11216

Merged

Core: Make totalRecordCount optional in PartitionStats #12226

Merged

stevenzwu approved these changes Feb 14, 2025

View reviewed changes

github-actions bot added the stale label Apr 7, 2025

github-actions bot closed this Apr 14, 2025

aokolnychyi reopened this Apr 29, 2025

github-actions bot removed the stale label Apr 30, 2025

Spec: Update partition stats for V3

a3d572e

aokolnychyi force-pushed the partition-stats branch from 72fcdc8 to a3d572e Compare May 5, 2025 21:09

aokolnychyi merged commit a214e9b into apache:main May 5, 2025
2 checks passed

ajantha-bhat mentioned this pull request May 29, 2025

Support DV for partition stats #13180

Closed

3 tasks

ajantha-bhat mentioned this pull request Jun 30, 2025

Core: Support DV for partition stats #13425

Merged

Spec: Update partition stats for V3 #12098

Spec: Update partition stats for V3 #12098

Uh oh!

Conversation

aokolnychyi commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aokolnychyi Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzwu Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajantha-bhat Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Mar 7, 2025

Uh oh!

github-actions bot commented Apr 7, 2025

Uh oh!

github-actions bot commented Apr 14, 2025

Uh oh!

aokolnychyi commented Apr 29, 2025

Uh oh!

ajantha-bhat commented Apr 30, 2025

aokolnychyi commented Jan 24, 2025 •

edited

Loading

ajantha-bhat Jan 25, 2025 •

edited

Loading

aokolnychyi Feb 5, 2025 •

edited

Loading

aokolnychyi Feb 11, 2025 •

edited

Loading

stevenzwu Feb 12, 2025 •

edited

Loading

aokolnychyi commented Jan 24, 2025 •

edited

Loading

ajantha-bhat Jan 25, 2025 •

edited

Loading

amogh-jahagirdar Jan 29, 2025 •

edited

Loading