Copilot commented on code in PR #9821:
URL: https://github.com/apache/gravitino/pull/9821#discussion_r2739720198
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -95,6 +97,28 @@ public void initialize(
this.columnDefaultValueConverter = jdbcColumnDefaultValueConverter;
}
+ protected String calculateDefaultValue(JdbcColumn column, Expression
defaultValueExpression) {
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(defaultValueExpression);
+ if ((column.dataType() instanceof
org.apache.gravitino.rel.types.Types.VarCharType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.StringType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") || !defaultValue.endsWith("'"))
{
+ defaultValue = "'" + defaultValue + "'";
+ }
+ }
+ return defaultValue;
+ }
+
+ protected void appendDefaultValue(JdbcColumn column, StringBuilder
sqlBuilder) {
+ if (DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
+ return;
+ }
+ String defaultValue = calculateDefaultValue(column, column.defaultValue());
+ sqlBuilder.append("DEFAULT ").append(defaultValue).append(SPACE);
+ }
Review Comment:
The new default-value quoting logic changes behavior for string-like
columns, but there’s no unit test covering `updateColumnDefaultValue(...,
Literals.NULL)` (dropping/setting default to SQL NULL). Add a test asserting
generated SQL uses `DEFAULT NULL` (not `'NULL'`) for string columns to prevent
regressions.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -95,6 +97,28 @@ public void initialize(
this.columnDefaultValueConverter = jdbcColumnDefaultValueConverter;
}
+ protected String calculateDefaultValue(JdbcColumn column, Expression
defaultValueExpression) {
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(defaultValueExpression);
+ if ((column.dataType() instanceof
org.apache.gravitino.rel.types.Types.VarCharType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.StringType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.FixedCharType)) {
+ if (StringUtils.isEmpty(defaultValue)) {
+ defaultValue = "''";
+ } else if (!defaultValue.startsWith("'") || !defaultValue.endsWith("'"))
{
+ defaultValue = "'" + defaultValue + "'";
+ }
+ }
+ return defaultValue;
+ }
+
Review Comment:
`calculateDefaultValue` will wrap the converted value in quotes for
string-like columns. When the user sets the default to `Literals.NULL`,
`JdbcColumnDefaultValueConverter` returns `NULL`, and this method turns it into
`'NULL'`, changing semantics (SQL NULL vs string literal). It will also
incorrectly quote function/keyword defaults if any converter returns them
unquoted.
Handle `Literals.NULL` (or a converted value equal to `NULL`
case-insensitively) as a special case and return it without quoting; likewise
avoid quoting function/keyword defaults.
```suggestion
// Special handling for SQL NULL: do not quote it, even for string-like
columns.
String trimmedDefault =
defaultValue == null ? null : defaultValue.trim();
if (defaultValueExpression == Literals.NULL
|| (trimmedDefault != null &&
"NULL".equalsIgnoreCase(trimmedDefault))) {
return "NULL";
}
if (column.dataType() instanceof
org.apache.gravitino.rel.types.Types.VarCharType
|| column.dataType() instanceof
org.apache.gravitino.rel.types.Types.StringType
|| column.dataType() instanceof
org.apache.gravitino.rel.types.Types.FixedCharType) {
if (StringUtils.isEmpty(defaultValue)) {
// Represent empty string default as two single quotes.
return "''";
}
// If the converter already produced a quoted literal, preserve it.
if (trimmedDefault != null
&& trimmedDefault.length() >= 2
&& trimmedDefault.startsWith("'")
&& trimmedDefault.endsWith("'")) {
return trimmedDefault;
}
// Avoid quoting function/keyword defaults that are intentionally
unquoted.
if (trimmedDefault != null &&
!shouldQuoteStringDefault(trimmedDefault)) {
return trimmedDefault;
}
// Fall back to quoting as a string literal.
return "'" + defaultValue + "'";
}
return defaultValue;
}
/**
* Returns whether a string default value for a string-like column should
be wrapped in single
* quotes.
*
* <p>This method is used to avoid changing semantics for SQL NULLs and
function/keyword
* defaults (for example, CURRENT_TIMESTAMP, NOW(), or UUID()) that the
converter already
* returns unquoted.
*/
private boolean shouldQuoteStringDefault(String trimmedDefault) {
if (StringUtils.isEmpty(trimmedDefault)) {
return true;
}
// Preserve explicit NULL, which is handled earlier but double-check
here.
if ("NULL".equalsIgnoreCase(trimmedDefault)) {
return false;
}
// Detect common SQL keywords or function-style expressions that should
not be quoted.
// Examples: CURRENT_TIMESTAMP, NOW(), UUID(), RANDOM_UUID().
if (trimmedDefault.equals(trimmedDefault.toUpperCase())
&& trimmedDefault.matches("[A-Z_][A-Z0-9_]*(\\s*\\(.*\\))?")) {
return false;
}
return true;
}
```
##########
catalogs/catalog-jdbc-starrocks/src/test/java/org/apache/gravitino/catalog/starrocks/operation/TestStarRocksTableOperationsSqlGeneration.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.NamedReference;
+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 TestStarRocksTableOperationsSqlGeneration {
+
+ 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 testCreateTableWithEmptyStringDefaultValue() {
+ 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();
+
+ // StarRocks requires distribution
+ Distribution distribution = Distributions.hash(1,
NamedReference.field("col1"));
+
+ String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1},
distribution);
+ Assertions.assertTrue(sql.contains("DEFAULT ''"), "Should contain DEFAULT
'' but was: " + sql);
+ }
+
+ @Test
+ public void testCreateTableWithNonEmptyStringDefaultValue() {
+ TestableStarRocksTableOperations ops = new
TestableStarRocksTableOperations();
+ String tableName = "test_table";
+ JdbcColumn col1 =
+ JdbcColumn.builder()
+ .withName("col1")
+ .withType(Types.VarCharType.of(255))
+ .withNullable(false)
+ .withDefaultValue(Literals.of("abc", Types.VarCharType.of(255)))
+ .build();
+
+ // StarRocks requires distribution
+ Distribution distribution = Distributions.hash(1,
NamedReference.field("col1"));
+
+ String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1},
distribution);
+ Assertions.assertTrue(
+ sql.contains("DEFAULT 'abc'"), "Should contain DEFAULT 'abc' but was:
" + sql);
+ }
+
+ @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,
NamedReference.field("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 unit test output noisy and can
hide real failures in CI logs. Please remove this debug print (or use the
project logger if you truly need it).
```suggestion
```
##########
catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.doris.operation;
+
+import java.util.Collections;
+import org.apache.gravitino.catalog.doris.converter.DorisTypeConverter;
+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.rel.expressions.NamedReference;
+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;
+import org.mockito.Mockito;
+
+public class TestDorisTableOperationsSqlGeneration {
+
+ private static class TestableDorisTableOperations extends
DorisTableOperations {
+ public TestableDorisTableOperations() {
+ super.exceptionMapper = new JdbcExceptionConverter();
+ super.typeConverter = new DorisTypeConverter();
+ 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 testCreateTableWithEmptyStringDefaultValue() {
+ TestableDorisTableOperations ops = new TestableDorisTableOperations();
+ 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();
+ // Doris requires distribution
+ Distribution distribution = Distributions.hash(1,
NamedReference.field("col1"));
+
+ TestableDorisTableOperations mockOps = Mockito.spy(ops);
+ Mockito.doAnswer(a -> a.getArgument(0))
+ .when(mockOps)
+ .appendNecessaryProperties(Mockito.anyMap());
Review Comment:
This test uses a Mockito spy just to bypass `appendNecessaryProperties`,
which forces adding Mockito dependencies to the module. Since
`appendNecessaryProperties` is now `@VisibleForTesting` and non-private, a
simpler approach is to override it in `TestableDorisTableOperations` (returning
the input map) instead of spying/stubbing at runtime.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -95,6 +97,28 @@ public void initialize(
this.columnDefaultValueConverter = jdbcColumnDefaultValueConverter;
}
+ protected String calculateDefaultValue(JdbcColumn column, Expression
defaultValueExpression) {
+ String defaultValue =
columnDefaultValueConverter.fromGravitino(defaultValueExpression);
+ if ((column.dataType() instanceof
org.apache.gravitino.rel.types.Types.VarCharType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.StringType
+ || column.dataType() instanceof
org.apache.gravitino.rel.types.Types.FixedCharType)) {
Review Comment:
This method uses fully qualified class names in the `instanceof` checks
(e.g., `org.apache.gravitino.rel.types.Types.VarCharType`). Per project
guidelines, prefer standard imports (or another non-FQN check, such as
comparing `column.dataType().name()` against the relevant `Type.Name` values).
##########
catalogs/catalog-jdbc-doris/build.gradle.kts:
##########
@@ -52,6 +52,8 @@ dependencies {
testImplementation(libs.awaitility)
testImplementation(libs.junit.jupiter.api)
testImplementation(libs.junit.jupiter.params)
+ testImplementation(libs.mockito.core)
+ testImplementation(libs.mockito.inline)
Review Comment:
Adding Mockito (`mockito.core`/`mockito.inline`) only to stub
`appendNecessaryProperties` in a simple SQL-generation unit test seems
avoidable. Prefer adjusting the test (e.g., override
`appendNecessaryProperties` in a test subclass) so this module doesn’t need
extra test dependencies.
```suggestion
```
--
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]
