Copilot commented on code in PR #10231:
URL: https://github.com/apache/gravitino/pull/10231#discussion_r2887787204
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -168,13 +168,15 @@ protected String generateCreateTableSql(
// Add Create table clause
appendCreateTableClause(notNullProperties, sqlBuilder, tableName);
- // Add columns
- buildColumnsDefinition(columns, sqlBuilder);
+ // We still allow empty columns when the engine is distributed.
+ if (columns.length > 0) {
+ buildColumnsDefinition(columns, sqlBuilder);
Review Comment:
The column-list suppression is currently based only on `columns.length > 0`,
but the comment says it should only be allowed when the table engine is
`Distributed`. As written, creating a non-Distributed table with an empty
column array will generate `CREATE TABLE <name> ENGINE = ...` (and also
silently drop any provided indexes), which is invalid ClickHouse SQL and will
fail later with a less actionable error. Consider determining the engine first
and then: (1) allow empty columns only for `ENGINE.DISTRIBUTED` (emitting `AS
<remote_db>.<remote_table>`), and (2) throw an `IllegalArgumentException` if
columns are empty for any other engine (or if indexes are provided with empty
columns).
--
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]
