[GitHub] [druid] suneet-s commented on a change in pull request #11874: Support changing dimension schema in Auto Compaction

[GitBox <gi...@apache.org>]

suneet-s commented on a change in pull request #11874:
URL: https://github.com/apache/druid/pull/11874#discussion_r743839200



##########
File path: docs/configuration/index.md
##########
@@ -988,7 +989,7 @@ The below is a list of the supported configurations for auto compaction.
 |`chatHandlerNumRetries`|Retries for reporting the pushed segments in worker tasks.|no (default = 5)|
 
 ###### Automatic compaction granularitySpec
-You can optionally use the `granularitySpec` object to configure the segment granularity of the compacted segments.
+You can optionally use the `granularitySpec` object to configure the granularity and rollup of the compacted segments.

Review comment:
       nit: These fields are defined below, so it seems redundant to mention them again here. 

##########
File path: core/src/test/java/org/apache/druid/timeline/DataSegmentTest.java
##########
@@ -120,6 +121,7 @@ public void testV1Serialization() throws Exception
         new NumberedShardSpec(3, 0),
         new CompactionState(
             new HashedPartitionsSpec(100000, null, ImmutableList.of("dim1")),
+            new DimensionsSpec(DimensionsSpec.getDefaultSchemas(ImmutableList.of("dim1", "bar", "foo")), null, null),

Review comment:
       Can we add a test for de-serialization of a CompactionState object that does not have the `dimensionsSpec` field

##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -440,6 +442,19 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
       }
     }
 
+    if (config.getDimensionsSpec() != null) {
+      final DimensionsSpec existingDimensionsSpec = lastCompactionState.getDimensionsSpec();
+      // Checks for list of dimensions
+      if (config.getDimensionsSpec().getDimensions() != null) {
+        final List<DimensionSchema> existingDimensions = existingDimensionsSpec != null ?
+                                                         existingDimensionsSpec.getDimensions() :
+                                                         null;
+        if (!config.getDimensionsSpec().getDimensions().equals(existingDimensions)) {
+          return true;

Review comment:
       nit: Please add a log line here for operators to know why the segment was selected for compaction similar to segmentGranularity above.
   
   Similar comments for rollup + queryGranularity

##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -440,6 +442,19 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
       }
     }
 
+    if (config.getDimensionsSpec() != null) {
+      final DimensionsSpec existingDimensionsSpec = lastCompactionState.getDimensionsSpec();
+      // Checks for list of dimensions
+      if (config.getDimensionsSpec().getDimensions() != null) {

Review comment:
       If `config.getDimensionsSpec().getDimensions()` == null and lastCompactionState.getDimensionsSpec() is non-null then we want needsCompaction to return false. Is that understanding correct?




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org