add_docs_and_backport_max_files_rewrite_option #13082

coderfender · 2025-05-16T20:40:30Z

Backport changes to flink 1.19 , 2.0 and spark 3.4, 4.0 and update spark docs to provide more info about this option
Original PR : #12824

coderfender · 2025-05-17T00:40:15Z

@pvary , @RussellSpitzer . Please take a look whenever you get a chance

pvary · 2025-05-19T08:30:02Z

docs/docs/spark-procedures.md

-| `delete-ratio-threshold` | 0.3 | Minimum deletion ratio that needs to be associated with a data file for it to be considered for rewriting |
-| `output-spec-id` | current partition spec id | Identifier of the output partition spec. Data will be reorganized during the rewrite to align with the output partitioning. |
-| `remove-dangling-deletes` | false | Remove dangling position and equality deletes after rewriting. A delete file is considered dangling if it does not apply to any live data files. Enabling this will generate an additional commit for the removal. |
+| Name | Default Value                                                                                                                 | Description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |


I think most of the changes is just formatting. Could we revert the formatting only changes?

I did try to revert the documentation changes but for some reason git picks up changes which it isnt supposed to

pvary · 2025-05-19T08:34:40Z

@coderfender: When backporting a PR, it is usually helpful if the contributor helps the reviewer by mentioning any specific changes which are specifically needed above the original ones, or say that this is a clean backport.

Am I right, that this is a clean backport, and only the documentation changes are specific?

Thanks for the backport,
Peter

pvary · 2025-05-19T20:23:59Z

docs/docs/spark-procedures.md

Unnecessary newline change?

Yeah seems like my IDE is adding these

@pvary . Retried the same approach through online git file edit. from UI as well. It seems like adding the new option is automatically showing line 1059 as a change.

Happy to take guidance to fix this

~~There is an extra " " at the end of this line that you are removing.~~

~~"for path rewrite " vs "for path rewrite"~~

Nvm, seems like it vanished? You could always reset the file and apply the other changes to see if it persists

Double never mind, there it is

@RussellSpitzer I tried editing the line from Github repo , sublime etc but for some reason my commits wouldnt work :|

coderfender · 2025-05-19T20:41:21Z

flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/RewriteDataFiles.java

+     * for more details.
+     *
+     * @param maxFilesToRewrite maximum files to rewrite
+     */
    public Builder maxFilesToRewrite(int maxFilesToRewrite) {


@pvary . The only change unrelated to backporting and docs is the method signature JAVA docs I added for flink API

docs/docs/spark-procedures.md

Co-authored-by: Russell Spitzer <[email protected]>

RussellSpitzer

I think we should keep the endline fix as well

RussellSpitzer · 2025-05-19T20:53:24Z

I'll let @pvary comment if he has any final comments, otherwise I'll merge tomorrow

fix whitespace

coderfender · 2025-05-19T21:00:52Z

@RussellSpitzer , @pvary the endline fix is unfortunately not recognized by Git (but it would let me commit after removing the whitespace though) . Ref : b87bbfc , c1939bb

pvary · 2025-05-20T12:44:07Z

Merged to main.
Thanks for the backport @coderfender and @RussellSpitzer for the review!
Also thanks for the fight with the whitespaces 😄

@coderfender, @RussellSpitzer: Do you think that the maxRewriteBytes configuration option would be useful for Spark as well?

iceberg/flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/RewriteDataFiles.java

Lines 79 to 85 in 09a5317

    
               /** 
        
                * Configures the maximum byte size of the rewrites for one scheduled compaction. This could be 
        
                * used to limit the resources used by the compaction. 
        
                * 
        
                * @param newMaxRewriteBytes to limit the size of the rewrites 
        
                */ 
        
               public Builder maxRewriteBytes(long newMaxRewriteBytes) {

If you find the feature would be useful in Spark as well, then we could move the filtering from Flink to the core. @coderfender : If you have time, I would be happy to review it, or we could find someone else who has more time to do this.

coderfender · 2025-05-20T16:15:38Z

@pvary , I am not sure if I understand this right ? I initially wrote it as part of SparkRewriteActions but after your refactor is merged, I had to move it upstream to the core planner logic. Are you saying I should add this option similar to flink maintenance API and perhaps add some more tests ? Please let me know if I am missing something here .

Thank you for merging in to main @pvary and thank you for the review / approval @RussellSpitzer

pvary · 2025-05-20T16:24:09Z

There is a feature in Flink compaction, where the user can limit the number of bytes to rewrite in a compaction (maxRewriteBytes). The question is: Would this be interesting for Spark as well? Then we should move the feature to the core so both Flink and Spark could use it.

coderfender · 2025-05-20T16:28:27Z

Ah now I get it . I was confused between maxRewriteBytes vs maxFilesToRewrite :) . I would love to get that feature ported to core from Flink so that we can use in both Spark and Flink engines . Let me create an issue and assign it to myself . Thank you for the suggestion @pvary

backport_max_files_rewrite_option

6e79798

github-actions bot added spark flink labels May 16, 2025

coderfender added 2 commits May 16, 2025 15:01

backport_max_files_rewrite_option

118f8d4

backport_max_files_rewrite_option

a9b24aa

coderfender changed the title ~~backport_max_files_rewrite_option~~ add_docs_and_backport_max_files_rewrite_option May 16, 2025

github-actions bot added the docs label May 16, 2025

coderfender marked this pull request as ready for review May 16, 2025 23:31

pvary reviewed May 19, 2025

View reviewed changes

address_review_comments

e2223b7

coderfender force-pushed the add_rewrite_data_files_option_backport branch from 95feed1 to e2223b7 Compare May 19, 2025 17:45

add_docs_to_run_single_test

2512dfc

pvary reviewed May 19, 2025

View reviewed changes

fix_whitespace_formatting

aa8936b

coderfender force-pushed the add_rewrite_data_files_option_backport branch from d6b12cc to aa8936b Compare May 19, 2025 20:37

Update spark-procedures.md

c9e7248

coderfender commented May 19, 2025

View reviewed changes

RussellSpitzer reviewed May 19, 2025

View reviewed changes

docs/docs/spark-procedures.md Outdated Show resolved Hide resolved

Update docs/docs/spark-procedures.md

37b307a

Co-authored-by: Russell Spitzer <[email protected]>

RussellSpitzer approved these changes May 19, 2025

View reviewed changes

coderfender added 2 commits May 19, 2025 13:56

Update spark-procedures.md

b87bbfc

Update spark-procedures.md

c1939bb

fix whitespace

pvary approved these changes May 20, 2025

View reviewed changes

pvary merged commit 09a5317 into apache:main May 20, 2025
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add_docs_and_backport_max_files_rewrite_option #13082

add_docs_and_backport_max_files_rewrite_option #13082

coderfender commented May 16, 2025 •

edited

Loading

coderfender commented May 17, 2025

pvary May 19, 2025

coderfender May 19, 2025

pvary commented May 19, 2025

pvary May 19, 2025

coderfender May 19, 2025

coderfender May 19, 2025 •

edited

Loading

coderfender May 19, 2025

RussellSpitzer May 19, 2025 •

edited

Loading

RussellSpitzer May 19, 2025

coderfender May 19, 2025

coderfender May 19, 2025

RussellSpitzer left a comment

RussellSpitzer commented May 19, 2025

coderfender commented May 19, 2025 •

edited

Loading

pvary commented May 20, 2025

coderfender commented May 20, 2025

pvary commented May 20, 2025

coderfender commented May 20, 2025 •

edited

Loading

add_docs_and_backport_max_files_rewrite_option #13082

add_docs_and_backport_max_files_rewrite_option #13082

Conversation

coderfender commented May 16, 2025 • edited Loading

coderfender commented May 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented May 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderfender May 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer May 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

RussellSpitzer commented May 19, 2025

coderfender commented May 19, 2025 • edited Loading

pvary commented May 20, 2025

coderfender commented May 20, 2025

pvary commented May 20, 2025

coderfender commented May 20, 2025 • edited Loading

coderfender commented May 16, 2025 •

edited

Loading

coderfender May 19, 2025 •

edited

Loading

RussellSpitzer May 19, 2025 •

edited

Loading

coderfender commented May 19, 2025 •

edited

Loading

coderfender commented May 20, 2025 •

edited

Loading