FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4313935236
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -797,16 +832,72 @@ 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.
+ //
+ // Which string columns actually hold multiple values is likewise only
knowable from the segments, and a caller
+ // that does not get told assumes they all might: MSQ groups every
string column as an array and converts it
+ // back on write, which can rewrite a single-valued column as something
else. So the answer is produced whenever
+ // it was asked for and the segments are being read anyway. A sealed
table without range partitioning is not
+ // read, and keeps that conservative assumption rather than paying for a
download to refine it.
+ final boolean analyzeSchema = !sealed;
+ final boolean analyzeProjections = !sealed && projections == null;
+ final boolean analyzeMultiValued = rangePartitioned ||
(needMultiValuedColumns && analyzeSchema);
Review Comment:
[P2] Sealed dynamic rollup compaction rewrites scalar strings
**Finding:** For a sealed rollup base-table task using dynamic, non-range
partitioning, this condition disables multi-valued-dimension analysis because
analyzeSchema is false, even though rollup compaction needs the actual
multi-valued-dimension set. CombinedDataSchema therefore carries a null set;
MSQ treats every string grouping dimension as multi-valued and applies
mv_to_array/array_to_mv, so scalar string columns can be rewritten as one-value
multi-valued columns during compaction.
**Suggestion:** Keep multi-valued-dimension analysis independent of sealed
schema inference, or propagate exact multi-valued-dimension metadata, when
rollup requires it. Add coverage for a sealed dynamically partitioned rollup
containing a scalar string dimension.
--
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]
