[GitHub] [incubator-paimon] JingsongLi commented on a diff in pull request #769: [core] Introduce dynamic partition overwrite as default behavior

["JingsongLi (via GitHub)" <gi...@apache.org>]

JingsongLi commented on code in PR #769:
URL: https://github.com/apache/incubator-paimon/pull/769#discussion_r1152918573


##########
paimon-common/src/main/java/org/apache/paimon/predicate/PredicateBuilder.java:
##########
@@ -344,4 +346,24 @@ public static Predicate partition(Map<String, String> map, RowType rowType) {
         }
         return predicate;
     }
+
+    public static Predicate partition(BinaryRow partition, RowType rowType) {

Review Comment:
   equalPartition



##########
docs/content/how-to/writing-tables.md:
##########
@@ -80,8 +80,10 @@ For more information, please check the syntax document:
 [Spark INSERT Statement](https://spark.apache.org/docs/latest/sql-ref-syntax-dml-insert-table.html)
 
 {{< hint info >}}
-Streaming reading will ignore the commits generated by `INSERT OVERWRITE` by default. If you want to read the
-commits of `OVERWRITE`, you can configure `streaming-read-overwrite`.
+1. Streaming reading will ignore the commits generated by `INSERT OVERWRITE` by default. If you want to read the
+commits of `OVERWRITE`, you can configure `overwrite.support-streaming-read`.

Review Comment:
   I prefer to keep `streaming-read-overwrite`.
   And `dynamic-partition-overwrite`?



##########
paimon-common/src/main/java/org/apache/paimon/predicate/PredicateBuilder.java:
##########
@@ -344,4 +346,24 @@ public static Predicate partition(Map<String, String> map, RowType rowType) {
         }
         return predicate;
     }
+
+    public static Predicate partition(BinaryRow partition, RowType rowType) {

Review Comment:
   RowType partitionType



##########
paimon-common/src/main/java/org/apache/paimon/predicate/PredicateBuilder.java:
##########
@@ -344,4 +346,24 @@ public static Predicate partition(Map<String, String> map, RowType rowType) {
         }
         return predicate;
     }
+
+    public static Predicate partition(BinaryRow partition, RowType rowType) {
+        Preconditions.checkArgument(
+                partition.getFieldCount() == rowType.getFieldCount(),
+                "Partition's field count should be equal to rowType's field count.");

Review Comment:
   partitionType's



##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -286,28 +287,53 @@ public void overwrite(
             LOG.warn(warnMessage.toString());
         }
 
-        // sanity check, all changes must be done within the given partition
-        Predicate partitionFilter = PredicateBuilder.partition(partition, partitionType);
-        if (partitionFilter != null) {
-            for (ManifestEntry entry : appendTableFiles) {
-                if (!partitionFilter.test(partitionObjectConverter.convert(entry.partition()))) {
-                    throw new IllegalArgumentException(
-                            "Trying to overwrite partition "
-                                    + partition
-                                    + ", but the changes in "
-                                    + pathFactory.getPartitionString(entry.partition())
-                                    + " does not belong to this partition");
+        boolean skipOverwrite = false;
+        // partition filter is built from static or dynamic partition according to properties
+        Predicate partitionFilter = null;
+        if (CoreOptions.fromMap(properties).dynamicPartitionOverwrite()) {

Review Comment:
   Here should read from table options instead of commit properties.



##########
docs/content/how-to/writing-tables.md:
##########
@@ -151,7 +153,8 @@ INSERT OVERWRITE MyTable PARTITION (key1 = value1, key2 = value2, ...) SELECT ..
 
 ## Purging tables
 
-You can use `INSERT OVERWRITE` to purge tables by inserting empty value.
+You can use `INSERT OVERWRITE` to purge tables by inserting empty value. Make sure that the table's

Review Comment:
   For partitioned table, make sure ....



##########
paimon-core/src/test/java/org/apache/paimon/operation/TestCommitThread.java:
##########
@@ -160,7 +162,10 @@ private void doOverwrite() throws Exception {
                         commit.overwrite(
                                 TestKeyValueGenerator.toPartitionMap(partition, MULTI_PARTITIONED),
                                 committable,
-                                Collections.emptyMap()));
+                                // use static partition overwrite mode
+                                new Options()

Review Comment:
    There is no test in `FileStoreCommitTest`.



-- 
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-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org