hililiwei commented on code in PR #5486:
URL: https://github.com/apache/iceberg/pull/5486#discussion_r949031169


##########
flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestFlinkUpsert.java:
##########
@@ -203,40 +205,40 @@ public void testPrimaryKeyFieldsAtBeginningOfSchema() {
     LocalDate dt = LocalDate.of(2022, 3, 1);
     try {
       sql(
-          "CREATE TABLE %s(data STRING NOT NULL, dt DATE NOT NULL, id INT, 
PRIMARY KEY(data,dt) NOT ENFORCED) "
-              + "PARTITIONED BY (data) WITH %s",
+          "CREATE TABLE %s(name STRING NOT NULL, dt DATE NOT NULL, id INT, 
PRIMARY KEY(name,dt) NOT ENFORCED) "

Review Comment:
   It still confusing to use `name dt` as the primary key. We can put `id` at 
the begining and use it and use `id dt` as the primary key.



##########
flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestFlinkUpsert.java:
##########
@@ -251,40 +253,40 @@ public void testPrimaryKeyFieldsAtEndOfTableSchema() {
     LocalDate dt = LocalDate.of(2022, 3, 1);
     try {
       sql(
-          "CREATE TABLE %s(id INT, data STRING NOT NULL, dt DATE NOT NULL, 
PRIMARY KEY(data,dt) NOT ENFORCED) "
-              + "PARTITIONED BY (data) WITH %s",
+          "CREATE TABLE %s(id INT, name STRING NOT NULL, dt DATE NOT NULL, 
PRIMARY KEY(name,dt) NOT ENFORCED) "

Review Comment:
   Like the previous one, we can put `id` after `name` and use `id dt` as the 
primary key.
   



##########
flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestFlinkUpsert.java:
##########
@@ -127,33 +127,35 @@ public void testUpsertAndQuery() {
     LocalDate dt20220302 = LocalDate.of(2022, 3, 2);
 
     sql(
-        "CREATE TABLE %s(id INT NOT NULL, province STRING NOT NULL, dt DATE, 
PRIMARY KEY(id,province) NOT ENFORCED) "
-            + "PARTITIONED BY (province) WITH %s",
+        "CREATE TABLE %s(id INT NOT NULL, name STRING NOT NULL, dt DATE, 
PRIMARY KEY(id,dt) NOT ENFORCED) "
+            + "PARTITIONED BY (dt) WITH %s",
         tableName, toWithClause(tableUpsertProps));
 
     try {
       sql(
           "INSERT INTO %s VALUES "
-              + "(1, 'a', DATE '2022-03-01'),"
-              + "(2, 'b', DATE '2022-03-01'),"
-              + "(1, 'b', DATE '2022-03-01')",
+              + "(1, 'Bob', DATE '2022-03-01'),"
+              + "(2, 'Jane', DATE '2022-03-01'),"
+              + "(1, 'Jane', DATE '2022-03-01')",
           tableName);
 
       sql(
           "INSERT INTO %s VALUES "
-              + "(4, 'a', DATE '2022-03-02'),"
-              + "(5, 'b', DATE '2022-03-02'),"
-              + "(1, 'b', DATE '2022-03-02')",
+              + "(4, 'Bob', DATE '2022-03-02'),"
+              + "(5, 'Jane', DATE '2022-03-02'),"
+              + "(1, 'Jane', DATE '2022-03-02')",

Review Comment:
   It is not verified that the second SQL will upsert the data in the first. I 
think is incorrect.
   For example, adding "(1, 'Jane', DATE '2022-03-01')"
   



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

Reply via email to