Jackie-Jiang commented on code in PR #19958:
URL: https://github.com/apache/pinot/pull/19958#discussion_r4330995213
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -141,9 +141,20 @@ private TableConfigUtils() {
private static final String KINESIS_STREAM_TYPE = "kinesis";
private static final String CONSUMING_SEGMENT_TIER = "consuming";
- private static final Set<String> UPSERT_DEDUP_ALLOWED_ROUTING_STRATEGIES =
-
ImmutableSet.of(RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE,
- RoutingConfig.MULTI_STAGE_REPLICA_GROUP_SELECTOR_TYPE);
+ private static final Set<String> SEGMENT_ASSIGNMENT_STRATEGIES = Set.of(
Review Comment:
Addressed: the strategy sets live in
`CommonConstants.Segment.AssignmentStrategy` next to the names, and the column
existence check has a single home. The factory default stays lenient on
purpose: throwing there would fail existing tables at assignment time instead
of on config update.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidationUtils.java:
##########
@@ -90,46 +85,25 @@ public static void
validateUpsertSegmentPartitionMetadata(SegmentMetadata segmen
}
}
+ /// Returns the partition column configured for segment assignment, falling
back to the single column of the segment
+ /// partition config, or `null` if neither is available.
+ @Nullable
private static String getPartitionColumn(TableConfig tableConfig) {
- if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
- InstanceAssignmentConfig instanceAssignmentConfig =
-
tableConfig.getInstanceAssignmentConfigMap().get(InstancePartitionsType.OFFLINE.name());
- String partitionColumn = getPartitionColumn(instanceAssignmentConfig);
- if (StringUtils.isNotEmpty(partitionColumn)) {
- return partitionColumn;
- }
- }
-
- //noinspection deprecation
- SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
- ReplicaGroupStrategyConfig replicaGroupStrategyConfig =
- validationConfig != null ?
validationConfig.getReplicaGroupStrategyConfig() : null;
- String partitionColumn =
- replicaGroupStrategyConfig != null ?
replicaGroupStrategyConfig.getPartitionColumn() : null;
- if (StringUtils.isNotEmpty(partitionColumn)) {
+ String partitionColumn = TableConfigUtils.getPartitionColumn(tableConfig);
+ if (partitionColumn != null) {
return partitionColumn;
}
-
- IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
- SegmentPartitionConfig segmentPartitionConfig =
- indexingConfig != null ? indexingConfig.getSegmentPartitionConfig() :
null;
- Map<String, ?> columnPartitionMap =
- segmentPartitionConfig != null ?
segmentPartitionConfig.getColumnPartitionMap() : null;
- if (MapUtils.isNotEmpty(columnPartitionMap) && columnPartitionMap.size()
== 1) {
- return columnPartitionMap.keySet().iterator().next();
+ SegmentPartitionConfig segmentPartitionConfig =
tableConfig.getIndexingConfig().getSegmentPartitionConfig();
Review Comment:
`TableConfig` rejects a null indexing config in both the constructor and the
setter, so the guard was dead. The `isNotEmpty` check on the single key is
removed too.
--
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]
