JingsongLi commented on code in PR #8236:
URL: https://github.com/apache/paimon/pull/8236#discussion_r3419528248
##########
paimon-core/src/main/java/org/apache/paimon/table/sink/BatchTableCommit.java:
##########
@@ -71,4 +71,6 @@ public interface BatchTableCommit extends TableCommit {
/** Compact the manifest entries. Generates a snapshot with {@link
CommitKind#COMPACT}. */
void compactManifests();
+
+ BatchTableCommit withCommitProperties(Map<String, String> properties);
Review Comment:
[P2] `BatchTableCommit` is annotated `@Public`, so adding a new abstract
method breaks existing third-party implementations at compile time (and can
surface as `AbstractMethodError` for already-compiled implementations). Since
this extension is optional for older implementations, could we make it a Java 8
`default` method and keep the concrete override in `TableCommitImpl`?
##########
paimon-spark/paimon-spark-common/src/main/scala/org/apache/paimon/spark/commands/DeleteFromPaimonTableCommand.scala:
##########
@@ -46,7 +47,7 @@ case class DeleteFromPaimonTableCommand(
} else {
performNonPrimaryKeyDelete(sparkSession)
}
- writer.commit(commitMessages)
+ writer.commit(commitMessages,
SnapshotOperation.asProperties(SnapshotOperation.DELETE))
Review Comment:
[P2] This only covers row-rewrite deletes. Metadata-only deletes are
optimized before this command runs (for example `DELETE FROM t` or a
partition-only DELETE becomes `TruncatePaimonTableWithFilterExec`), and that
path calls `truncateTable` / `truncatePartitions` without any snapshot
properties. Those DELETE snapshots would still miss `operation=DELETE`, so we
need to carry `SnapshotOperation.DELETE` through the truncate path as well.
--
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]
