[GitHub] [pinot] geeknarrator commented on a diff in pull request #8781: Fix Upsert config validation to check for metrics aggregation

[GitBox]

geeknarrator commented on code in PR #8781:
URL: https://github.com/apache/pinot/pull/8781#discussion_r886341586


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -521,19 +520,32 @@ static void validateUpsertConfig(TableConfig tableConfig, 
Schema schema) {
     Preconditions.checkState(streamConfig.hasLowLevelConsumerType() && 
!streamConfig.hasHighLevelConsumerType(),
         "Upsert table must use low-level streaming consumer type");
     // replica group is configured for routing
-    Preconditions.checkState(
-        tableConfig.getRoutingConfig() != null && 
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE
-            
.equalsIgnoreCase(tableConfig.getRoutingConfig().getInstanceSelectorType()),
+    Preconditions.checkState(tableConfig.getRoutingConfig() != null
+            && 
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE.equalsIgnoreCase(
+            tableConfig.getRoutingConfig().getInstanceSelectorType()),
         "Upsert table must use strict replica-group (i.e. strictReplicaGroup) 
based routing");
     // no startree index
-    Preconditions.checkState(
-        
CollectionUtils.isEmpty(tableConfig.getIndexingConfig().getStarTreeIndexConfigs())
 && !tableConfig
-            .getIndexingConfig().isEnableDefaultStarTree(), "The upsert table 
cannot have star-tree index.");
+    
Preconditions.checkState(CollectionUtils.isEmpty(tableConfig.getIndexingConfig().getStarTreeIndexConfigs())
+        && !tableConfig.getIndexingConfig().isEnableDefaultStarTree(), "The 
upsert table cannot have star-tree index.");
     // comparison column exists
     if (tableConfig.getUpsertConfig().getComparisonColumn() != null) {
       String comparisonCol = 
tableConfig.getUpsertConfig().getComparisonColumn();
       Preconditions.checkState(schema.hasColumn(comparisonCol), "The 
comparison column does not exist on schema");
     }
+    Preconditions.checkState(!isAggregateMetricsEnabled(tableConfig),
+        "Metrics aggregation and upsert cannot be enabled together");
+  }
+
+  /**
+   * Checks if Metrics aggregation is enabled.
+   */
+  @VisibleForTesting
+  static boolean isAggregateMetricsEnabled(TableConfig config) {
+    boolean isAggregateMetricsEnabledInIndexingConfig =
+        
Optional.ofNullable(config.getIndexingConfig()).map(IndexingConfig::isAggregateMetrics).orElse(false);
+    boolean hasAggregationConfigs = 
Optional.ofNullable(config.getIngestionConfig())
+        .map(ingestionConfig -> 
CollectionUtils.isNotEmpty(ingestionConfig.getAggregationConfigs())).orElse(false);

Review Comment:
   👍 sgtm



-- 
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-unsubscr...@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org