clintropolis commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4330046881
##########
server/src/main/java/org/apache/druid/server/coordinator/CatalogDataSourceCompactionConfig.java:
##########
@@ -238,9 +336,53 @@ public AggregatorFactory[] getMetricsSpec()
@Override
public CompactionConfigValidationResult validate(ClusterCompactionConfig
clusterCompactionConfig)
{
+ final CompactionEngine effectiveEngine = engine == null ?
clusterCompactionConfig.getEngine() : engine;
+ if (effectiveEngine == CompactionEngine.MSQ) {
+ // the validation below can only check partition column types when the
config has a schema to read them from,
+ // which a catalog table without a baseTable layout does not, so check
the declared types here instead
+ final CompactionConfigValidationResult clusterKeyTypes =
validateClusterKeyTypes();
+ if (!clusterKeyTypes.isValid()) {
+ return clusterKeyTypes;
+ }
+ }
return ClientCompactionRunnerInfo.validateCompactionConfig(this,
clusterCompactionConfig.getEngine());
}
+ /**
+ * Checks that every cluster key names a declared {@code VARCHAR} column,
the only type the MSQ engine range
+ * partitions by. A key naming a column the table does not declare is left
alone: the table may still store it, and
+ * its type is not knowable from the catalog.
+ */
+ private CompactionConfigValidationResult validateClusterKeyTypes()
+ {
+ final ResolvedTable table = catalog.resolveTable(tableId);
+ final List<ClusterKeySpec> clusterKeys = getClusterKeys(table);
+ if (CollectionUtils.isNullOrEmpty(clusterKeys)) {
+ return CompactionConfigValidationResult.success();
+ }
+ final List<ColumnSpec> columns = table.spec().columns();
+ if (CollectionUtils.isNullOrEmpty(columns)) {
+ return CompactionConfigValidationResult.success();
+ }
+ for (ClusterKeySpec clusterKey : clusterKeys) {
+ final int position = CatalogUtils.findColumn(columns, clusterKey.expr());
+ if (position < 0) {
+ continue;
+ }
+ final ColumnType type = Columns.druidType(columns.get(position));
+ if (!ColumnType.STRING.equals(type)) {
Review Comment:
I think it is reasonable to enforce, but this isn't the right place to do
it, rather I think it should probably be in `ColumnSpec.validate()` or similar
whenever updating the catalog entry. Here I think actually should allow the
typeless columns, they later end up treated as string typed, so any
pre-existing table spec like this could work if we let it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
