Copilot commented on code in PR #9821:
URL: https://github.com/apache/gravitino/pull/9821#discussion_r2736958470
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -294,10 +294,22 @@ private void appendColumnDefinition(JdbcColumn column,
StringBuilder sqlBuilder)
}
// Add DEFAULT value if specified
if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
- sqlBuilder
- .append("DEFAULT ")
-
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
- .append(SPACE);
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
+ // If the default value is not wrapped in single quotes, wrap it.
+ // This is to support cases where the default value is like " "
(space) or "abc" (unquoted
+ // string).
+ // And standard converters might return unquoted strings in some
versions or
+ // configurations.
+ defaultValue = "'" + defaultValue + "'";
Review Comment:
This new quoting logic will incorrectly convert a NULL default into the
string literal 'NULL' for string-typed columns. Since
`JdbcColumnDefaultValueConverter.fromGravitino(Literals.NULL)` returns `NULL`,
this block will wrap it in quotes. Add a guard to leave SQL NULL unquoted (and
consider handling other SQL keywords consistently).
```suggestion
// Leave SQL NULL keyword unquoted for string-typed columns.
if (!"NULL".equalsIgnoreCase(defaultValue)) {
if (StringUtils.isEmpty(defaultValue)) {
defaultValue = "''";
} else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
// If the default value is not wrapped in single quotes, wrap it.
// This is to support cases where the default value is like " "
(space) or "abc" (unquoted
// string).
// And standard converters might return unquoted strings in some
versions or
// configurations.
defaultValue = "'" + defaultValue + "'";
}
```
##########
catalogs/catalog-jdbc-starrocks/src/test/java/org/apache/gravitino/catalog/starrocks/operation/TestStarRocksWhitespaceDefaultValue.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.starrocks.operation;
+
+import java.util.Collections;
+import org.apache.gravitino.catalog.jdbc.JdbcColumn;
+import
org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter;
+import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter;
+import org.apache.gravitino.catalog.starrocks.converter.StarRocksTypeConverter;
+import
org.apache.gravitino.catalog.starrocks.operations.StarRocksTableOperations;
+import org.apache.gravitino.rel.expressions.distributions.Distribution;
+import org.apache.gravitino.rel.expressions.distributions.Distributions;
+import org.apache.gravitino.rel.expressions.literals.Literals;
+import org.apache.gravitino.rel.expressions.transforms.Transforms;
+import org.apache.gravitino.rel.indexes.Indexes;
+import org.apache.gravitino.rel.types.Types;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestStarRocksWhitespaceDefaultValue {
+
+ private static class TestableStarRocksTableOperations extends
StarRocksTableOperations {
+ public TestableStarRocksTableOperations() {
+ super.exceptionMapper = new JdbcExceptionConverter();
+ super.typeConverter = new StarRocksTypeConverter();
+ super.columnDefaultValueConverter = new
JdbcColumnDefaultValueConverter();
+ }
+
+ public String createTableSql(
+ String tableName, JdbcColumn[] columns, Distribution distribution) {
+ return generateCreateTableSql(
+ tableName,
+ columns,
+ "comment",
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ distribution,
+ Indexes.EMPTY_INDEXES);
+ }
+ }
+
+ @Test
+ public void testCreateTableWithWhitespaceDefaultValue() {
+ TestableStarRocksTableOperations ops = new
TestableStarRocksTableOperations();
+ String tableName = "test_table";
+ JdbcColumn col1 =
+ JdbcColumn.builder()
+ .withName("col1")
+ .withType(Types.VarCharType.of(255))
+ .withNullable(false)
+ .withDefaultValue(Literals.of(" ", Types.VarCharType.of(255)))
+ .build();
+
+ Distribution distribution = Distributions.hash(1,
Distributions.fields("col1"));
+
+ String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1},
distribution);
+ // Logic check: if converter returns "' '", this should pass.
+ // If it returns " " (unquoted), this assert might fail or pass
depending on what we expect.
+ // The user suspects it generates "DEFAULT ;" or similar invalid SQL.
+ // Let's see what it actually generates.
+ System.out.println("Generated SQL: " + sql);
Review Comment:
Avoid `System.out.println` in tests; it makes test output noisy and can fail
style checks. Remove the print/debug comments and rely on assertions (or use
the project logger if output is truly needed).
```suggestion
```
##########
catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java:
##########
@@ -634,10 +634,22 @@ private StringBuilder appendColumnDefinition(JdbcColumn
column, StringBuilder sq
// Add DEFAULT value if specified
if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
- sqlBuilder
- .append("DEFAULT ")
-
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
- .append(SPACE);
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
+ // If the default value is not wrapped in single quotes, wrap it.
+ // This is to support cases where the default value is like " "
(space) or "abc" (unquoted
+ // string).
+ // And standard converters might return unquoted strings in some
versions or
+ // configurations.
+ defaultValue = "'" + defaultValue + "'";
Review Comment:
This new logic can incorrectly quote SQL NULL defaults as 'NULL' for
string-typed columns (the default value converter returns `NULL` for
`Literals.NULL`). Please ensure NULL remains unquoted for string columns.
```suggestion
// For SQL NULL defaults, keep the bare NULL keyword unquoted.
if (!"NULL".equalsIgnoreCase(defaultValue)) {
if (StringUtils.isEmpty(defaultValue)) {
defaultValue = "''";
} else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
// If the default value is not wrapped in single quotes, wrap it.
// This is to support cases where the default value is like " "
(space) or "abc"
// (unquoted string).
// And standard converters might return unquoted strings in some
versions or
// configurations.
defaultValue = "'" + defaultValue + "'";
}
```
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -325,10 +326,22 @@ public StringBuilder appendColumnDefinition(JdbcColumn
column, StringBuilder sql
// Add DEFAULT value if specified
if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
- sqlBuilder
- .append("DEFAULT ")
-
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
- .append(SPACE);
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
+ // If the default value is not wrapped in single quotes, wrap it.
+ // This is to support cases where the default value is like " "
(space) or "abc" (unquoted
+ // string).
+ // And standard converters might return unquoted strings in some
versions or
+ // configurations.
+ defaultValue = "'" + defaultValue + "'";
+ }
+ }
+ sqlBuilder.append("DEFAULT ").append(defaultValue).append(SPACE);
}
Review Comment:
This new logic can incorrectly quote SQL NULL defaults as 'NULL' for
string-typed columns (the default value converter returns `NULL` for
`Literals.NULL`). Please ensure NULL remains unquoted for string columns.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -563,16 +575,25 @@ private String updateColumnDefaultValueFieldDefinition(
}
StringBuilder sqlBuilder = new StringBuilder(ALTER_TABLE +
jdbcTable.name());
+ String newDefaultValue =
+
columnDefaultValueConverter.fromGravitino(updateColumnDefaultValue.getNewDefaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(newDefaultValue)) {
+ newDefaultValue = "''";
+ } else if (!newDefaultValue.startsWith("'") ||
!newDefaultValue.endsWith("'")) {
+ newDefaultValue = "'" + newDefaultValue + "'";
+ }
+ }
Review Comment:
The PR changes `updateColumnDefaultValueFieldDefinition` to handle
quoting/empty string defaults, but there is no unit test covering `alter ...
SET DEFAULT` SQL generation for empty/whitespace string defaults in PostgreSQL.
Please add a focused test for this path (e.g., calling the method indirectly
via `alterTable` SQL generation) to ensure the bug fix is covered.
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:
##########
@@ -619,10 +619,22 @@ private StringBuilder appendColumnDefinition(JdbcColumn
column, StringBuilder sq
// Add DEFAULT value if specified
if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
- sqlBuilder
- .append("DEFAULT ")
-
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
- .append(SPACE);
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
+ // If the default value is not wrapped in single quotes, wrap it.
+ // This is to support cases where the default value is like " "
(space) or "abc" (unquoted
+ // string).
+ // And standard converters might return unquoted strings in some
versions or
+ // configurations.
+ defaultValue = "'" + defaultValue + "'";
+ }
+ }
+ sqlBuilder.append("DEFAULT ").append(defaultValue).append(SPACE);
Review Comment:
The new quoting logic will incorrectly turn a NULL default into the string
literal 'NULL' for string-typed columns.
`JdbcColumnDefaultValueConverter.fromGravitino(Literals.NULL)` returns `NULL`,
and this block will wrap it in single quotes. Please special-case SQL NULL (and
potentially other keywords) so it is not quoted for string columns.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -563,16 +575,25 @@ private String updateColumnDefaultValueFieldDefinition(
}
StringBuilder sqlBuilder = new StringBuilder(ALTER_TABLE +
jdbcTable.name());
+ String newDefaultValue =
+
columnDefaultValueConverter.fromGravitino(updateColumnDefaultValue.getNewDefaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(newDefaultValue)) {
+ newDefaultValue = "''";
+ } else if (!newDefaultValue.startsWith("'") ||
!newDefaultValue.endsWith("'")) {
+ newDefaultValue = "'" + newDefaultValue + "'";
+ }
+ }
Review Comment:
In `updateColumnDefaultValueFieldDefinition`, this logic will wrap a `NULL`
default as `'NULL'` for string-typed columns, changing semantics and generating
incorrect SQL. Since the converter returns `NULL` for `Literals.NULL`, please
skip quoting for SQL NULL here as well.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -750,10 +751,22 @@ private StringBuilder appendColumnDefinition(JdbcColumn
column, StringBuilder sq
// Add DEFAULT value if specified
if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
- sqlBuilder
- .append("DEFAULT ")
-
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
- .append(SPACE);
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
+ if ((column.dataType() instanceof Types.VarCharType
+ || column.dataType() instanceof Types.StringType
+ || column.dataType() instanceof Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
+ // If the default value is not wrapped in single quotes, wrap it.
+ // This is to support cases where the default value is like " "
(space) or "abc" (unquoted
+ // string).
+ // And standard converters might return unquoted strings in some
versions or
+ // configurations.
+ defaultValue = "'" + defaultValue + "'";
+ }
+ }
+ sqlBuilder.append("DEFAULT ").append(defaultValue).append(SPACE);
Review Comment:
This new logic can incorrectly quote SQL NULL defaults as 'NULL' for
string-typed columns (the default value converter returns `NULL` for
`Literals.NULL`). Please ensure NULL remains unquoted for string columns.
```suggestion
if (column.defaultValue() instanceof Literal
&& ((Literal) column.defaultValue()).value() == null) {
// Preserve SQL NULL default without quotes, even for string-typed
columns.
sqlBuilder.append("DEFAULT NULL").append(SPACE);
} else {
String defaultValue =
columnDefaultValueConverter.fromGravitino(column.defaultValue());
if ((column.dataType() instanceof Types.VarCharType
|| column.dataType() instanceof Types.StringType
|| column.dataType() instanceof Types.FixedCharType)) {
if (StringUtils.isEmpty(defaultValue)) {
defaultValue = "''";
} else if (!defaultValue.startsWith("'") ||
!defaultValue.endsWith("'")) {
// If the default value is not wrapped in single quotes, wrap it.
// This is to support cases where the default value is like " "
(space) or "abc" (unquoted
// string).
// And standard converters might return unquoted strings in some
versions or
// configurations.
defaultValue = "'" + defaultValue + "'";
}
}
sqlBuilder.append("DEFAULT ").append(defaultValue).append(SPACE);
}
```
--
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]
