gargvishesh commented on code in PR #16291:
URL: https://github.com/apache/druid/pull/16291#discussion_r1673446225


##########
server/src/test/java/org/apache/druid/client/indexing/ClientCompactionRunnerInfoTest.java:
##########
@@ -27,117 +27,166 @@
 import org.apache.druid.indexer.partitions.HashedPartitionsSpec;
 import org.apache.druid.indexer.partitions.PartitionsSpec;
 import org.apache.druid.java.util.common.HumanReadableBytes;
+import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.java.util.common.granularity.Granularities;
 import org.apache.druid.query.aggregation.AggregatorFactory;
 import org.apache.druid.query.aggregation.LongSumAggregatorFactory;
 import org.apache.druid.segment.IndexSpec;
 import org.apache.druid.segment.data.CompressionFactory;
 import org.apache.druid.segment.data.CompressionStrategy;
 import org.apache.druid.segment.writeout.TmpFileSegmentWriteOutMediumFactory;
+import org.apache.druid.server.coordinator.CompactionConfigValidationResult;
 import org.apache.druid.server.coordinator.DataSourceCompactionConfig;
 import org.apache.druid.server.coordinator.UserCompactionTaskGranularityConfig;
 import org.apache.druid.server.coordinator.UserCompactionTaskQueryTuningConfig;
 import org.joda.time.Duration;
 import org.joda.time.Period;
+import org.junit.Assert;
 import org.junit.Test;
 
 import javax.annotation.Nullable;
 import java.util.Collections;
 import java.util.Map;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
 public class ClientCompactionRunnerInfoTest
 {
   @Test
-  public void testHashedPartitionsSpecs()
+  public void testMSQEngineWithHashedPartitionsSpecIsInvalid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new HashedPartitionsSpec(100, null, null),
         Collections.emptyMap(),
         null,
         null
     );
-    
assertFalse(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
-                                          .isValid());
+    CompactionConfigValidationResult validationResult = 
ClientCompactionRunnerInfo.validateCompactionConfig(
+        compactionConfig,
+        CompactionEngine.NATIVE
+    );
+    Assert.assertFalse(validationResult.isValid());
+    Assert.assertEquals(
+        StringUtils.format(
+            "Invalid partitionsSpec type[%s] for MSQ engine."
+            + " Type must be either 'dynamic' or 'range'.",
+            HashedPartitionsSpec.class.getSimpleName()
+        ),
+        validationResult.getReason()
+    );
   }
 
   @Test
-  public void testrInvalidDynamicPartitionsSpecs()
+  public void testMSQEngineWithMaxTotalRowsIsInvalid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new DynamicPartitionsSpec(100, 100L),
         Collections.emptyMap(),
         null,
         null
     );
-    
assertFalse(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
-                                          .isValid());
+    CompactionConfigValidationResult validationResult = 
ClientCompactionRunnerInfo.validateCompactionConfig(
+        compactionConfig,
+        CompactionEngine.NATIVE
+    );
+    Assert.assertFalse(validationResult.isValid());
+    Assert.assertEquals(StringUtils.format(
+        "maxTotalRows[%d] in DynamicPartitionsSpec not supported for MSQ 
engine.",
+        100
+    ), validationResult.getReason());
   }
 
   @Test
-  public void testDynamicPartitionsSpecs()
+  public void testMSQEngineWithDynamicPartitionsSpecIsValid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new DynamicPartitionsSpec(100, null),
         Collections.emptyMap(),
         null,
         null
     );
-    
assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
+    
Assert.assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
                                          .isValid());
   }
 
   @Test
-  public void testDimensionRangePartitionsSpecs()
+  public void testMSQEngineWithDimensionRangePartitionsSpecIsValid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new DimensionRangePartitionsSpec(100, null, 
ImmutableList.of("partitionDim"), false),
         Collections.emptyMap(),
         null,
         null
     );
-    
assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
+    
Assert.assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
                                          .isValid());
   }
 
   @Test
-  public void testQueryGranularityAll()
+  public void testMSQEngineWithQueryGranularityAllIsValid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new DynamicPartitionsSpec(3, null),
         Collections.emptyMap(),
         new UserCompactionTaskGranularityConfig(Granularities.ALL, 
Granularities.ALL, false),
         null
     );
-    
assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
+    
Assert.assertTrue(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
                                           .isValid());
   }
 
   @Test
-  public void testRollupFalseWithMetricsSpec()
+  public void testMSQEngineWithRollupFalseWithMetricsSpecIsInValid()
   {
     DataSourceCompactionConfig compactionConfig = createCompactionConfig(
         new DynamicPartitionsSpec(3, null),
         Collections.emptyMap(),
         new UserCompactionTaskGranularityConfig(null, null, false),
         new AggregatorFactory[]{new LongSumAggregatorFactory("sum", "sum")}
     );
-    
assertFalse(ClientCompactionRunnerInfo.validateCompactionConfig(compactionConfig,
 CompactionEngine.NATIVE)
-                                          .isValid());
+    CompactionConfigValidationResult validationResult = 
ClientCompactionRunnerInfo.validateCompactionConfig(
+        compactionConfig,
+        CompactionEngine.NATIVE
+    );
+    Assert.assertFalse(validationResult.isValid());
+    Assert.assertEquals(
+        "rollup in granularitySpec must be set to True if metricsSpec is 
specifed for MSQ engine.",
+        validationResult.getReason()
+    );
+  }
+
+  @Test
+  public void testMSQEngineWithUnsupportedMetricsSpecIsInValid()

Review Comment:
   Added a comment to avoid having too long a function name.



-- 
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]

Reply via email to