FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4270957621
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -797,16 +817,51 @@ private static DataSchema createDataSchema(
@Nonnull ClientCompactionTaskGranularitySpec granularitySpec,
@Nullable List<AggregateProjectionSpec> projections,
@Nullable BaseTableProjectionSpec baseTable,
+ boolean sealed,
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 declaration of the table, so
existing-segment analysis has nothing to add and is
+ // skipped entirely.
+ BaseTableProjectionSpec baseTableSpec =
baseTable.withQueryGranularity(granularitySpec.getQueryGranularity());
+ if (!sealed) {
+ // Otherwise the segments may carry columns the spec does not know
about. Find them and append them after the
+ // declared columns, leaving the shape the operator asked for
unchanged.
+ final ExistingSegmentAnalyzer analyzer = new ExistingSegmentAnalyzer(
+ segments,
+ false,
+ false,
+ true,
+ true,
+ false,
+ false
Review Comment:
[P2] Preserve multi-value detection for base-table compaction
**Finding:** The task computes needMultiValuedColumns specifically so MSQ
can reject multi-valued range-partition dimensions, but the base-table analyzer
hardcodes this flag to false. CombinedDataSchema.forBaseTable then exposes no
multi-valued dimension set, causing MSQ validation to skip the check. A catalog
string cluster key whose segments contain multi-valued values can therefore
reach an unsupported MSQ range partition instead of being rejected.
**Suggestion:** Pass the computed multi-value analysis through the
base-table schema and validate it in the MSQ runner; add coverage for a
multi-valued catalog cluster key.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -797,16 +817,51 @@ private static DataSchema createDataSchema(
@Nonnull ClientCompactionTaskGranularitySpec granularitySpec,
@Nullable List<AggregateProjectionSpec> projections,
@Nullable BaseTableProjectionSpec baseTable,
+ boolean sealed,
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 declaration of the table, so
existing-segment analysis has nothing to add and is
+ // skipped entirely.
+ BaseTableProjectionSpec baseTableSpec =
baseTable.withQueryGranularity(granularitySpec.getQueryGranularity());
+ if (!sealed) {
+ // Otherwise the segments may carry columns the spec does not know
about. Find them and append them after the
+ // declared columns, leaving the shape the operator asked for
unchanged.
+ final ExistingSegmentAnalyzer analyzer = new ExistingSegmentAnalyzer(
+ segments,
+ false,
+ false,
+ true,
+ true,
+ false,
Review Comment:
[P2] Preserve existing projections when catalog leaves them undeclared
**Finding:** When the catalog has no projections property,
CatalogDataSourceCompactionConfig.getProjections() returns null. The base-table
path does not analyze or propagate existing projection metadata, so
CombinedDataSchema carries null and MSQ destination generation normalizes it to
an empty projection list. Recompacting a segment with materialized aggregate
projections therefore drops those projections, contrary to the documented
behavior that an undeclared projection set preserves existing projections.
**Suggestion:** Analyze and carry forward existing projection specs when the
catalog projection list is absent, while treating an explicitly declared list
as authoritative.
--
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]
