FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4258841676
##########
server/src/main/java/org/apache/druid/server/coordinator/CatalogDataSourceCompactionConfig.java:
##########
@@ -238,9 +316,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();
Review Comment:
[P2] Preserve undeclared cluster keys for base-table validation
**Finding:** The catalog-specific validation intentionally accepts a cluster
key absent from declared catalog columns, but the following generic MSQ
validation sees only the base-table dimensions and treats that key's missing
type as non-string. An unsealed catalog table with a baseTable layout and a
cluster key on a segment-only column is therefore rejected before compaction
can infer the column.
**Suggestion:** Make partial-schema partition validation defer unknown
cluster-key types until schema inference while continuing to reject known
non-string declared keys.
##########
processing/src/main/java/org/apache/druid/data/input/impl/RollupTableProjectionSpec.java:
##########
@@ -191,6 +191,47 @@ public boolean
hasEqualCompactionState(BaseTableProjectionSpec other)
return withoutQueryGranularity().equals(((RollupTableProjectionSpec)
other).withoutQueryGranularity());
}
+ /**
+ * Since {@link #withAdditionalColumns} appends undeclared columns after the
declared grouping columns and changes
+ * nothing else, {@code other} is this spec extended exactly when it
declares the same virtual columns and
+ * aggregators and its grouping column list starts with this one. List
equality is the zero-appended-columns case, so
+ * this subsumes {@link #hasEqualCompactionState(BaseTableProjectionSpec)}.
+ */
+ @Override
+ public boolean hasEqualCompactionState(BaseTableProjectionSpec other,
boolean allowUndeclaredColumns)
+ {
+ if (!allowUndeclaredColumns) {
+ return hasEqualCompactionState(other);
+ }
+ if (!(other instanceof RollupTableProjectionSpec)) {
+ return false;
+ }
+ final RollupTableProjectionSpec mine = withoutQueryGranularity();
+ final RollupTableProjectionSpec theirs = ((RollupTableProjectionSpec)
other).withoutQueryGranularity();
+ return mine.virtualColumns.equals(theirs.virtualColumns)
+ && Arrays.equals(mine.aggregators, theirs.aggregators)
Review Comment:
[P2] Allow appended metrics in relaxed rollup-state comparison
**Finding:** Unsealed rollup compaction appends undeclared metrics to the
recorded base-table spec, but the relaxed comparison still requires exact
aggregator-array equality. Whenever the coordinator evaluates such a state
without a matching fingerprint, it reports the interval as pending and
schedules the same compaction again, causing repeated work.
**Suggestion:** Compare declared aggregators against the recorded prefix and
allow appended metrics under the relaxed path; add a regression covering an
extra recorded metric.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1054,15 +1166,32 @@ public void fetchAndProcessIfNeeded()
processRollup(index);
processQueryGranularity(index);
- processDimensionsSpec(index);
+ // Column data is read from a different index for a clustered
segment, but rollup, query granularity,
+ // metrics and projections all come from the top-level metadata
either way.
+ final QueryableIndex columnIndex = columnIndex(index);
+ processDimensionsSpec(columnIndex);
processMetricsSpec(index);
- processMultiValuedDimensions(index);
+ processMultiValuedDimensions(columnIndex);
processProjections(index);
}
}
}
}
+ /**
+ * The index to read column data from. A clustered segment stores its rows
in per-cluster-group containers and its
+ * top-level index deliberately holds no columns of its own, so reading
the schema from it would find nothing;
+ * every group carries the same schema, so the first one answers for the
segment.
+ */
+ private static QueryableIndex columnIndex(QueryableIndex index)
+ {
+ final ClusteredValueGroupsBaseTableSchema clusteredBaseTable =
index.getMetadata().getClusteredBaseTable();
+ if (clusteredBaseTable == null ||
clusteredBaseTable.getClusterGroups().isEmpty()) {
+ return index;
+ }
+ return
index.getClusterGroupQueryableIndex(clusteredBaseTable.getClusterGroups().get(0),
true);
Review Comment:
[P1] Inspect every cluster group when inferring columns
**Finding:** A clustered segment can omit a dimension file from one group
when that group has no values while storing the column in a later group. The
analyzer selects only the first cluster-group index, so an undeclared column
present only in a later group is never appended to an unsealed base-table spec.
The MSQ scan and output schema then omit that column and compaction drops its
values.
**Suggestion:** Union column presence and schemas across every cluster-group
view, and add a regression where an undeclared column exists only in a later
group.
--
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]
