xiangfu0 commented on code in PR #19958:
URL: https://github.com/apache/pinot/pull/19958#discussion_r4329946445
##########
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:
**[MAJOR] Removing the DDL key makes old `CREATE TABLE` statements silently
lose a still-honored config.**
With the `replicagroupstrategyconfig` case removed, the key no longer errors
— it falls through the generic key loop into `TableCustomConfig.customConfigs`
(line ~249). Re-running an existing DDL script with
`replicaGroupStrategyConfig='{"partitionColumn":"userUUID",...}'` now succeeds,
but `validationConfig.replicaGroupStrategyConfig` stays null and segment
assignment silently stops partitioning. Symmetrically, `PropertyExtractor` no
longer emits the key, so `SHOW CREATE TABLE` on an existing table drops a
config that still affects assignment, and re-running the generated DDL silently
loses it. Since the config is "still accepted and honored" this release, the
compile side should either keep mapping it (with a deprecation warning) or
reject it loudly — not swallow 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:
**[MAJOR] `getPartitionColumn` is map-iteration-order nondeterministic, and
this PR widens its blast radius.**
When no new-style column is set but `instanceAssignmentConfigMap` has
multiple entries (e.g. `OFFLINE` + a tier) with *different* deprecated
partition columns, nothing validates they agree (the cross-check above only
runs when a segment-assignment column exists), and this method returns
whichever entry iteration yields first. That value now feeds (a) the new
`validateSegmentAssignmentPartitionColumn` `containsKey` check — so a config
update can pass or fail depending on entry order — and (b)
`SegmentValidationUtils.validateUpsertSegmentPartitionMetadata`, which pre-PR
consulted **only** the `OFFLINE` entry and can now validate uploads against a
tier's column, rejecting pushes that previously passed. The TODO acknowledges
per-key resolution is the right design; until then, the legacy columns across
entries should at least be validated for consistency.
##########
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:
**[MAJOR] Tier-keyed strategy entries that the new validation blesses are
silently ignored by this `toUpperCase()` lookup.**
The PR's new `SegmentAssignmentConfig` Javadoc and
`validateSegmentAssignmentConfigs` test explicitly accept entries keyed by tier
name (e.g. `"hotTier"`), but `BaseSegmentAssignment` passes the raw tier name
here and `segmentAssignmentConfigMap.get(assignmentType.toUpperCase())` turns
it into `"HOTTIER"`, which never matches — so a validated per-tier
`segmentAssignmentStrategy` is silently dropped and the strategy is derived
from instance partitions instead. Meanwhile the `partitionColumn` half of the
same entry IS honored (`getPartitionColumn` scans values ignoring keys), making
the strategy half's silent no-op especially surprising. Pre-existing lookup,
but this PR is what blesses tier keys.
##########
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:
**[MINOR] Public SPI removed without a deprecation cycle.**
`TableConfigBuilder.setReplicaGroupStrategyConfig(...)` is deleted outright
and the 1-arg `SegmentAssignmentConfig(String)` constructor is replaced by the
2-arg form. pinot-spi is the surface plugins and external tooling compile
against; both changes are source/binary breaks (`NoSuchMethodError` for binary
deps) for a feature that is still "accepted and honored" this release. A
`@Deprecated(forRemoval = true)` pass-through builder setter and a delegating
1-arg constructor would preserve the migration window the PR otherwise promises
— and would also avoid the error-prone positional `new
SegmentAssignmentConfig(null, column)` / `(strategy, null)` calls (two
same-typed String params that compile fine when transposed) sprinkled through
the tests.
##########
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:
**[MINOR] Inconsistent accept/reject: `partitionColumn` is accepted when the
derived strategy is non-partition-based.**
When `assignmentStrategy` is null, `partitionBased` defaults to
`!isDimTable`, so a `partitionColumn` passes validation for a table whose
instance partitions derive `balanced` — the column is silently dead config,
exactly the class of misconfiguration this PR set out to eliminate. Had the
same user written `segmentAssignmentStrategy: "balanced"` explicitly with the
same column, it would be rejected. Consider deriving `partitionBased` from the
instance partitions (as the factory does) when the strategy is unspecified.
##########
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:
**[MINOR] Locale-sensitive `toLowerCase()`.**
`assignmentStrategy.toLowerCase()` uses the JVM default locale: under a
Turkish-type locale (`-Duser.language=tr`), `"ROUNDROBIN"` lowercases to
`roundrobın` (dotless i) and no longer matches the whitelist, so a valid config
is rejected. Use `toLowerCase(Locale.ROOT)` or
`equalsIgnoreCase`/case-insensitive set membership. (The same pattern in
`SegmentAssignmentStrategyFactory` is pre-existing, but there a mismatch meant
silent-balanced; here it becomes a hard validation failure.)
##########
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:
**[MINOR] Strategy whitelist duplicated across modules, with a
validation/runtime split-brain.**
`SEGMENT_ASSIGNMENT_STRATEGIES` /
`PARTITION_BASED_SEGMENT_ASSIGNMENT_STRATEGIES` re-encode the case labels of
`SegmentAssignmentStrategyFactory`'s switch (pinot-controller) with no
compile-time link: a strategy added to the factory but not here gets rejected
at validation; added only here, it silently runs as balanced via the factory's
`default` case (which this PR leaves lenient even though validation is now
strict — consider making the factory default throw). Also, the `containsKey`
check at line 487 duplicates the identical check and message string in
`validateSegmentAssignmentPartitionColumn` (line 1542), and
`validateInstanceAssignmentConfigs` re-runs
`validateSegmentAssignmentPartitionColumn` once per qualifying entry instead of
at most once. A canonical set next to the name constants in
`CommonConstants.Segment.AssignmentStrategy` would give the strategy list one
owner.
##########
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:
**[MINOR] Dropped null guard on `getIndexingConfig()`.**
The pre-PR code read `indexingConfig != null ?
indexingConfig.getSegmentPartitionConfig() : null`; the rewrite dereferences
`tableConfig.getIndexingConfig()` directly. `TableConfig`'s JsonCreator
requires `tableIndexConfig`, but `setIndexingConfig(null)` is public, and this
same PR's `TableConfigUtils.getColumnPartitionMap` still defensively returns
`Map.of()` on null — the two paths added/edited by one PR disagree about
nullability. A programmatically built/mutated config reaching upsert upload
validation now NPEs (HTTP 500) instead of skipping the check. The
`StringUtils.isNotEmpty` guard on the single map key below is also dead — a
column name key cannot usefully be empty, and the sibling fallback in
`validateSegmentAssignmentPartitionColumn` has no such guard.
--
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]
