FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4283266647
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -649,9 +649,16 @@ private CompactionStatus
baseTableIsUpToDate(CompactionState lastCompactionState
{
// The baseTable spec compares its own state; segment/query granularity
and rollup are covered by their own
// checks, so the spec excludes them. A null configured baseTable means
"don't care".
+ //
+ // When the config is not sealed, the compaction task appends the
columns it finds in the segments to the spec,
+ // so the recorded state is a superset of the configured one and a
strict comparison would recompact forever.
+ // The relaxed comparison accepts that superset. The cost is that the
config-derived fingerprint never matches
+ // either, so these datasources always fall through to this
field-by-field pass instead of the fingerprint fast
+ // path; recording the declared spec instead is not an option, since it
would misdescribe the segment.
final BaseTableProjectionSpec configured =
compactionConfig.getBaseTable();
final BaseTableProjectionSpec current =
lastCompactionState.getBaseTable();
- if (configured == null || (current != null &&
configured.hasEqualCompactionState(current))) {
+ if (configured == null
+ || (current != null && configured.hasEqualCompactionState(current,
!compactionConfig.isSealed()))) {
Review Comment:
[P2] Schedule sealed cleanup for undeclared projections
**Finding:** The sealed-aware base-table comparison leaves the projection
check's null-as-don't-care behavior unchanged. For a catalog base table with
sealed=true and no projections property, existing segment projections can
therefore be treated as up to date, so the compaction needed to remove
undeclared projections is never scheduled.
**Suggestion:** For sealed catalog configs with an absent projections
property, compare the recorded projections against an explicit empty list while
preserving null-as-don't-care for unsealed and legacy configs, and add a status
regression test.
##########
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;
+ BaseTableProjectionSpec baseTableSpec =
baseTable.withQueryGranularity(granularitySpec.getQueryGranularity());
+ List<AggregateProjectionSpec> projectionSpecs = projections;
+ Set<String> multiValuedDimensions = null;
+ if (analyzeSchema || analyzeMultiValued) {
+ final ExistingSegmentAnalyzer analyzer = new ExistingSegmentAnalyzer(
+ segments,
+ false,
+ false,
+ analyzeSchema,
+ analyzeSchema,
+ analyzeProjections,
+ analyzeMultiValued
+ );
+ fetchAndProcess(analyzer, emitter, metricBuilder);
+ if (analyzeSchema) {
+ // An unsealed spec is partial, so append what it does not declare
after the declared columns, leaving the
+ // shape the operator asked for unchanged.
+ baseTableSpec = baseTableSpec.withAdditionalColumns(
+ undeclaredColumns(baseTableSpec, analyzer.getDimensionsSpec())
+ );
+ if (baseTableSpec instanceof RollupBaseTableProjectionSpec) {
+ baseTableSpec = ((RollupBaseTableProjectionSpec)
baseTableSpec).withAdditionalMetrics(
+ undeclaredMetrics(baseTableSpec, analyzer.getMetricsSpec())
+ );
+ } else {
+ // A spec with no metric columns has nowhere to put these, and
dropping them would delete data the table
+ // never said it did not want. Sealing the table is how an
operator asks for that.
+ final Set<String> metricNames = analyzer.getMetricNames();
+ if (!metricNames.isEmpty()) {
+ throw InvalidInput.exception(
+ "Segments of dataSource[%s] store metric column(s) %s, but
its base table has no metric columns to"
+ + " keep them in. Declare a rollup base table, or seal the
table to drop them.",
+ dataSource,
+ metricNames
+ );
+ }
+ }
+ }
+ if (analyzeProjections) {
+ projectionSpecs = analyzer.getProjections();
Review Comment:
[P2] Preserve projections across input segments
**Finding:** When an unsealed base-table compaction leaves projections
undeclared, this branch consumes ExistingSegmentAnalyzer.getProjections(). That
analyzer resets its projection map for every segment with non-empty projection
metadata, so inputs with disjoint projection names retain only the last
processed segment's definitions and silently drop the others.
**Suggestion:** Accumulate projection definitions across all segments while
retaining the intended same-name replacement behavior, and add a regression
test with disjoint projection sets.
--
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]
