You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "yuzelin (via GitHub)" <gi...@apache.org> on 2023/03/30 07:14:10 UTC

[GitHub] [incubator-paimon] yuzelin opened a new pull request, #769: [core] Introduce dynamic partition overwrite as default behavior

yuzelin opened a new pull request, #769:
URL: https://github.com/apache/incubator-paimon/pull/769

   ### Purpose
   
   Currently, `INSERT OVERWRITE` in Paimon will first purge all partitions specified in `PARTITION (...)`. In hive, the overwrite statement only purges partitions appeared in new data by default (called dynamic partition overwrite).
   This PR introduce dynamic partition overwrite as default behavior.
   
   ### Tests
   ReadWriteTableITCase:
   - `testOverwriteNothing`
   - `testSpecifySinglePartitionKey`
   - `testDynamicOverwrite`
   
   ### API and Format 
   CoreOptions:
   - streaming-read-overwrite -> overwrite.support-streaming-read
   - New: overwrite.dynamic-partition
   
   ### Documentation
   
   Modified: how-to#Writing Tables


-- 
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


[GitHub] [incubator-paimon] JingsongLi merged pull request #769: [core] Introduce dynamic partition overwrite as default behavior

Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #769:
URL: https://github.com/apache/incubator-paimon/pull/769


-- 
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


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

Posted by "yuzelin (via GitHub)" <gi...@apache.org>.
yuzelin commented on PR #769:
URL: https://github.com/apache/incubator-paimon/pull/769#issuecomment-1491313435

   ![278C6CA1-099E-4ED8-974D-11F174C3E2BA](https://user-images.githubusercontent.com/33053040/229030223-7abb0709-fe26-4f32-a365-8260e518e291.png)
   
   verified.


-- 
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


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

Posted by "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


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

Posted by "yuzelin (via GitHub)" <gi...@apache.org>.
yuzelin commented on code in PR #769:
URL: https://github.com/apache/incubator-paimon/pull/769#discussion_r1152982588


##########
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:
   No problem.



-- 
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