flyrain commented on code in PR #8959:
URL: https://github.com/apache/iceberg/pull/8959#discussion_r1380821842
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java:
##########
@@ -73,8 +73,10 @@
*
* <p>By default, this action rewrites all manifests for the current partition
spec and writes the
* result to the metadata folder. The behavior can be modified by passing a
custom predicate to
- * {@link #rewriteIf(Predicate)} and a custom spec id to {@link #specId(int)}.
In addition, there is
- * a way to configure a custom location for new manifests via {@link
#stagingLocation}.
+ * {@link #rewriteIf(Predicate)} and a custom spec ID to {@link #specId(int)}.
In addition, there is
+ * a way to configure a custom location for staged manifests via {@link
#stagingLocation(String)}.
+ * The provided staging location will be ignored if new manifests are
committed directly. In such
Review Comment:
Is it more clear if we say
```
The provided staging location will be ignored if snapshot inheritance is
enabled.
```
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteManifestsSparkAction.java:
##########
@@ -104,13 +107,20 @@ public class RewriteManifestsSparkAction
TableProperties.MANIFEST_TARGET_SIZE_BYTES,
TableProperties.MANIFEST_TARGET_SIZE_BYTES_DEFAULT);
- // default the staging location to the metadata location
+ // default the output location to the metadata location
TableOperations ops = ((HasTableOperations) table).operations();
Path metadataFilePath = new Path(ops.metadataFileLocation("file"));
- this.stagingLocation = metadataFilePath.getParent().toString();
+ this.outputLocation = metadataFilePath.getParent().toString();
// use the current table format version for new manifests
this.formatVersion = ops.current().formatVersion();
+
+ boolean snapshotIdInheritanceEnabled =
+ PropertyUtil.propertyAsBoolean(
+ table.properties(),
+ TableProperties.SNAPSHOT_ID_INHERITANCE_ENABLED,
+ TableProperties.SNAPSHOT_ID_INHERITANCE_ENABLED_DEFAULT);
+ this.shouldStageManifests = formatVersion == 1 &&
!snapshotIdInheritanceEnabled;
Review Comment:
What if we made change here,
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadata.java#L482-L482,
like this, so that we don't have to keep check the format version?
```
public boolean propertyAsBoolean(String property, boolean defaultValue) {
boolean value = PropertyUtil.propertyAsBoolean(properties, property,
defaultValue);
if (property == TableProperties.SNAPSHOT_ID_INHERITANCE_ENABLED) {
if (formatVersion == 1) {
return value;
} else {
return true;
}
}
return value;
}
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
