clintropolis commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4321709290
##########
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:
i decided to keep sealed tables skipping analysis and instead make sealed
assume all string columns are single valued strings, so tasks will fail for
rollup specs because they will be missing the array conversion. Instead, in a
follow-up I will add the ability for rollup specs to explicitly specify which
columns are multi-valued, so an operator that wishes to rollup on multi-value
strings can explicitly state this in the spec (likely through the virtual
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]
