Jackie-Jiang commented on code in PR #19958:
URL: https://github.com/apache/pinot/pull/19958#discussion_r4330993537
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1448,6 +1515,37 @@ static void
validateInstanceAssignmentConfigs(TableConfig tableConfig) {
"numInstancesPerPartition must be 1 when using
IMPLICIT_REALTIME_TABLE_PARTITION_SELECTOR");
}
// TODO: Add more validations for other partition selectors here
+
+ // Partition based placement of OFFLINE segments reads the partition of
a segment from its metadata
+ InstanceReplicaGroupPartitionConfig replicaGroupPartitionConfig =
+ instanceAssignmentConfig.getReplicaGroupPartitionConfig();
+ if (offlineSegmentKeys.contains(instancePartitionsType) &&
replicaGroupPartitionConfig.isReplicaGroupBased()
Review Comment:
Addressed. The check is now
`validatePartitionedSegmentAssignment(tableConfig, key)`: it rejects an
explicit non-partition-based strategy on partitioned instance partitions
instead of demanding a column that strategy would never read. The controller
check passes `OFFLINE`.
##########
pinot-sql-ddl/src/main/java/org/apache/pinot/sql/ddl/compile/PropertyMapping.java:
##########
@@ -132,7 +131,6 @@ public final class PropertyMapping {
keys.add("fieldconfigs");
Review Comment:
Addressed. The DDL property and the `TableConfigBuilder` setter are
restored, deprecated, so the compiler maps the key again and `SHOW CREATE
TABLE` emits it.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -2594,26 +2692,36 @@ private static JsonNode getTierOverwrite(@Nullable
JsonNode tierOverwrites, Stri
return tierOverwrites != null && tierOverwrites.isObject() ?
tierOverwrites.get(tier) : null;
}
- /// Get the partition column from tableConfig instance assignment config map.
- /// @param tableConfig table config
- /// @return partition column
+ /// Returns the partition column configured for segment assignment, or
`null` if none is configured. The column is
+ /// read from the segment assignment configs, falling back to the deprecated
partition column of the instance
+ /// assignment configs and then to the deprecated
`replicaGroupStrategyConfig`.
+ @Nullable
public static String getPartitionColumn(TableConfig tableConfig) {
- // check InstanceAssignmentConfigMap is null or empty,
- if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
- for (InstanceAssignmentConfig instanceAssignmentConfig :
tableConfig.getInstanceAssignmentConfigMap().values()) {
- //check InstanceAssignmentConfig has the
InstanceReplicaGroupPartitionConfig with non-empty partitionColumn
- if
(StringUtils.isNotEmpty(instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn()))
{
- return
instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn();
+ // TODO: Revisit to resolve the partition column per key (instance
partitions type or tier) instead of returning
Review Comment:
Addressed. `validateInstanceAssignmentConfigs` now rejects instance
assignment entries that configure different deprecated partition columns, so a
validated config has a single one. Per-key resolution stays a TODO.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/strategy/SegmentAssignmentStrategyFactory.java:
##########
@@ -59,7 +59,7 @@ public static SegmentAssignmentStrategy
getSegmentAssignmentStrategy(HelixManage
// Use the pre-defined segment assignment strategy
segmentAssignmentConfig =
segmentAssignmentConfigMap.get(assignmentType.toUpperCase());
// Segment assignment config is only applicable to offline tables and
completed segments of realtime tables
- if (segmentAssignmentConfig != null) {
+ if (segmentAssignmentConfig != null &&
segmentAssignmentConfig.getAssignmentStrategy() != null) {
Review Comment:
Addressed. The factory looks the entry up by its exact key, like every other
reader of the map. Noted in the Compatibility section.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -242,11 +239,6 @@ public TableConfigBuilder setSegmentPushFrequency(String
segmentPushFrequency) {
return this;
Review Comment:
The builder setter is back, deprecated. The one-argument constructor is not:
a second constructor would only keep the strategy-only form around, and the
positional form is used by tests only.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -431,26 +442,70 @@ private static void validateValidationConfig(TableConfig
tableConfig, Schema sch
validateRetentionConfig(tableConfig);
}
- private static void validateSegmentAssignmentConfig(TableConfig tableConfig)
{
+ /// Validates the segment assignment configs: the strategy must be a known
one (`allservers` for a dimension table),
+ /// and the partition column, which only applies to partition based
strategies, must be a column of the segment
+ /// partition config, the same across all entries, and the same as the
deprecated partition columns of the instance
+ /// assignment configs and `replicaGroupStrategyConfig` when those are still
configured.
+ @VisibleForTesting
+ static void validateSegmentAssignmentConfigs(TableConfig tableConfig) {
Map<String, SegmentAssignmentConfig> segmentAssignmentConfigMap =
tableConfig.getSegmentAssignmentConfigMap();
- if (segmentAssignmentConfigMap == null) {
+ if (MapUtils.isEmpty(segmentAssignmentConfigMap)) {
return;
}
- if (tableConfig.isDimTable()) {
- SegmentAssignmentConfig segmentAssignmentConfig =
-
segmentAssignmentConfigMap.get(InstancePartitionsType.OFFLINE.toString());
- if (segmentAssignmentConfig == null) {
- return;
- }
- String segmentAssignmentStrategy =
segmentAssignmentConfig.getAssignmentStrategy();
- if (segmentAssignmentStrategy != null
- &&
!segmentAssignmentStrategy.equalsIgnoreCase(AssignmentStrategy.DIM_TABLE_SEGMENT_ASSIGNMENT_STRATEGY))
{
- throw new IllegalStateException(
- String.format("Dimension table: %s can only use '%s' segment
assignment strategy, found: %s",
- tableConfig.getTableName(),
-
CommonConstants.Segment.AssignmentStrategy.DIM_TABLE_SEGMENT_ASSIGNMENT_STRATEGY,
- segmentAssignmentStrategy));
+ String tableName = tableConfig.getTableName();
+ boolean isDimTable = tableConfig.isDimTable();
+ String partitionColumn = null;
+ for (Map.Entry<String, SegmentAssignmentConfig> entry :
segmentAssignmentConfigMap.entrySet()) {
+ String key = entry.getKey();
+ SegmentAssignmentConfig segmentAssignmentConfig = entry.getValue();
+ String assignmentStrategy =
segmentAssignmentConfig.getAssignmentStrategy();
+ boolean partitionBased = !isDimTable;
Review Comment:
Leaving as is. With the strategy unset, the derived strategy depends on the
instance partitions, which validation cannot see for pre-configured ones. The
column is inert in that case, not harmful.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -431,26 +442,70 @@ private static void validateValidationConfig(TableConfig
tableConfig, Schema sch
validateRetentionConfig(tableConfig);
}
- private static void validateSegmentAssignmentConfig(TableConfig tableConfig)
{
+ /// Validates the segment assignment configs: the strategy must be a known
one (`allservers` for a dimension table),
+ /// and the partition column, which only applies to partition based
strategies, must be a column of the segment
+ /// partition config, the same across all entries, and the same as the
deprecated partition columns of the instance
+ /// assignment configs and `replicaGroupStrategyConfig` when those are still
configured.
+ @VisibleForTesting
+ static void validateSegmentAssignmentConfigs(TableConfig tableConfig) {
Map<String, SegmentAssignmentConfig> segmentAssignmentConfigMap =
tableConfig.getSegmentAssignmentConfigMap();
- if (segmentAssignmentConfigMap == null) {
+ if (MapUtils.isEmpty(segmentAssignmentConfigMap)) {
return;
}
- if (tableConfig.isDimTable()) {
- SegmentAssignmentConfig segmentAssignmentConfig =
-
segmentAssignmentConfigMap.get(InstancePartitionsType.OFFLINE.toString());
- if (segmentAssignmentConfig == null) {
- return;
- }
- String segmentAssignmentStrategy =
segmentAssignmentConfig.getAssignmentStrategy();
- if (segmentAssignmentStrategy != null
- &&
!segmentAssignmentStrategy.equalsIgnoreCase(AssignmentStrategy.DIM_TABLE_SEGMENT_ASSIGNMENT_STRATEGY))
{
- throw new IllegalStateException(
- String.format("Dimension table: %s can only use '%s' segment
assignment strategy, found: %s",
- tableConfig.getTableName(),
-
CommonConstants.Segment.AssignmentStrategy.DIM_TABLE_SEGMENT_ASSIGNMENT_STRATEGY,
- segmentAssignmentStrategy));
+ String tableName = tableConfig.getTableName();
+ boolean isDimTable = tableConfig.isDimTable();
+ String partitionColumn = null;
+ for (Map.Entry<String, SegmentAssignmentConfig> entry :
segmentAssignmentConfigMap.entrySet()) {
+ String key = entry.getKey();
+ SegmentAssignmentConfig segmentAssignmentConfig = entry.getValue();
+ String assignmentStrategy =
segmentAssignmentConfig.getAssignmentStrategy();
+ boolean partitionBased = !isDimTable;
+ if (assignmentStrategy != null) {
+ assignmentStrategy = assignmentStrategy.toLowerCase();
Review Comment:
Addressed, in both places.
--
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]
