Fix the issues from the code review.

apache · Mar 14, 2024 · a3a4813 · a3a4813
1 parent d8ed020
commit a3a4813
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 41 deletions.
diff --git a/catalogs/catalog-jdbc-doris/build.gradle.kts b/catalogs/catalog-jdbc-doris/build.gradle.kts
@@ -31,9 +31,9 @@ dependencies {
 
   testImplementation(libs.commons.lang3)
   testImplementation(libs.guava)
-  testImplementation(libs.mysql.driver)
   testImplementation(libs.junit.jupiter.api)
   testImplementation(libs.junit.jupiter.params)
+  testImplementation(libs.mysql.driver)
   testImplementation(libs.testcontainers)
   testImplementation(libs.testcontainers.mysql)
 

diff --git a/...is/src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisTypeConverter.java b/...is/src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisTypeConverter.java
@@ -88,6 +88,13 @@ public String fromGravitinoType(Type type) {
     } else if (type instanceof Types.VarCharType) {
       return VARCHAR + "(" + ((Types.VarCharType) type).length() + ")";
     } else if (type instanceof Types.FixedCharType) {
+      int length = ((Types.FixedCharType) type).length();
+      if (length < 1 || length > 255) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Type %s is invalid, length should be between 1 and 255", type.toString()));
+      }
+
       return CHAR + "(" + ((Types.FixedCharType) type).length() + ")";
     } else if (type instanceof Types.StringType) {
       return STRING;

diff --git a/...c/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisDatabaseOperations.java b/...c/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisDatabaseOperations.java
@@ -21,6 +21,8 @@
 
 /** Database operations for Doris. */
 public class DorisDatabaseOperations extends JdbcDatabaseOperations {
+  private static final String BACK_QUOTE = "`";
+
   @Override
   public String generateCreateDatabaseSql(
       String databaseName, String comment, Map<String, String> properties) {
@@ -31,7 +33,7 @@ public String generateCreateDatabaseSql(
     StringBuilder sqlBuilder = new StringBuilder("CREATE DATABASE ");
 
     // Append database name
-    sqlBuilder.append("`").append(databaseName).append("`");
+    sqlBuilder.append(BACK_QUOTE).append(databaseName).append(BACK_QUOTE);
 
     // Append properties
     sqlBuilder.append(DorisUtils.generatePropertiesSql(properties));
@@ -44,14 +46,14 @@ public String generateCreateDatabaseSql(
   @Override
   public String generateDropDatabaseSql(String databaseName, boolean cascade) {
     StringBuilder sqlBuilder = new StringBuilder("DROP DATABASE");
-    sqlBuilder.append("`").append(databaseName).append("`");
+    sqlBuilder.append(BACK_QUOTE).append(databaseName).append(BACK_QUOTE);
     if (cascade) {
       sqlBuilder.append(" FORCE");
       return sqlBuilder.toString();
     }
 
     try (final Connection connection = this.dataSource.getConnection()) {
-      String query = "SHOW TABLES IN " + databaseName;
+      String query = "SHOW TABLES IN " + BACK_QUOTE + databaseName + BACK_QUOTE;
       try (Statement statement = connection.createStatement()) {
         // Execute the query and check if there exists any tables in the database
         try (ResultSet resultSet = statement.executeQuery(query)) {

diff --git a/.../src/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisTableOperations.java b/.../src/main/java/com/datastrato/gravitino/catalog/doris/operation/DorisTableOperations.java
@@ -22,7 +22,6 @@
 import com.datastrato.gravitino.rel.expressions.transforms.Transform;
 import com.datastrato.gravitino.rel.indexes.Index;
 import com.datastrato.gravitino.rel.indexes.Indexes;
-import com.datastrato.gravitino.rel.types.Types;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import java.sql.Connection;
@@ -43,10 +42,10 @@
 
 /** Table operations for Doris. */
 public class DorisTableOperations extends JdbcTableOperations {
-  public static final String BACK_QUOTE = "`";
-  public static final String DORIS_AUTO_INCREMENT = "AUTO_INCREMENT";
+  private static final String BACK_QUOTE = "`";
+  private static final String DORIS_AUTO_INCREMENT = "AUTO_INCREMENT";
 
-  public static final String NEW_LINE = "\n";
+  private static final String NEW_LINE = "\n";
 
   @Override
   public List<String> listTables(String databaseName) throws NoSuchSchemaException {
@@ -142,37 +141,13 @@ protected String generateCreateTableSql(
   }
 
   private static void validateIncrementCol(JdbcColumn[] columns) {
-    // Check auto increment column
+    // Get all auto increment column
     List<JdbcColumn> autoIncrementCols =
         Arrays.stream(columns).filter(Column::autoIncrement).collect(Collectors.toList());
-    String autoIncrementColsStr =
-        autoIncrementCols.stream().map(JdbcColumn::name).collect(Collectors.joining(",", "[", "]"));
 
+    // Doris does not support auto increment column before version 2.1.0
     Preconditions.checkArgument(
-        autoIncrementCols.size() <= 1,
-        "Only one column can be auto-incremented. There are multiple auto-increment columns in your table: "
-            + autoIncrementColsStr);
-
-    // Check Auto increment column type
-    autoIncrementCols.forEach(
-        column -> {
-          Preconditions.checkArgument(
-              column.dataType().equals(Types.LongType.get()),
-              "Auto-increment column must be of type BIGINT. The column "
-                  + column.name()
-                  + " is of type "
-                  + column.dataType());
-        });
-
-    // Check auto increment column is nullable
-    autoIncrementCols.forEach(
-        column -> {
-          Preconditions.checkArgument(
-              !column.nullable(),
-              "Auto-increment column must be not nullable. The column "
-                  + column.name()
-                  + " is nullable");
-        });
+        autoIncrementCols.isEmpty(), "Doris does not support auto-increment column");
   }
 
   private static void validateDistribution(Distribution distribution, JdbcColumn[] columns) {
@@ -205,12 +180,11 @@ static void appendIndexesSql(
 
     // validate indexes
     Arrays.stream(indexes)
-        .map(
+        .forEach(
             index -> {
               if (index.fieldNames().length > 1) {
                 throw new IllegalArgumentException("Index does not support multi fields in Doris");
               }
-              return index;
             });
 
     String indexSql =
@@ -266,7 +240,8 @@ protected List<Index> getIndexes(String databaseName, String tableName, Database
     try {
       Connection connection = metaData.getConnection();
 
-      String sql = "SHOW INDEX FROM " + tableName + " FROM " + databaseName;
+      String sql = String.format("SHOW INDEX FROM `%s` FROM `%s`", tableName, databaseName);
+
       PreparedStatement preparedStatement = connection.prepareStatement(sql);
       ResultSet resultSet = preparedStatement.executeQuery();
       List<Index> indexes = new ArrayList<>();
@@ -510,13 +485,17 @@ private String updateColumnPositionFieldDefinition(
     String col = updateColumnPosition.fieldName()[0];
     JdbcColumn column = getJdbcColumnFromTable(jdbcTable, col);
     StringBuilder columnDefinition = new StringBuilder();
-    columnDefinition.append("MODIFY COLUMN ").append(col);
+    columnDefinition.append("MODIFY COLUMN ").append(BACK_QUOTE).append(col).append(BACK_QUOTE);
     appendColumnDefinition(column, columnDefinition);
     if (updateColumnPosition.getPosition() instanceof TableChange.First) {
       columnDefinition.append("FIRST");
     } else if (updateColumnPosition.getPosition() instanceof TableChange.After) {
       TableChange.After afterPosition = (TableChange.After) updateColumnPosition.getPosition();
-      columnDefinition.append("AFTER ").append(afterPosition.getColumn());
+      columnDefinition
+          .append("AFTER ")
+          .append(BACK_QUOTE)
+          .append(afterPosition.getColumn())
+          .append(BACK_QUOTE);
     } else {
       Arrays.stream(jdbcTable.columns())
           .reduce((column1, column2) -> column2)
@@ -555,7 +534,7 @@ private String updateColumnTypeFieldDefinition(
     }
     String col = updateColumnType.fieldName()[0];
     JdbcColumn column = getJdbcColumnFromTable(jdbcTable, col);
-    StringBuilder sqlBuilder = new StringBuilder("MODIFY COLUMN " + col);
+    StringBuilder sqlBuilder = new StringBuilder("MODIFY COLUMN " + BACK_QUOTE + col + BACK_QUOTE);
     JdbcColumn newColumn =
         new JdbcColumn.Builder()
             .withName(col)