Spark: Ensure that partition stats files are considered for GC procedures by ajantha-bhat · Pull Request #9284 · apache/iceberg

ajantha-bhat · 2023-12-12T11:35:22Z

Expire snapshots and remove orphan files action/procedure should also consider partition stats files.

ajantha-bhat · 2023-12-12T11:36:43Z

 * @return the location of statistics files
+ * @deprecated use the {@code statisticsFilesLocationsForSnapshots(table, snapshotIds)} instead.
 */
+ @Deprecated


can't use the Predicate<StatisticsFile> for partition stats. Hence, deprecated and introduced snapshot id based input like other methods in this class (manifest list, manifest etc)

ajantha-bhat · 2023-12-12T11:38:09Z

 Assertions.assertThat(output.get(0)[5]).as("should be 1 deleted statistics file").isEqualTo(1L);

 table.refresh();
- List<StatisticsFile> statsWithSnapshotId1 =


Since we are checking containsExactly at line 482, no need of these checks.

…ocedures

aokolnychyi · 2023-12-20T09:41:50Z

 * @param table table for which statistics files needs to be listed
 * @param predicate predicate for filtering the statistics files
 * @return the location of statistics files
+ * @deprecated use the {@code statisticsFilesLocationsForSnapshots(table, snapshotIds)} instead.


Shall we add a note on when it was deprecated (1.5.0) and when it will be removed (1.6.0)?

I have added this already in previous commit.

aokolnychyi · 2023-12-20T09:44:10Z

+ * statistics files for all the snapshots will be returned.
+ * @return the location of statistics files
+ */
+ public static List<String> statisticsFilesLocationsForSnapshots(


What do you think about calling it statisticsFilesLocations to match manifestListLocations that also accepts a set of snapshot IDs?

The deprecated method already has a name statisticsFilesLocations. So, If I keep the same name, compiler says ambiguous error when I call statisticsFilesLocationsForSnapshots(table, null)
Hence, I had to keep different name.

aokolnychyi · 2023-12-20T09:47:47Z

@ajantha-bhat, this looks mostly good to me. Can we add the tests to all Spark versions to be safe?

ajantha-bhat · 2023-12-20T09:55:34Z

@ajantha-bhat, this looks mostly good to me. Can we add the tests to all Spark versions to be safe?

Sure. I didn't initially add to other spark version thinking if I get any comments for test, I have to rework in all the spark version. So, I thought of adding tests in a single follow up PR for other spark versions.

Now that we don't have any specific comment on test, I can add it in this PR itself.

ajantha-bhat · 2023-12-20T11:40:00Z

@aokolnychyi: Thanks for the review.
Addressed all the comments except #9284 (comment) (with a reasoning).

Build passed and PR is ready.

ajantha-bhat · 2024-01-03T05:31:15Z

ping @aokolnychyi

aokolnychyi

It looks good to me, I had one optional comment. Let me know what you want to do there, @ajantha-bhat.

ajantha-bhat · 2024-01-17T01:59:42Z

It looks good to me, I had one optional comment. Let me know what you want to do there, @ajantha-bhat.

@aokolnychyi : Are you referring to #9284 (comment) comment? I have replied why I can't change the name due to compile time ambiguity of same signature.
Other than that I don't see any more open comment. So, please let me know.

aokolnychyi · 2024-01-18T03:41:04Z

Oops, I probably forgot to post the comment. I was thinking about statisticsFileLocations to match the naming of other methods like metadataFileLocations (Files -> File). It is optional, though.

ajantha-bhat · 2024-01-18T06:11:46Z

Oops, I probably forgot to post the comment. I was thinking about statisticsFileLocations to match the naming of other methods like metadataFileLocations (Files -> File). It is optional, though.

statisticsFileLocations is an existing public method (before this PR). So, we can't rename it directly. We have to deprecate and introduce new one. I think we can leave it as it as.

The new method statisticsFilesLocationsForSnapshots can be renamed but it will become inconsistent with existing statisticsFileLocations.

aokolnychyi · 2024-01-18T19:10:14Z

Yep, that's fair. Let's keep it as is then.

aokolnychyi · 2024-01-18T19:11:00Z

Thanks, @ajantha-bhat!

…pache#9284)

…GC procedures (apache#9284)

github-actionsBot added API spark core labels Dec 12, 2023

ajantha-bhat requested a review from aokolnychyi December 12, 2023 11:38

ajantha-bhat mentioned this pull request Dec 12, 2023
Api: Track partition statistics via TableMetadata #8502
Merged

ajantha-bhat commented Dec 12, 2023
View reviewed changes

ajantha-bhat changed the title ~~Spark-3.5: Ensure that partition stats files are considered for GC procedures~~Spark 3.5: Ensure that partition stats files are considered for GC proceduresDec 12, 2023

Spark-3.5: Ensure that partition stats files are considered for GC pr…
d3e75e0
…ocedures

ajantha-bhat force-pushed the p_track_spark branch from bf5a896 to d3e75e0Compare December 19, 2023 01:12

github-actionsBot removed the API label Dec 19, 2023

aokolnychyi reviewed Dec 20, 2023
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/ReachableFileUtil.java

aokolnychyi reviewed Dec 20, 2023
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/ReachableFileUtil.java

ajantha-bhat added 2 commits December 20, 2023 15:37

Address comemnts
81e4efd

Add tests with other Spark versions
6e1c75d

ajantha-bhat changed the title ~~Spark 3.5: Ensure that partition stats files are considered for GC procedures~~Spark: Ensure that partition stats files are considered for GC proceduresDec 20, 2023

aokolnychyi approved these changes Jan 16, 2024
View reviewed changes

aokolnychyi merged commit 02836ea into apache:mainJan 18, 2024

geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
Spark: Ensure partition stats files are considered for GC procedures (a…
6cb1d40
…pache#9284)

adnanhemani pushed a commit to adnanhemani/iceberg that referenced this pull request Jan 30, 2024
Spark: Ensure partition stats files are considered for GC procedures (a…
bb2e8aa
…pache#9284)

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Spark: Ensure partition stats files are considered for GC procedures (a…
e1c8d8b
…pache#9284)

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025
[Cherry-pick] Spark: Ensure partition stats files are considered for …
bc4e0d1
…GC procedures (apache#9284)

ajantha-bhat mentioned this pull request May 8, 2025
Core: Support incremental compute for partition stats #12629
Merged

Uh oh!

Conversation

ajantha-bhat commented Dec 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajantha-bhatDec 12, 2023

Choose a reason for hiding this comment

Uh oh!

ajantha-bhatDec 12, 2023

Choose a reason for hiding this comment

Uh oh!

aokolnychyiDec 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajantha-bhatJan 17, 2024

Choose a reason for hiding this comment

Uh oh!

aokolnychyiDec 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajantha-bhatDec 20, 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!

Uh oh!

Uh oh!

aokolnychyi commented Dec 20, 2023

Uh oh!

ajantha-bhat commented Dec 20, 2023

Uh oh!

ajantha-bhat commented Dec 20, 2023

Uh oh!

ajantha-bhat commented Jan 3, 2024

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Jan 17, 2024

Uh oh!

aokolnychyi commented Jan 18, 2024

Uh oh!

ajantha-bhat commented Jan 18, 2024

Uh oh!

aokolnychyi commented Jan 18, 2024

Uh oh!

aokolnychyi commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ajantha-bhat commented Dec 12, 2023•
edited
Loading

ajantha-bhatDec 20, 2023•
edited
Loading