anuragmantri commented on code in PR #18406:
URL: https://github.com/apache/iceberg/pull/18406#discussion_r4213255491
##########
spark/v4.2/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java:
##########
@@ -2028,6 +2029,28 @@ public void testSnapshotProperty() {
assertThat(table.currentSnapshot().summary()).containsKeys(commitMetricsKeys);
}
+ @TestTemplate
Review Comment:
Thanks for raising this. I tried it, and a colliding key fails the commit
today with `IllegalArgumentException: Multiple entries with same key:
added-data-files=1 and added-data-files=999`. The rewrite work is thrown away
and the table is left unchanged.
That error comes from `SnapshotSummary.Builder` in core, and it happens the
same way through `snapshotProperty()` and through writes with this session
property, so this PR doesn't change it. #17009 tracks the reserved-key problem
in core, and #17107 tried to fix it there before it went stale.
I'd rather not pin the current Guava error in a Spark test, because it would
break as soon as core fixes #17009. Would you suggest a test that only checks
that the commit fails and the table is unchanged?
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotUpdateSparkAction.java:
##########
@@ -21,14 +21,21 @@
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.spark.SparkSQLProperties;
+import org.apache.iceberg.util.PropertyUtil;
import org.apache.spark.sql.SparkSession;
+import scala.collection.JavaConverters;
abstract class BaseSnapshotUpdateSparkAction<ThisT> extends
BaseSparkAction<ThisT> {
private final Map<String, String> summary = Maps.newHashMap();
protected BaseSnapshotUpdateSparkAction(SparkSession spark) {
super(spark);
+ summary.putAll(
+ PropertyUtil.propertiesWithPrefix(
+ JavaConverters.mapAsJavaMap(spark.conf().getAll()),
+ SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX));
Review Comment:
Good catch on the dangling deletes commit. Explicit properties were still
dropped there, because only session properties reached the nested action. That
also meant an explicit value won on the rewrite commit while the session value
won on the dangling deletes commit.
I also moved the session lookup into `SparkUtil.sessionSnapshotProperties`,
which `BaseSnapshotUpdateSparkAction` and `SparkWriteConf` now share, as you
suggested.
--
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]
