Spark-3.3: Handle statistics file clean up from expireSnapshots action/procedure #6091

ajantha-bhat · 2022-11-01T07:30:36Z

This PR handles the spark action/procedure side of the statistics file clean-up for expire_snapshots.
It is a follow-up based on the core functionality of #6090

ajantha-bhat · 2022-11-01T07:31:12Z

cc: @findepi, @rdblue, @szehon-ho

core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java

ajantha-bhat · 2023-02-23T06:51:01Z

Now that the core functionality PR (#6090) is merged, I think this PR can be reviewed and merged now.

cc: @aokolnychyi, @szehon-ho, @rdblue, @jackye1995

ajantha-bhat · 2023-01-20T06:01:14Z

...tensions/src/test/java/org/apache/iceberg/spark/extensions/TestExpireSnapshotsProcedure.java


    List<Object[]> output = sql("CALL %s.system.expire_snapshots('%s')", catalogName, tableIdent);
-    assertEquals("Should not delete any files", ImmutableList.of(row(0L, 0L, 0L, 0L, 0L)), output);
+    assertEquals(


added one more entry for deletedStatisticsFilesCount for all the existing cases.

jackye1995 · 2023-02-23T16:45:51Z

I added the core PR to 1.2 release milestone, not sure if we can get this in in time, but I can add it first so it gets more tractions.

jackye1995 · 2023-02-28T20:54:45Z

core/src/main/java/org/apache/iceberg/ReachableFileUtil.java

+   *     returns location of all the statistics files in a table.
+   * @return the location of statistics files
+   */
+  public static List<String> statisticsFilesLocations(Table table, Set<Long> snapshotIds) {


can we simplify the logic of the original method using stream()? Also for this method, I think we can provide a more generic util method that takes in a filter, instead of just adding a specific method to filter by snapshot ID set

Updated now.
Originally I wanted to keep the same style as manifestListLocations and others. So, I didn't used modified.

jackye1995

this overall looks good to me, just a nit comment

core/src/main/java/org/apache/iceberg/ReachableFileUtil.java

…n/procedure

ajantha-bhat · 2023-03-09T18:14:25Z

finally figured out the testcase failure 😄 . It was not failing locally as I didn't rebase it. After rebase it failed locally and the reason is newly merged #6682 was missing a case for stats files.

core/src/main/java/org/apache/iceberg/actions/BaseExpireSnapshotsActionResult.java

ajantha-bhat · 2023-03-09T18:17:07Z

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/BaseSparkAction.java

      } else if (MANIFEST_LIST.equalsIgnoreCase(type)) {
        manifestListsCount.addAndGet(numFiles);

+      } else if (STATISTICS_FILES.equalsIgnoreCase(type)) {


newly added #6682 was missing this case. Hence, test was failing after rebasing.

ajantha-bhat · 2023-03-09T19:16:37Z

@rdblue, @jackye1995 : PR is ready

aokolnychyi · 2023-03-10T05:01:27Z

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/ExpireSnapshotsSparkAction.java

-        summary.equalityDeleteFilesCount(),
-        summary.manifestsCount(),
-        summary.manifestListsCount());
+    return ImmutableResult.builder()


I am not familiar with this lib but won't the name conflict if we migrate more results to auto-generated beans as it does not take the outer class name into account?

import org.apache.iceberg.actions.ImmutableResult;

We call it Result in all actions and use outer class for identification.

good catch. I will check if it is possible to alter the generated class name or include the outer class name.
If I don't find anything in a few hours, I will just use a new private Inner class in ExpireSnapshotsSparkAction.java.

Later It can be easily replaced (as it is private) with Immutables once we have better ways in a follow-up PR.
I don't want to block this PR (targeted for 1.2.0).

Used @Value.Enclosing So that builder looks like ImmutableExpireSnapshots.Result.builder() now.

In a follow-up PR, I plan to deprecate all public result classes of actions and use immutables annotation for actions to build result objects from that.

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/ExpireSnapshotsSparkAction.java

aokolnychyi · 2023-03-10T05:08:40Z

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/BaseSparkAction.java

+    }
+
+    return toFileInfoDS(
+        ReachableFileUtil.statisticsFilesLocations(table, predicate), STATISTICS_FILES);


nit: Can we define an extra var with a relatively short name like statisticsFiles or locations so that this would fit on one line?

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/BaseSparkAction.java

aokolnychyi · 2023-03-10T19:04:53Z

Thanks, @ajantha-bhat! Thanks for reviewing, @findepi @rdblue @jackye1995 @nastra!

ajantha-bhat · 2023-03-11T05:14:56Z

Thanks @aokolnychyi for merging this.

I have backported it to spark:3.2: Spark-3.2: Handle statistics file clean up from expireSnapshots action/procedure #7074 and build passes for it.
Spark3.1 backport needs some effort as spark 3.1 doesn't have Spark: Improve performance of expire snapshot by not double-scanning retained Snapshots #3457 and Bulk delete #6682 (not sure why it was not backported)
same problems for spark2.4 too.

This change backports PR #6091 to Spark 3.2.

aokolnychyi · 2023-03-11T05:34:28Z

I merged the Spark 3.2 PR, thanks. We stopped active development on 2.4 and 3.1. We also consider dropping 3.1 soon. In my view, it is OK not to cherrypick this change there. Up to you, though.

)

) This change backports PR apache#6091 to Spark 3.2.

…hots (apache#6091)

ajantha-bhat marked this pull request as draft November 1, 2022 07:30

github-actions bot added API core spark labels Nov 1, 2022

ajantha-bhat force-pushed the stats_expire_all branch 3 times, most recently from b557b25 to 7743b5c Compare November 1, 2022 18:05

ajantha-bhat marked this pull request as ready for review November 2, 2022 01:59

findepi reviewed Nov 4, 2022

View reviewed changes

core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java Outdated Show resolved Hide resolved

ajantha-bhat mentioned this pull request Dec 3, 2022

Docs: Update spec about statistics file snapshot id #6267

Merged

ajantha-bhat force-pushed the stats_expire_all branch 2 times, most recently from bc5575b to 69c1b96 Compare December 9, 2022 07:23

ajantha-bhat force-pushed the stats_expire_all branch from 69c1b96 to 7a5be5d Compare December 13, 2022 09:35

ajantha-bhat marked this pull request as draft January 18, 2023 17:11

ajantha-bhat force-pushed the stats_expire_all branch from 7a5be5d to d3b5fa1 Compare January 20, 2023 12:08

ajantha-bhat marked this pull request as ready for review January 20, 2023 15:00

ajantha-bhat mentioned this pull request Feb 5, 2023

Core: Handle statistics file clean up from expireSnapshots #6090

Merged

ajantha-bhat force-pushed the stats_expire_all branch from d3b5fa1 to a9253d8 Compare February 23, 2023 05:50

ajantha-bhat commented Feb 23, 2023

View reviewed changes

jackye1995 added this to the Iceberg 1.2.0 milestone Feb 23, 2023

jackye1995 self-requested a review February 23, 2023 16:45

jackye1995 reviewed Feb 28, 2023

View reviewed changes

ajantha-bhat force-pushed the stats_expire_all branch from 22611ea to a7413fe Compare March 1, 2023 07:16

jackye1995 reviewed Mar 1, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/ReachableFileUtil.java Outdated Show resolved Hide resolved

ajantha-bhat force-pushed the stats_expire_all branch from a7413fe to 96081b3 Compare March 1, 2023 07:21

jackye1995 reviewed Mar 1, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/ReachableFileUtil.java Show resolved Hide resolved

ajantha-bhat force-pushed the stats_expire_all branch from f81301d to 1030155 Compare March 9, 2023 16:41

ajantha-bhat added 3 commits March 9, 2023 22:17

Spark-3.3: Handle statistics file clean up from expireSnapshots actio…

be219cd

…n/procedure

Address suggestions

4bf16d1

Address review comments

83bc875

ajantha-bhat force-pushed the stats_expire_all branch from 1030155 to 6421f45 Compare March 9, 2023 16:49

ajantha-bhat marked this pull request as draft March 9, 2023 18:00

ajantha-bhat force-pushed the stats_expire_all branch from 6421f45 to c4fba5f Compare March 9, 2023 18:12

ajantha-bhat marked this pull request as ready for review March 9, 2023 18:12

nastra reviewed Mar 9, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/actions/BaseExpireSnapshotsActionResult.java Show resolved Hide resolved

ajantha-bhat commented Mar 9, 2023

View reviewed changes

Address comments

78b495a

ajantha-bhat force-pushed the stats_expire_all branch from c4fba5f to 78b495a Compare March 9, 2023 18:25

aokolnychyi reviewed Mar 10, 2023

View reviewed changes

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/ExpireSnapshotsSparkAction.java Show resolved Hide resolved

aokolnychyi reviewed Mar 10, 2023

View reviewed changes

spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/BaseSparkAction.java Show resolved Hide resolved

Address new comments

347a554

aokolnychyi approved these changes Mar 10, 2023

View reviewed changes

aokolnychyi merged commit 5921421 into apache:master Mar 10, 2023

ajantha-bhat mentioned this pull request Mar 11, 2023

Spark-3.2: Handle statistics file clean up from expireSnapshots action/procedure #7074

Merged

aokolnychyi pushed a commit that referenced this pull request Mar 11, 2023

Spark 3.2: Handle statistics files while expiring snapshots (#7074)

c6f1b37

This change backports PR #6091 to Spark 3.2.

ajantha-bhat mentioned this pull request Mar 12, 2023

Core: Refactor actions results #7089

Merged

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Spark-3.3: Handle statistics files while expiring snapshots (apache#6091

6da76b9

)

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Spark 3.2: Handle statistics files while expiring snapshots (apache#7074

e129d0d

) This change backports PR apache#6091 to Spark 3.2.

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

[Cherry-pick] Spark-3.3: Handle statistics files while expiring snaps…

bea4f4f

…hots (apache#6091)

Spark-3.3: Handle statistics file clean up from expireSnapshots action/procedure #6091

Spark-3.3: Handle statistics file clean up from expireSnapshots action/procedure #6091

Uh oh!

Conversation

ajantha-bhat commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajantha-bhat commented Nov 1, 2022

Uh oh!

Uh oh!

ajantha-bhat commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajantha-bhat commented Mar 9, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Mar 9, 2023

Uh oh!

aokolnychyi Mar 10, 2023 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aokolnychyi commented Mar 10, 2023

Uh oh!

ajantha-bhat commented Mar 11, 2023

Uh oh!

aokolnychyi commented Mar 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ajantha-bhat commented Nov 1, 2022 •

edited

Loading

aokolnychyi Mar 10, 2023 •

edited

Loading