97harsh commented on code in PR #14964:
URL: https://github.com/apache/iceberg/pull/14964#discussion_r2676525260
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -157,13 +158,26 @@ public RewriteDataFilesSparkAction filter(Expression
expression) {
return this;
}
+ public RewriteDataFilesSparkAction toBranch(String targetBranch) {
+ if (targetBranch != null) {
+ this.branch = targetBranch;
+ }
+ return this;
Review Comment:
Thank you this makes sense, the only logical path out of this I see is to
default it to MAIN_BRANCH
Updated to align with core Iceberg API behavior.
SnapshotProducer.targetBranch() defaults to MAIN_BRANCH and rejects null with
IllegalArgumentException - since RewriteDataFilesSparkAction eventually calls
through to SnapshotProducer via rewrite.toBranch(), it makes sense to have
consistent behavior at the action level.
Changes:
- branch now defaults to SnapshotRef.MAIN_BRANCH instead of null
- toBranch(null) now throws IllegalArgumentException (matching
SnapshotProducer)
- Removed null-check guards in execute() that are no longer needed
- Added test testRewriteDataFilesToNullBranchFails
cc: @huaxingao
--
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]
