linyanghao commented on code in PR #7628:
URL: https://github.com/apache/iceberg/pull/7628#discussion_r1322712913


##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java:
##########
@@ -322,39 +323,265 @@ public void testAlterTable() throws 
TableNotExistException {
     Assert.assertEquals(properties, table("tl").properties());
 
     // remove property
-    CatalogTable catalogTable = catalogTable("tl");
+    sql("ALTER TABLE tl RESET('oldK')");
     properties.remove("oldK");
-    getTableEnv()
-        .getCatalog(getTableEnv().getCurrentCatalog())
-        .get()
-        .alterTable(new ObjectPath(DATABASE, "tl"), 
catalogTable.copy(properties), false);
     Assert.assertEquals(properties, table("tl").properties());
   }
 
   @Test
-  public void testAlterTableWithPrimaryKey() throws TableNotExistException {
-    sql("CREATE TABLE tl(id BIGINT, PRIMARY KEY(id) NOT ENFORCED) WITH 
('oldK'='oldV')");
-    Map<String, String> properties = Maps.newHashMap();
-    properties.put("oldK", "oldV");
+  public void testAlterTableAddColumn() {
+    sql("CREATE TABLE tl(id BIGINT)");
+    Schema schemaBefore = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(Types.NestedField.optional(1, "id", 
Types.LongType.get())).asStruct(),
+        schemaBefore.asStruct());
 
-    // new
-    sql("ALTER TABLE tl SET('newK'='newV')");
-    properties.put("newK", "newV");
-    Assert.assertEquals(properties, table("tl").properties());
+    sql("ALTER TABLE tl ADD (dt STRING)");
+    Schema schemaAfter1 = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaAfter1.asStruct());
 
-    // update old
-    sql("ALTER TABLE tl SET('oldK'='oldV2')");
-    properties.put("oldK", "oldV2");
-    Assert.assertEquals(properties, table("tl").properties());
+    // Add multiple columns
+    sql("ALTER TABLE tl ADD (col1 STRING, col2 BIGINT)");
+    Schema schemaAfter2 = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()),
+                Types.NestedField.optional(3, "col1", Types.StringType.get()),
+                Types.NestedField.optional(4, "col2", Types.LongType.get()))
+            .asStruct(),
+        schemaAfter2.asStruct());
 
-    // remove property
-    CatalogTable catalogTable = catalogTable("tl");
-    properties.remove("oldK");
-    getTableEnv()
-        .getCatalog(getTableEnv().getCurrentCatalog())
-        .get()
-        .alterTable(new ObjectPath(DATABASE, "tl"), 
catalogTable.copy(properties), false);
-    Assert.assertEquals(properties, table("tl").properties());
+    // Adding a required field should fail
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl ADD (pk STRING NOT 
NULL)"))
+        .isInstanceOf(TableException.class);
+
+    // Adding an existing field should fail
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl ADD (id STRING)"))
+        .isInstanceOf(ValidationException.class);
+  }
+
+  @Test
+  public void testAlterTableDropColumn() {
+    sql("CREATE TABLE tl(id BIGINT, dt STRING, col1 STRING, col2 BIGINT)");
+    Schema schemaBefore = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()),
+                Types.NestedField.optional(3, "col1", Types.StringType.get()),
+                Types.NestedField.optional(4, "col2", Types.LongType.get()))
+            .asStruct(),
+        schemaBefore.asStruct());
+
+    sql("ALTER TABLE tl DROP (dt)");
+    Schema schemaAfter1 = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(3, "col1", Types.StringType.get()),
+                Types.NestedField.optional(4, "col2", Types.LongType.get()))
+            .asStruct(),
+        schemaAfter1.asStruct());
+
+    // Drop multiple columns
+    sql("ALTER TABLE tl DROP (col1, col2)");
+    Schema schemaAfter2 = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(Types.NestedField.optional(1, "id", 
Types.LongType.get())).asStruct(),
+        schemaAfter2.asStruct());
+
+    // Dropping an non-existing field should fail
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl DROP (foo)"))
+        .isInstanceOf(ValidationException.class);
+
+    // Dropping an already-deleted field should fail
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl DROP (dt)"))
+        .isInstanceOf(ValidationException.class);
+  }
+
+  @Test
+  public void testAlterTableModifyColumnName() {
+    sql("CREATE TABLE tl(id BIGINT, dt STRING)");
+    Schema schemaBefore = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaBefore.asStruct());
+
+    sql("ALTER TABLE tl RENAME dt TO data");
+    Schema schemaAfter = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "data", Types.StringType.get()))
+            .asStruct(),
+        schemaAfter.asStruct());
+  }
+
+  @Test
+  public void testAlterTableModifyColumnType() {
+    sql("CREATE TABLE tl(id INTEGER, dt STRING)");
+    Schema schemaBefore = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.IntegerType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaBefore.asStruct());
+
+    // Promote type from Integer to Long
+    sql("ALTER TABLE tl MODIFY (id BIGINT)");
+    Schema schemaAfter = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.LongType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaAfter.asStruct());
+
+    // Type change that doesn't follow the type-promotion rule should fail
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl MODIFY (dt 
INTEGER)"))
+        .isInstanceOf(TableException.class);
+  }
+
+  @Test
+  public void testAlterTableModifyColumnNullability() {
+    sql("CREATE TABLE tl(id INTEGER NOT NULL, dt STRING)");
+    Schema schemaBefore = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.required(1, "id", Types.IntegerType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaBefore.asStruct());
+
+    // Cannot change nullability from optional to required
+    Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl MODIFY (dt STRING 
NOT NULL)"))
+        .isInstanceOf(TableException.class);
+
+    // Set nullability from required to optional
+    sql("ALTER TABLE tl MODIFY (id INTEGER)");
+    Schema schemaAfter = table("tl").schema();
+    Assert.assertEquals(
+        new Schema(
+                Types.NestedField.optional(1, "id", Types.IntegerType.get()),
+                Types.NestedField.optional(2, "dt", Types.StringType.get()))
+            .asStruct(),
+        schemaAfter.asStruct());
+  }
+
+  @Test
+  public void testAlterTableModifyColumnPosition() {

Review Comment:
   Added.



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