huaxingao commented on code in PR #15975:
URL: https://github.com/apache/iceberg/pull/15975#discussion_r3132878854
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -367,7 +369,8 @@ public static RewriteResult<DataFile> rewriteDataManifest(
* Rewrite a delete manifest, replacing path references.
*
* @param manifestFile source delete manifest to rewrite
- * @param snapshotIds snapshot ids for filtering returned delete manifest
entries
+ * @param snapshotIds snapshot ids for filtering returned delete manifest
entries. When null, all
+ * live entries are included in the copy plan regardless of their
snapshot id.
Review Comment:
Yes, I used null to avoid adding a new public method to the utility class.
The Javadoc documents the null behavior: when null, all live entries are
included for full copies. I can add an overloaded method without the
snapshotIds parameter if you think that's cleaner.
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -429,10 +432,7 @@ private static RewriteResult<DataFile> writeDataFileEntry(
DataFile newDataFile =
DataFiles.builder(spec).copy(entry.file()).withPath(targetDataFilePath).build();
appendEntryWithFile(entry, writer, newDataFile);
- // keep the following entries in metadata but exclude them from copyPlan
- // 1) deleted data files
- // 2) entries not changed by snapshotIds
Review Comment:
You're right, the comment is still valid for incremental copies. Added it
back with a note that the snapshotIds only applies for incremental copies.
--
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]
