You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "slfan1989 (via GitHub)" <gi...@apache.org> on 2025/04/18 23:48:29 UTC
[PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
slfan1989 opened a new pull request, #12844:
URL: https://github.com/apache/iceberg/pull/12844
This is a minor feature improvement. The background is that we are using `RewriteTablePathProcedure` to convert Hive tables to Iceberg tables, as detailed in #12762. `RewriteTablePathProcedure` generates a `file-list` file, and I need to manually clean up this file after each conversion. I understand that the `file-list` is mainly used to check data integrity, but since it is not essential for metadata, I believe allowing users to decide whether to generate this file would offer greater flexibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2823233934
> @manuzhang Thank you for your suggestion! but I would like to confirm your thoughts. I have previously looked at the content of the copy plan, which displays many records. When you refer to printing the copy plan, do you mean outputting it to the console, or should it be outputted to the file-list? From my understanding, outputting it to the file-list seems more reasonable.
Yea I was also a bit confused what you mean 'print' the plan? It can be a big plan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2822741670
@manuzhang just curious, why print the copy plan, if the user doesnt want it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2821332526
I'd suggest adding an option to print copy plan rather than not returning info. Also, it will be easier to iterate on the idea if you target Spark 3.5 first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2816544324
Interesting, is it all that you need to do Hive -> Iceberg conversion. Seems simple and make sense to me. cc @flyrain @dramaticlly for any thoughts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2053192186
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,18 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Allows the user to skip saving the file list, determining whether certain files should be
+ * skipped from being saved.
+ *
+ * @param skipFileList A boolean value indicating whether to skip file saving. Pass true to skip,
+ * false to not skip.
+ * @return this for method chaining
+ */
Review Comment:
Thank you for your suggestion! I have updated this part of the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2899943230
@manuzhang @szehon-ho @dramaticlly The Spark 4.0 module has now been successfully merged. Can we restart this PR? Also, do we need to extract the API definitions in RewriteTablePath.java into a separate pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2138139230
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int deleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
+ default int metadataFilesCount() {
+ return 0;
Review Comment:
Can you share a bit more on what are those 2 results are used for? I think add some unit tests with non-zero results would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 closed pull request #12844: Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation
URL: https://github.com/apache/iceberg/pull/12844
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2986321548
@manuzhang @dramaticlly @szehon-ho I've completed the rebase on the code. If you have some time, Could you please review this PR and provide any feedback? Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2819858442
> > Interesting, is it all that you need to do Hive -> Iceberg conversion. Seems simple and make sense to me. cc @flyrain @dramaticlly for any thoughts
>
> Glad to hear RewriteTablePath can be used this way and I am ok to add a flag to control the behavior of saving file list location. I think currently it's tied to staging location and changing staging location will also influence where the metadata files will be saved on disk.
>
> Also nit, I think your change include both Spark 3.4 and Spark 3.5, so you might want to reflect that in the PR title.
@szehon-ho @dramaticlly Thank you very much for your messages and for providing the RewriteTablePathProcedure, which makes Hive2Iceberg much simpler. I will continue improving #12762, and once this PR is ready, I will ask you to review the code. Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2819025708
> Interesting, is it all that you need to do Hive -> Iceberg conversion. Seems simple and make sense to me. cc @flyrain @dramaticlly for any thoughts
Glad to hear RewriteTablePath can be used this way and I am ok to add a flag to control the behavior of saving file list location. I think currently it's tied to staging location and changing staging location will also influence where the metadata files will be saved on disk.
Also nit, I think your change include both Spark 3.4 and Spark 3.5, so you might want to reflect that in the PR title.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2052716230
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,18 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Allows the user to skip saving the file list, determining whether certain files should be
+ * skipped from being saved.
+ *
+ * @param skipFileList A boolean value indicating whether to skip file saving. Pass true to skip,
+ * false to not skip.
+ * @return this for method chaining
+ */
Review Comment:
I feel we can be more concise about it since this is a boolean flag, how about?
```
/**
* Whether to skip saving the file list location.
*
* @param skipFileList true to skip saving the file list, false to include it
* @return this instance for method chaining
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2822971609
> @slfan1989 I'd suggest adding an option to print copy plan rather than not returning any info.
@manuzhang Thank you for your suggestion! but I would like to confirm your thoughts. I have previously looked at the content of the copy plan, which displays many records. When you refer to printing the copy plan, do you mean outputting it to the console, or should it be outputted to the file-list? From my understanding, outputting it to the file-list seems more reasonable.
> Also, it will be easier to iterate on the idea if you target Spark 3.5 first.
We can indeed focus on the changes in Spark 3.5 first, and once the modifications for Spark 3.5 are complete, we can backport them to Spark 3.4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2864981462
@szehon-ho @manuzhang @dramaticlly Could you please review this PR again? Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2055177800
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -45,6 +45,8 @@ public class RewriteTablePathProcedure extends BaseProcedure {
ProcedureParameter.optional("end_version", DataTypes.StringType);
private static final ProcedureParameter STAGING_LOCATION_PARAM =
ProcedureParameter.optional("staging_location", DataTypes.StringType);
+ private static final ProcedureParameter SKIP_FILE_LIST_PARAM =
+ ProcedureParameter.optional("skip_file_list", DataTypes.BooleanType);
Review Comment:
I prefer an option like `save_file_list` which defaults to true. `skip_file_list` doesn't reflect that it's skipping saving file list to a file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2136766160
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath skipFileList(boolean skipFileList) {
Review Comment:
@szehon-ho @dramaticlly @manuzhang Could you please spare some time to review this PR? Thanks a lot! Apologies for the delayed response. Recently, I've been working on migrating Hive tables to Iceberg tables, and following the approach mentioned in #12762, I've migrated over 500 tables in the past two months. During this process, I've encountered some issues that I'd like to discuss with you. I’m currently summarizing some information from the migration process and plan to update #12762 within 1-2 days. Looking forward to your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2816562105
> Interesting, is it all that you need to do Hive -> Iceberg conversion. Seems simple and make sense to me. cc @flyrain @dramaticlly for any thoughts
@szehon-ho Thank you for your reply! The sample code(#12769) I provided is consistent with the code in our production environment. We have successfully used this method to batch convert over 80 Hive tables to Iceberg tables. The migrated tables are expected, with both read and write tasks normally. This feature relies on the community's existing Snapshot and RewriteTablePath functions, which offer good stability. Additionally, by adjusting some Spark parameters, it enables the fast migration of large tables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2822959214
@manuzhang @szehon-ho @dramaticlly Thank you very much for reviewing this PR. I will continue to improve it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2051418050
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -303,6 +310,11 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(endMetadata, deleteFiles);
+ // skip file list
+ if (skipFileList) {
+ return "skip-file-list";
Review Comment:
I believe using null is fine, but during unit testing, I found that I cannot use null directly due to a non-null validation. Therefore, I considered using an empty string or the string 'null' as alternatives. In this version, I opted for the empty string as a replacement. I'm wondering if this approach is appropriate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2051407254
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Allows the user to skip saving the file list, determining whether certain files should be
+ * skipped from being saved.
+ *
+ * @param skipFileList A boolean value indicating whether to skip file saving. Pass true to skip,
+ * false to not skip.
+ * @return this for method chaining
+ */
+ RewriteTablePath skipFileList(boolean skipFileList);
Review Comment:
we should make a default impl to avoid breaking change
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -303,6 +310,11 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(endMetadata, deleteFiles);
+ // skip file list
+ if (skipFileList) {
+ return "skip-file-list";
Review Comment:
just thinking out loud, would 'null' be better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-3092649833
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 dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2124219965
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath skipFileList(boolean skipFileList) {
Review Comment:
@manuzhangThank you for your suggestion! I will optimize the code based on your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2112148631
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath skipFileList(boolean skipFileList) {
Review Comment:
I still prefer `saveFileList` or `generateFileList`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2865004582
@slfan1989 FYI, Spark 4.0 integration [will be redone](https://github.com/apache/iceberg/pull/13006). Anyway, could you please create a separate PR for API change only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark3.5: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2051416856
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Allows the user to skip saving the file list, determining whether certain files should be
+ * skipped from being saved.
+ *
+ * @param skipFileList A boolean value indicating whether to skip file saving. Pass true to skip,
+ * false to not skip.
+ * @return this for method chaining
+ */
+ RewriteTablePath skipFileList(boolean skipFileList);
Review Comment:
Thank you for your suggestion! I have added a default implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2824212686
@slfan1989 @szehon-ho I meant outputting to console if user doesn't want to save to file. If that's not possible when the plan is big, maybe adding two more output values, "rewrite_delete_files_count" and "rewrite_metadata_files_count".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2825908909
> @slfan1989 @szehon-ho I meant outputting to console if user doesn't want to save to file. If that's not possible when the plan is big, maybe add two more output values, "rewrite_delete_files_count" and "rewrite_metadata_files_count". Then I think `file_list` file can be optional.
@manuzhang @szehon-ho Thank you for your suggestions! I will make improvements to the code as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2822818505
@szehon-ho Otherwise, we don't know whether this procedure has run as expected and the intention of this PR is not to worry about cleaning the copy plan file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-3120680613
Thank you all for your attention. This piece of code is a bit outdated, so I’ll reorganize it and submit a new version soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2825286985
Makes sense, i think adding a count sounds fine to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#discussion_r2138972580
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int deleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
+ default int metadataFilesCount() {
+ return 0;
Review Comment:
> @slfan1989 @szehon-ho I meant outputting to console if user doesn't want to save to file. If that's not possible when the plan is big, maybe add two more output values, "rewrite_delete_files_count" and "rewrite_metadata_files_count". Then I think `file_list` file can be optional.
@dramaticlly Thank you very much for reviewing the code! These changes were made based on discussions with @manuzhang.
Do you have any suggestions for the default values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #12844:
URL: https://github.com/apache/iceberg/pull/12844#issuecomment-2865095776
> @slfan1989 FYI, Spark 4.0 integration [will be redone](https://github.com/apache/iceberg/pull/13006). Anyway, could you please create a separate PR for API change only?
@manuzhang Thank you for your message! I will continue to follow up on this PR once the Spark 4.0 integration process is completed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org