[#401] feat(api): Introduce default column position (#927)

### What changes were proposed in this pull request?

Introduce a default column position for unspecified positions when
adding a column.

### Why are the changes needed?

Passing null values in code carries risks and uncertainties. Using
default values can solve this problem and improve the robustness of the
code.


Fix: #401 

### Does this PR introduce _any_ user-facing change?

no, users can still use null, but it will be internally converted to the
default value.

### How was this patch tested?
adding new UTs

api/src/main/java/com/datastrato/gravitino/rel/TableChange.java

@@ -345,6 +345,10 @@ static ColumnPosition first() {
     static ColumnPosition after(String column) {
       return new After(column);
     }
+
+    static ColumnPosition defaultPos() {
+      return Default.INSTANCE;
+    }
   }
 
   /**
@@ -384,6 +388,21 @@ public String toString() {
     }
   }
 
+  /**
+   * Column position DEFAULT means the position of the column was ignored by the user, and should be
+   * determined by the catalog implementation.
+   */
+  final class Default implements ColumnPosition {
+    private static final Default INSTANCE = new Default();
+
+    private Default() {}
+
+    @Override
+    public String toString() {
+      return "DEFAULT";
+    }
+  }
+
   interface ColumnChange extends TableChange {
     String[] fieldName();
   }
@@ -418,7 +437,7 @@ private AddColumn(
       this.fieldName = fieldName;
       this.dataType = dataType;
       this.comment = comment;
-      this.position = position;
+      this.position = position == null ? ColumnPosition.defaultPos() : position;
       this.nullable = nullable;
     }
 

api/src/test/java/com/datastrato/gravitino/TestTableChange.java

@@ -75,7 +75,7 @@ public void testAddColumn() {
     assertArrayEquals(fieldName, addColumn.fieldName());
     assertEquals(dataType, addColumn.getDataType());
     assertEquals(comment, addColumn.getComment());
-    assertNull(addColumn.getPosition());
+    assertEquals(ColumnPosition.defaultPos(), addColumn.getPosition());
   }
 
   @Test
@@ -101,7 +101,7 @@ public void testAddColumnWithNullCommentAndPosition() {
     assertArrayEquals(fieldName, addColumn.fieldName());
     assertEquals(dataType, addColumn.getDataType());
     assertNull(addColumn.getComment());
-    assertNull(addColumn.getPosition());
+    assertEquals(ColumnPosition.defaultPos(), addColumn.getPosition());
   }
 
   @Test

catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java

@@ -782,11 +782,11 @@ private void doRemoveProperty(
 
   private void doAddColumn(List<FieldSchema> cols, TableChange.AddColumn change) {
     int targetPosition;
-    if (change.getPosition() == null) {
+    if (change.getPosition() instanceof TableChange.Default) {
       // add to the end by default
       targetPosition = cols.size();
       LOG.info(
-          "Add position is null, add column {} to the end of non-partition columns",
+          "Hive catalog add column {} to the end of non-partition columns by default",
           change.fieldName()[0]);
     } else {
       targetPosition = columnPosition(cols, change.getPosition());

catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java

@@ -387,20 +387,15 @@ private String addColumnFieldDefinition(
     }
 
     // Append position if available
-    if (addColumn.getPosition() != null) {
-      if (addColumn.getPosition() instanceof TableChange.First) {
-        columnDefinition.append("FIRST");
-      } else if (addColumn.getPosition() instanceof TableChange.After) {
-        TableChange.After afterPosition = (TableChange.After) addColumn.getPosition();
-        columnDefinition.append("AFTER ").append(afterPosition.getColumn());
-      }
+    if (addColumn.getPosition() instanceof TableChange.First) {
+      columnDefinition.append("FIRST");
+    } else if (addColumn.getPosition() instanceof TableChange.After) {
+      TableChange.After afterPosition = (TableChange.After) addColumn.getPosition();
+      columnDefinition.append("AFTER ").append(afterPosition.getColumn());
+    } else if (addColumn.getPosition() instanceof TableChange.Default) {
+      // do nothing, follow the default behavior of mysql
     } else {
-      List<ColumnDefinition> columnDefinitions = createTable.getColumnDefinitions();
-      if (CollectionUtils.isNotEmpty(columnDefinitions)) {
-        columnDefinition
-            .append("AFTER ")
-            .append(getColumnName(columnDefinitions.get(columnDefinitions.size() - 1)));
-      }
+      throw new IllegalArgumentException("Invalid column position.");
     }
     return columnDefinition.toString();
   }

catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/ops/IcebergTableOpsHelper.java

@@ -160,7 +160,7 @@ private void doUpdateColumnType(
   }
 
   private ColumnPosition getAddColumnPosition(StructType parent, ColumnPosition columnPosition) {
-    if (columnPosition != null) {
+    if (!(columnPosition instanceof TableChange.Default)) {
       return columnPosition;
     }
 

common/src/main/java/com/datastrato/gravitino/dto/requests/TableUpdateRequest.java

@@ -214,6 +214,15 @@ public AddTableColumnRequest() {
       this(null, null, null, null, true);
     }
 
+    /**
+     * Constructor for AddTableColumnRequest.
+     *
+     * @param fieldName the field name to add
+     * @param dataType the data type of the field to add
+     * @param comment the comment of the field to add
+     * @param position the position of the field to add, null for default position
+     * @param nullable whether the field to add is nullable
+     */
     public AddTableColumnRequest(
         String[] fieldName,
         Type dataType,
@@ -223,15 +232,34 @@ public AddTableColumnRequest(
       this.fieldName = fieldName;
       this.dataType = dataType;
       this.comment = comment;
-      this.position = position;
+      this.position = position == null ? TableChange.ColumnPosition.defaultPos() : position;
       this.nullable = nullable;
     }
 
+    /**
+     * Constructor for AddTableColumnRequest with default nullable value(true).
+     *
+     * @param fieldName the field name to add
+     * @param dataType the data type of the field to add
+     * @param comment the comment of the field to add
+     * @param position the position of the field to add
+     */
     public AddTableColumnRequest(
         String[] fieldName, Type dataType, String comment, TableChange.ColumnPosition position) {
       this(fieldName, dataType, comment, position, true);
     }
 
+    /**
+     * Constructor for AddTableColumnRequest with default position and nullable value(true).
+     *
+     * @param fieldName the field name to add
+     * @param dataType the data type of the field to add
+     * @param comment the comment of the field to add
+     */
+    public AddTableColumnRequest(String[] fieldName, Type dataType, String comment) {
+      this(fieldName, dataType, comment, TableChange.ColumnPosition.defaultPos());
+    }
+
     @Override
     public void validate() throws IllegalArgumentException {
       Preconditions.checkArgument(

common/src/main/java/com/datastrato/gravitino/json/JsonUtils.java

@@ -63,7 +63,8 @@ public class JsonUtils {
   private static final String NAMESPACE = "namespace";
   private static final String NAME = "name";
   private static final String POSITION_FIRST = "first";
-  private static final String POSITION_LAST = "last";
+  private static final String POSITION_AFTER = "after";
+  private static final String POSITION_DEFAULT = "default";
   private static final String STRATEGY = "strategy";
   private static final String FIELD_NAME = "fieldName";
   private static final String FIELD_NAMES = "fieldNames";
@@ -679,9 +680,11 @@ public void serialize(
       } else if (value instanceof TableChange.After) {
         gen.writeStartObject();
         TableChange.After after = (TableChange.After) value;
-        gen.writeStringField(POSITION_LAST, after.getColumn());
+        gen.writeStringField(POSITION_AFTER, after.getColumn());
         gen.writeEndObject();
 
+      } else if (value instanceof TableChange.Default) {
+        gen.writeString(POSITION_DEFAULT);
       } else {
         throw new IOException("Unknown column position: " + value);
       }
@@ -701,8 +704,10 @@ public TableChange.ColumnPosition deserialize(JsonParser p, DeserializationConte
           node);
       if (node.isTextual() && node.asText().equals(POSITION_FIRST)) {
         return TableChange.ColumnPosition.first();
+      } else if (node.isTextual() && node.asText().equals(POSITION_DEFAULT)) {
+        return TableChange.ColumnPosition.defaultPos();
       } else if (node.isObject()) {
-        String afterColumn = getString(POSITION_LAST, node);
+        String afterColumn = getString(POSITION_AFTER, node);
         return TableChange.ColumnPosition.after(afterColumn);
       } else {
         throw new IOException("Unknown json column position: " + node);

common/src/test/java/com/datastrato/gravitino/dto/requests/TestTableUpdatesRequest.java

@@ -103,4 +103,72 @@ public void testTableUpdatesRequest() throws JsonProcessingException {
     Assertions.assertEquals(
         JsonUtils.objectMapper().readTree(expected), JsonUtils.objectMapper().readTree(jsonString));
   }
+
+  @Test
+  public void testAddTableColumnRequest() throws JsonProcessingException {
+    TableUpdateRequest addTableColumnRequest =
+        new TableUpdateRequest.AddTableColumnRequest(
+            new String[] {"column"},
+            Types.StringType.get(),
+            "comment",
+            TableChange.ColumnPosition.after("afterColumn"),
+            false);
+    String jsonString = JsonUtils.objectMapper().writeValueAsString(addTableColumnRequest);
+    String expected =
+        "{\n"
+            + "  \"@type\": \"addColumn\",\n"
+            + "  \"fieldName\": [\n"
+            + "    \"column\"\n"
+            + "  ],\n"
+            + "  \"type\": \"string\",\n"
+            + "  \"comment\": \"comment\",\n"
+            + "  \"position\": {\n"
+            + "    \"after\": \"afterColumn\"\n"
+            + "  },\n"
+            + "  \"nullable\": false\n"
+            + "}";
+    Assertions.assertEquals(
+        JsonUtils.objectMapper().readTree(expected), JsonUtils.objectMapper().readTree(jsonString));
+
+    // test default nullability
+    addTableColumnRequest =
+        new TableUpdateRequest.AddTableColumnRequest(
+            new String[] {"column"},
+            Types.StringType.get(),
+            "test default nullability",
+            TableChange.ColumnPosition.first());
+    jsonString = JsonUtils.objectMapper().writeValueAsString(addTableColumnRequest);
+    expected =
+        "{\n"
+            + "  \"@type\": \"addColumn\",\n"
+            + "  \"fieldName\": [\n"
+            + "    \"column\"\n"
+            + "  ],\n"
+            + "  \"type\": \"string\",\n"
+            + "  \"comment\": \"test default nullability\",\n"
+            + "  \"position\": \"first\",\n"
+            + "  \"nullable\": true\n"
+            + "}";
+    Assertions.assertEquals(
+        JsonUtils.objectMapper().readTree(expected), JsonUtils.objectMapper().readTree(jsonString));
+
+    // test default position
+    addTableColumnRequest =
+        new TableUpdateRequest.AddTableColumnRequest(
+            new String[] {"column"}, Types.StringType.get(), "test default position");
+    jsonString = JsonUtils.objectMapper().writeValueAsString(addTableColumnRequest);
+    expected =
+        "{\n"
+            + "  \"@type\": \"addColumn\",\n"
+            + "  \"fieldName\": [\n"
+            + "    \"column\"\n"
+            + "  ],\n"
+            + "  \"type\": \"string\",\n"
+            + "  \"comment\": \"test default position\",\n"
+            + "  \"position\": \"default\",\n"
+            + "  \"nullable\": true\n"
+            + "}";
+    Assertions.assertEquals(
+        JsonUtils.objectMapper().readTree(expected), JsonUtils.objectMapper().readTree(jsonString));
+  }
 }

integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java

@@ -300,22 +300,19 @@ public void testAlterTable() {
 
     newComment = "txt3";
     String newCol2Comment = "xxx";
-    // update column position 、comment and add column、set table properties
+    // update column position 、comment and add column(by default position)、set table properties
     MYSQL_TABLE_OPERATIONS.alterTable(
         TEST_DB_NAME,
         tableName,
         TableChange.updateColumnPosition(
             new String[] {newColName_1}, TableChange.ColumnPosition.after(newColName_2)),
         TableChange.updateComment(newComment),
-        TableChange.addColumn(
-            new String[] {"col_3"}, VARCHAR, "txt3", TableChange.ColumnPosition.first()),
+        TableChange.addColumn(new String[] {"col_3"}, VARCHAR, "txt3"),
         TableChange.updateColumnComment(new String[] {newColName_2}, newCol2Comment));
     load = MYSQL_TABLE_OPERATIONS.load(TEST_DB_NAME, tableName);
 
     columns.clear();
 
-    columns.add(
-        new JdbcColumn.Builder().withName("col_3").withType(VARCHAR).withComment("txt3").build());
     columns.add(
         new JdbcColumn.Builder()
             .withName(col_2.name())
@@ -326,6 +323,8 @@ public void testAlterTable() {
             .withNullable(col_2.nullable())
             .build());
     columns.add(col_1);
+    columns.add(
+        new JdbcColumn.Builder().withName("col_3").withType(VARCHAR).withComment("txt3").build());
     //    properties.put("ROW_FORMAT", "DYNAMIC");
     assertionsTableInfo(tableName, newComment, columns, properties, load);