jiangxt2 commented on code in PR #11806:
URL: https://github.com/apache/gravitino/pull/11806#discussion_r3611737637
##########
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:
Followed up as promised. After evaluating several restructuring approaches
(fetching columns then passing down, internal property markers, ThreadLocal),
they all introduce either implicit state or side effects in the pure generation
path.
My suggestion is to keep the current structure — the split is intentional
separation of concerns: `create()` as the orchestration layer handles
IO-dependent pre-validation, while `handleDistributeTable` handles validation
when columns are already available.
If the base class evolves to support a `Connection`-aware generation path in
the future, we can consolidate both checks into `handleDistributeTable` at that
point.
What do you think?
--
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]
