Copilot commented on code in PR #10318:
URL: https://github.com/apache/gravitino/pull/10318#discussion_r2905353516
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/converter/ClickHouseColumnDefaultValueConverter.java:
##########
@@ -59,8 +59,15 @@ public String fromGravitino(Expression defaultValue) {
Object value = literal.value();
if (value instanceof LocalDateTime) {
value = ((LocalDateTime) value).format(DATE_TIME_FORMATTER);
+ } else if (value instanceof String str
+ && str.length() >= 2
+ && str.startsWith("'")
+ && str.endsWith("'")) {
+ // The API caller may pass quoted string literals (e.g. "'active'"),
but DDL generation
+ // is responsible for adding the outer quotes.
+ value = str.substring(1, str.length() - 1);
}
- return String.format("'%s'", value);
+ return String.format("'%s'", value.toString().replace("'", "''"));
Review Comment:
`fromGravitino` now calls `value.toString()` for non-numeric literals, which
will throw a NullPointerException if a caller constructs a string literal with
a null value (e.g., `Literals.stringLiteral(null)` or `Literals.of(null,
Types.StringType.get())`). Previously this path would render `'null'` (still
arguably wrong, but non-crashing). Consider using `String.valueOf(value)` /
`Objects.toString(value, "")` (and/or explicitly rejecting null here) before
escaping.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/converter/ClickHouseColumnDefaultValueConverter.java:
##########
@@ -59,8 +59,15 @@ public String fromGravitino(Expression defaultValue) {
Object value = literal.value();
if (value instanceof LocalDateTime) {
value = ((LocalDateTime) value).format(DATE_TIME_FORMATTER);
+ } else if (value instanceof String str
+ && str.length() >= 2
+ && str.startsWith("'")
+ && str.endsWith("'")) {
+ // The API caller may pass quoted string literals (e.g. "'active'"),
but DDL generation
+ // is responsible for adding the outer quotes.
+ value = str.substring(1, str.length() - 1);
}
- return String.format("'%s'", value);
+ return String.format("'%s'", value.toString().replace("'", "''"));
Review Comment:
The new "strip outer quotes" logic treats any value like `"'... '"` as
already-quoted SQL, but it does not unescape SQL-style doubled quotes before
re-escaping. For example, an input like `"'o''reilly'"` would become `DEFAULT
'o''''reilly'`, changing the intended value. If you want to support
already-quoted SQL literals, consider unescaping doubled single-quotes after
stripping outer quotes (convert `''` -> `'`) before applying the standard
escaping step.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java:
##########
@@ -1058,6 +1058,86 @@ void
testGenerateCreateTableSqlWithSecondaryIndexAndPartition() {
Assertions.assertTrue(sql.contains("INDEX `idx_c3` `c3` TYPE bloom_filter
GRANULARITY 3"));
}
+ @Test
+ void testGenerateCreateTableSqlWithQuotedStringDefault() {
+ TestableClickHouseTableOperations ops = new
TestableClickHouseTableOperations();
+ ops.initialize(
+ null,
+ new ClickHouseExceptionConverter(),
+ new ClickHouseTypeConverter(),
+ new ClickHouseColumnDefaultValueConverter(),
+ new HashMap<>());
+
+ JdbcColumn[] cols =
+ new JdbcColumn[] {
+ JdbcColumn.builder()
+ .withName("id")
+ .withType(Types.IntegerType.get())
+ .withNullable(false)
+ .build(),
+ JdbcColumn.builder()
+ .withName("status")
+ .withType(Types.StringType.get())
+ .withNullable(false)
+ .withComment("Status")
+ .withDefaultValue(Literals.stringLiteral("'active'"))
+ .build()
+ };
+
+ String sql =
+ ops.buildCreateSql(
+ "t_status",
+ cols,
+ null,
+ new HashMap<>(),
+ Transforms.EMPTY_TRANSFORM,
+ Distributions.NONE,
+ new Index[0],
+ ClickHouseUtils.getSortOrders("id"));
+
+ Assertions.assertTrue(sql.contains("`status` String DEFAULT 'active'
COMMENT 'Status'"));
+ Assertions.assertFalse(sql.contains("DEFAULT ''active''"));
Review Comment:
This assertion is sensitive to exact whitespace formatting in the generated
SQL (note the double space between `String` and `DEFAULT`). Since the generator
intentionally adds surrounding spaces, minor formatting changes could make this
test fail without changing semantics. Consider asserting on smaller/semantic
substrings (e.g., `DEFAULT 'active'` and `COMMENT 'Status'`) or normalizing
whitespace before matching.
--
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]
