FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4302706567
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1054,15 +1199,39 @@ 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.
+ for (QueryableIndex columnIndex : columnIndexes(index)) {
+ processDimensionsSpec(columnIndex);
+ processMultiValuedDimensions(columnIndex);
+ }
processMetricsSpec(index);
- processMultiValuedDimensions(index);
processProjections(index);
}
}
}
}
+ /**
+ * The indexes 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.
+ * <p>
+ * Every group has to be read rather than taking the first as
representative: a group omits a column's files
+ * entirely when it holds no values for that column, so a column can be
absent from one group and present in a
+ * later one.
+ */
+ private static Iterable<QueryableIndex> columnIndexes(QueryableIndex index)
+ {
+ final ClusteredValueGroupsBaseTableSchema clusteredBaseTable =
index.getMetadata().getClusteredBaseTable();
+ if (clusteredBaseTable == null ||
clusteredBaseTable.getClusterGroups().isEmpty()) {
+ return Collections.singletonList(index);
+ }
+ return Iterables.transform(
+ clusteredBaseTable.getClusterGroups(),
+ group -> index.getClusterGroupQueryableIndex(group, true)
Review Comment:
[P1] Cluster-group indexes hide clustering columns from inference
**Finding:** For unsealed base-table compaction, the analyzer reads each
cluster-group sub-index. `getClusterGroupQueryableIndex(..., true)` injects
clustering columns as constant holders, but the returned index deliberately
exposes only non-clustering dimensions and ordering.
`ExistingSegmentAnalyzer.processDimensionsSpec` therefore cannot discover an
old clustering column that the current catalog layout no longer declares, so
`undeclaredColumns` omits it and the rewrite silently drops stored data.
**Suggestion:** Expose the segment-level clustering columns and ordering to
schema inference, or merge that metadata into the inferred dimensions before
computing undeclared columns and multi-valued-dimension state.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -797,16 +831,66 @@ private static DataSchema createDataSchema(
@Nonnull ClientCompactionTaskGranularitySpec granularitySpec,
@Nullable List<AggregateProjectionSpec> projections,
@Nullable BaseTableProjectionSpec baseTable,
+ boolean sealed,
+ boolean rangePartitioned,
boolean needMultiValuedColumns
)
{
if (baseTable != null) {
// BaseTable mode: the spec owns dimensions/metrics and (via its
query-granularity virtual column) query
// granularity; segment granularity + intervals are the compaction
config's. Attach the config's query
// granularity up front (a no-op if absent/NONE); the generator's
incremental index applies it at build time.
- // Existing-segment analysis (dims/metrics/rollup inference) is bypassed
— the spec is authoritative.
- final BaseTableProjectionSpec baseTableSpec =
-
baseTable.withQueryGranularity(granularitySpec.getQueryGranularity());
+ // A sealed spec is the complete description of the table, so the
columns and projections it leaves undeclared
+ // are taken to not exist rather than inferred. Range partitioning is
the exception that reads the segments even
+ // when sealed: only they can say whether a partition column is
multi-valued, which MSQ cannot partition by.
+ final boolean analyzeSchema = !sealed;
+ final boolean analyzeProjections = !sealed && projections == null;
+ final boolean analyzeMultiValued = rangePartitioned;
Review Comment:
[P2] Rollup base tables skip MVD analysis without range partitioning
**Finding:** A catalog `rollupTable` base table with dynamic partitioning
has `rangePartitioned == false`, so this branch never analyzes multi-valued
dimensions even though `identifyMultiValuedDimensions()` requests that
information for a base-table rollup task. `CombinedDataSchema` consequently
leaves the MVD set null, and MSQ treats every string grouping column as
potentially multi-valued, wrapping scalar strings with `mv_to_array` and
converting them back with `array_to_mv`, which can change storage and
multi-value/null semantics.
**Suggestion:** Analyze multi-valued dimensions whenever
`needMultiValuedColumns` is true, while retaining the range-partition trigger,
and pass an empty set when analysis finds no actual multi-valued 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
