Copilot commented on code in PR #9873: URL: https://github.com/apache/gravitino/pull/9873#discussion_r2762460269
########## catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.postgresql.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.postgresql.converter.PostgreSqlTypeConverter; +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 TestPostgreSqlTableOperationsSqlGeneration { + + private static class TestablePostgreSqlTableOperations extends PostgreSqlTableOperations { + public TestablePostgreSqlTableOperations() { + super.exceptionMapper = new JdbcExceptionConverter(); + super.typeConverter = new PostgreSqlTypeConverter(); + super.columnDefaultValueConverter = new JdbcColumnDefaultValueConverter(); + } + + public String createTableSql(String tableName, JdbcColumn[] columns) { + return generateCreateTableSql( + tableName, + columns, + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + } + } + + @Test + public void testCreateTableWithEmptyStringDefaultValue() { + TestablePostgreSqlTableOperations ops = new TestablePostgreSqlTableOperations(); + 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(); + + String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1}); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT '' but was: " + sql); + } Review Comment: The tests use VarCharType literals (e.g., `Literals.of("", Types.VarCharType.of(255))`), which are NOT NumericType. The fix in JdbcColumnDefaultValueConverter (lines 64-72) only applies to NumericType literals. Therefore, these tests do not actually validate the fix. VarCharType literals with empty strings will fall through to line 82 of the converter and return `''` regardless of the fix. To properly test the fix, you should create test cases using NumericType literals with empty string values, such as `Literals.of("", Types.IntegerType.get())`. ########## catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.IntegerType.get()) + .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()); + + String sql = mockOps.createTableSql(tableName, new JdbcColumn[] {col1}, distribution); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT '' but was: " + sql); + } Review Comment: The tests use VarCharType literals (e.g., `Literals.of("", Types.VarCharType.of(255))` at line 68), which are NOT NumericType. The fix in JdbcColumnDefaultValueConverter (lines 64-72) only applies to NumericType literals. Therefore, these tests do not actually validate the fix. VarCharType literals with empty strings will fall through to line 82 of the converter and return `''` regardless of the fix. To properly test the fix, you should create test cases using NumericType literals with empty string values, such as `Literals.of("", Types.IntegerType.get())` - note that while the column type at line 66 is IntegerType, the literal type at line 68 is VarCharType, and it's the literal type that determines which code path is taken in the converter. ########## catalogs/catalog-jdbc-mysql/src/test/java/org/apache/gravitino/catalog/mysql/operation/TestMysqlTableOperationsSqlGeneration.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.mysql.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.mysql.converter.MysqlTypeConverter; +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 TestMysqlTableOperationsSqlGeneration { + + private static class TestableMysqlTableOperations extends MysqlTableOperations { + public TestableMysqlTableOperations() { + super.exceptionMapper = new JdbcExceptionConverter(); + super.typeConverter = new MysqlTypeConverter(); + super.columnDefaultValueConverter = new JdbcColumnDefaultValueConverter(); + } + + public String createTableSql(String tableName, JdbcColumn[] columns) { + return generateCreateTableSql( + tableName, + columns, + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + } + } + + @Test + public void testCreateTableWithEmptyStringDefaultValue() { + TestableMysqlTableOperations ops = new TestableMysqlTableOperations(); + 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(); + + String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1}); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT '' but was: " + sql); + } Review Comment: The tests use VarCharType literals (e.g., `Literals.of("", Types.VarCharType.of(255))`), which are NOT NumericType. The fix at lines 64-72 only applies to NumericType literals. Therefore, these tests do not actually validate the fix. VarCharType literals with empty strings will fall through to line 82 (`return String.format("'%s'", literal.value())`) and return `''` regardless of the fix. To properly test the fix, you should create test cases using NumericType literals with empty string values, such as `Literals.of("", Types.IntegerType.get())`. ########## catalogs/catalog-jdbc-starrocks/src/test/java/org/apache/gravitino/catalog/starrocks/operation/TestStarRocksTableOperationsSqlGeneration.java: ########## @@ -0,0 +1,123 @@ +/* + * 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); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "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); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT value 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); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT ' ' but was: " + sql); + } Review Comment: The tests use VarCharType literals (e.g., `Literals.of("", Types.VarCharType.of(255))`), which are NOT NumericType. The fix in JdbcColumnDefaultValueConverter (lines 64-72) only applies to NumericType literals. Therefore, these tests do not actually validate the fix. VarCharType literals with empty strings will fall through to line 82 of the converter and return `''` regardless of the fix. To properly test the fix, you should create test cases using NumericType literals with empty string values, such as `Literals.of("", Types.IntegerType.get())`. ########## catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/converter/JdbcColumnDefaultValueConverter.java: ########## @@ -61,7 +62,14 @@ public String fromGravitino(Expression defaultValue) { if (defaultValue.equals(Literals.NULL)) { return NULL; } else if (type instanceof Type.NumericType) { - return literal.value().toString(); + String value = literal.value().toString(); + // It seems that literals.value().toString() can be an empty string for numeric types + // in some cases like `alter table t modify column `id` int null default '';`, in such + // case value is an empty string, we should wrap it with single quotes to avoid SQL error. Review Comment: The comment describes an invalid scenario: setting an empty string default on an INT column (`alter table t modify column `id` int null default ''`). This is semantically incorrect SQL. The comment should explain the actual bug scenario from issue #9816, which is about VARCHAR columns with empty string defaults causing SQL syntax errors. The current explanation doesn't match the actual fix or the problem being solved. ```suggestion // In some edge cases (for example, when handling VARCHAR columns with empty-string // defaults from DDL like `alter table t modify column id varchar(20) null default ''`), // literals.value().toString() can be an empty string. In this case, we must wrap the // value with single quotes to avoid generating invalid SQL syntax. ``` ########## catalogs/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseTableOperationsSqlGeneration.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.oceanbase.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.oceanbase.converter.OceanBaseTypeConverter; +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 TestOceanBaseTableOperationsSqlGeneration { + + private static class TestableOceanBaseTableOperations extends OceanBaseTableOperations { + public TestableOceanBaseTableOperations() { + super.exceptionMapper = new JdbcExceptionConverter(); + super.typeConverter = new OceanBaseTypeConverter(); + super.columnDefaultValueConverter = new JdbcColumnDefaultValueConverter(); + } + + public String createTableSql(String tableName, JdbcColumn[] columns) { + return generateCreateTableSql( + tableName, + columns, + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + } + } + + @Test + public void testCreateTableWithEmptyStringDefaultValue() { + TestableOceanBaseTableOperations ops = new TestableOceanBaseTableOperations(); + 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(); + + String sql = ops.createTableSql(tableName, new JdbcColumn[] {col1}); + JdbcColumnDefaultValueConverter converter = new JdbcColumnDefaultValueConverter(); + Assertions.assertTrue( + sql.contains("DEFAULT " + converter.fromGravitino(col1.defaultValue())), + "Should contain DEFAULT '' but was: " + sql); + } Review Comment: The tests use VarCharType literals (e.g., `Literals.of("", Types.VarCharType.of(255))`), which are NOT NumericType. The fix in JdbcColumnDefaultValueConverter (lines 64-72) only applies to NumericType literals. Therefore, these tests do not actually validate the fix. VarCharType literals with empty strings will fall through to line 82 of the converter and return `''` regardless of the fix. To properly test the fix, you should create test cases using NumericType literals with empty string values, such as `Literals.of("", Types.IntegerType.get())`. -- 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]
