dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2294839225
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -1384,4 +1384,26 @@ private void
removeBroadcastValuesFromLocalBlockManager(long id) {
blockInfoManager.removeBlock(blockId);
blockManager.memoryStore().remove(blockId);
}
+
+ @Test
+ public void testRewritePathWithCreateFileListFalse() throws Exception {
Review Comment:
nit: how about `testRewritePathWithoutCreateFileList`
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +125,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** Number of delete files whose paths were updated. Default value is 0. */
+ default int deleteFilesPathRewrittenCount() {
+ return 0;
+ }
+
+ /** Number of manifest files whose paths were updated. Default value is 0.
*/
+ default int manifestFilesPathRewrittenCount() {
+ return 0;
+ }
Review Comment:
```suggestion
/** Number of delete files with rewritten paths. */
default int deleteFilesPathRewrittenCount() {
return 0;
}
/** Number of manifest files with rewritten paths. */
default int manifestFilesPathRewrittenCount() {
return 0;
}
```
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .deleteFilesPathRewrittenCount(deleteFiles.size())
+ .manifestFilesPathRewrittenCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // file list generation disabled
+ if (!fileListEnabled) {
+ return builder.fileListLocation("N/A").build();
Review Comment:
nit: can we pull this up as a package local variable to represent not
applicable and use it to assert in
`spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java`
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -96,7 +96,7 @@ public class RewriteTablePathSparkAction extends
BaseSparkAction<RewriteTablePat
private String startVersionName;
private String endVersionName;
private String stagingDir;
- private boolean skipFileList;
+ private boolean fileListEnabled = true;
Review Comment:
can we call it `createFileList` so we can have consist naming in interface,
action and procedure?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
