Jackie-Jiang commented on code in PR #8781:
URL: https://github.com/apache/pinot/pull/8781#discussion_r886043582
##########
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);
+ return isAggregateMetricsEnabledInIndexingConfig || hasAggregationConfigs;
Review Comment:
Let's validate both configs cannot be enabled at the same time
##########
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:
Suggest using the non-functional apis which performs better and more readable
```suggestion
boolean isAggregateMetricsEnabledInIndexingConfig =
config.getIndexingConfig().isAggregateMetrics();
boolean hasAggregationConfigs = config.getIngestionConfig() != null &&
CollectionUtils.isNotEmpty(config.getIngestionConfig().getAggregationConfigs());
```
--
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
