jiangxt2 commented on code in PR #11806:
URL: https://github.com/apache/gravitino/pull/11806#discussion_r3602417542
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -112,6 +121,98 @@ ARRAY JOIN arrayZip(splitByChar(',', primary_key),
arrayEnumerate(splitByChar(',
ORDER BY COLUMN_NAME
""";
+ @Override
+ public void create(
+ String databaseName,
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders)
+ throws TableAlreadyExistsException {
+ // When columns is empty (distributed table using AS remote_table), the
shard key validation
+ // in handleDistributeTable is skipped. Fetch remote table columns and
validate here.
+ if (ArrayUtils.isEmpty(columns)) {
+ Map<String, String> props =
+ MapUtils.isNotEmpty(properties) ? properties :
Collections.emptyMap();
+ String engine = props.get(GRAVITINO_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(engine) && ENGINE.DISTRIBUTED ==
ENGINE.fromString(engine)) {
+ String shardingKey = props.get(DistributedTableConstants.SHARDING_KEY);
+ String remoteDb = props.get(DistributedTableConstants.REMOTE_DATABASE);
+ String remoteTbl = props.get(DistributedTableConstants.REMOTE_TABLE);
+ if (StringUtils.isNotBlank(shardingKey)) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(remoteDb),
+ "Remote database must be specified for Distributed");
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(remoteTbl), "Remote table must be
specified for Distributed");
+ try (Connection conn = getConnection(databaseName)) {
+ JdbcColumn[] remoteCols = fetchRemoteColumns(conn, remoteDb,
remoteTbl);
+ validateShardKeyColumns(
+ remoteCols, shardingKey, "in remote table
%s.%s".formatted(remoteDb, remoteTbl));
+ } catch (SQLException e) {
+ throw exceptionMapper.toGravitinoException(e);
+ }
+ }
+ }
+ }
+ super.create(
+ databaseName,
+ tableName,
+ columns,
+ comment,
+ properties,
+ partitioning,
+ distribution,
+ indexes,
+ sortOrders);
+ }
Review Comment:
Good point. The reason it's here rather than in `handleDistributeTable` is
an architectural constraint: `generateCreateTableSql` (and its callees
including `handleDistributeTable`) is a pure SQL generation method with no
`Connection` parameter. When `columns` is empty, we need a JDBC connection to
fetch the remote table's column metadata for shard key validation.
Moving the fetch into `handleDistributeTable` would require either changing
the base class signature (breaks all catalog impls) or passing state implicitly
(hurts readability).
I agree it's not ideal to check Distributed engine in two places though. Let
me see if there's a cleaner way to restructure this — will follow up.
--
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]
