FrankChen021 commented on code in PR #20379:
URL: https://github.com/apache/druid/pull/20379#discussion_r4294689994
##########
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] Catalog query-granularity changes are ignored
**Finding:** Catalog base-table metadata stores query granularity in the
baseTable virtual-column carrier, but
CatalogDataSourceCompactionConfig.getGranularitySpec() returns a null query
granularity. This comparison strips the carrier, while the separate
query-granularity check treats a null configured value as complete. Changing a
catalog table's query granularity can therefore leave existing segments marked
compacted and skip the required rewrite.
**Suggestion:** Expose the catalog base-table query granularity to the
status check, or compare the carrier explicitly before applying the relaxed
schema comparison; add a regression test for changing it.
--
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]
