Copilot commented on code in PR #10119:
URL: https://github.com/apache/seatunnel/pull/10119#discussion_r2565485831
##########
seatunnel-connectors-v2/connector-clickhouse/src/main/java/org/apache/seatunnel/connectors/seatunnel/clickhouse/util/ClickhouseCatalogUtil.java:
##########
@@ -21,15 +21,43 @@
import org.apache.seatunnel.api.table.catalog.Column;
import org.apache.seatunnel.api.table.catalog.TablePath;
+import org.apache.seatunnel.api.table.catalog.TableSchema;
import
org.apache.seatunnel.connectors.seatunnel.clickhouse.catalog.ClickhouseTypeConverter;
import org.apache.seatunnel.connectors.seatunnel.common.util.CatalogUtil;
+import java.util.HashSet;
+import java.util.Set;
+
import static
org.apache.seatunnel.shade.com.google.common.base.Preconditions.checkNotNull;
public class ClickhouseCatalogUtil extends CatalogUtil {
+ private static final ThreadLocal<Set<String>> PRIMARY_KEY_COLUMNS =
+ ThreadLocal.withInitial(HashSet::new);
+
public static final ClickhouseCatalogUtil INSTANCE = new
ClickhouseCatalogUtil();
+ @Override
+ public String getCreateTableSql(
+ String template,
+ String database,
+ String table,
+ TableSchema tableSchema,
+ String comment,
+ String optionsKey) {
+ Set<String> pkColumns = PRIMARY_KEY_COLUMNS.get();
+ pkColumns.clear();
+ if (tableSchema.getPrimaryKey() != null) {
+ pkColumns.addAll(tableSchema.getPrimaryKey().getColumnNames());
+ }
+ try {
+ return super.getCreateTableSql(
+ template, database, table, tableSchema, comment,
optionsKey);
+ } finally {
+ pkColumns.clear();
+ }
Review Comment:
The ThreadLocal is cleared twice (lines 49 and 57), but never removed.
Consider changing the cleanup pattern to:
```java
try {
return super.getCreateTableSql(...);
} finally {
PRIMARY_KEY_COLUMNS.remove();
}
```
This ensures complete cleanup and prevents potential memory leaks in thread
pool environments where threads are reused.
##########
seatunnel-connectors-v2/connector-clickhouse/src/main/java/org/apache/seatunnel/connectors/seatunnel/clickhouse/util/ClickhouseCatalogUtil.java:
##########
@@ -49,6 +85,10 @@ public String columnToConnectorType(Column column) {
+ "'");
}
+ private static boolean isUnsupportedNullableType(String columnType) {
+ return columnType.startsWith("Map(") ||
columnType.startsWith("Array(");
+ }
Review Comment:
Missing test coverage for the `isUnsupportedNullableType` logic. Consider
adding tests to verify that:
1. Columns with `Map(String, String)` type are NOT wrapped with `Nullable()`
even when `isNullable()` returns true
2. Columns with `Array(String)` type are NOT wrapped with `Nullable()` even
when `isNullable()` returns true
This would ensure the logic at line 73 correctly prevents wrapping Map and
Array types with Nullable.
##########
seatunnel-connectors-v2/connector-clickhouse/src/main/java/org/apache/seatunnel/connectors/seatunnel/clickhouse/util/ClickhouseCatalogUtil.java:
##########
@@ -38,6 +66,14 @@ public String columnToConnectorType(Column column) {
} else {
columnType =
ClickhouseTypeConverter.INSTANCE.reconvert(column).getColumnType();
}
+
+ Set<String> pkColumns = PRIMARY_KEY_COLUMNS.get();
+ boolean isPrimaryKeyColumn = pkColumns != null &&
pkColumns.contains(column.getName());
+
+ if (column.isNullable() && !isUnsupportedNullableType(columnType) &&
!isPrimaryKeyColumn) {
+ columnType = "Nullable(" + columnType + ")";
+ }
Review Comment:
Missing test coverage for primary key column behavior. The unit tests in
ClickhouseCatalogUtilTest only test `columnToConnectorType` in isolation, but
don't verify that primary key columns are NOT wrapped with `Nullable()` even
when `isNullable()` returns true. Consider adding an integration test that
calls `getCreateTableSql` with a schema containing nullable primary key columns
to ensure they are not wrapped with `Nullable()`.
--
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]
