xiangfu0 commented on code in PR #19953:
URL: https://github.com/apache/pinot/pull/19953#discussion_r4343857807
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpartition/SegmentPartitionUtils.java:
##########
@@ -175,8 +194,13 @@ public static Map<String, SegmentPartitionInfo>
extractPartitionInfoMap(String t
segment, tableNameWithType);
continue;
}
- SegmentPartitionInfo segmentPartitionInfo = new
SegmentPartitionInfo(partitionColumn,
- getPartitionFunction(columnPartitionMetadata),
getPartitions(columnPartitionMetadata));
+ PartitionFunction partitionFunction =
+ getPartitionFunction(tableNameWithType, partitionColumn, segment,
columnPartitionMetadata);
+ if (partitionFunction == null) {
Review Comment:
Nice — dropping just the unbuildable column here (rather than invalidating
the whole map) is the conservative choice:
`MultiPartitionColumnsSegmentPruner.isPartitionMatch` treats a missing column
as a match, so the segment stays routable for predicates on this column while
the other columns still prune. Matches the existing `columnPartitionMetadata ==
null` branch right above.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpartition/SegmentPartitionUtils.java:
##########
@@ -100,8 +100,27 @@ public static SegmentPartitionInfo
extractPartitionInfo(String tableNameWithType
return INVALID_PARTITION_INFO;
}
- return new SegmentPartitionInfo(partitionColumn,
getPartitionFunction(columnPartitionMetadata),
- getPartitions(columnPartitionMetadata));
+ PartitionFunction partitionFunction =
+ getPartitionFunction(tableNameWithType, partitionColumn, segment,
columnPartitionMetadata);
+ if (partitionFunction == null) {
+ return INVALID_PARTITION_INFO;
+ }
+ return new SegmentPartitionInfo(partitionColumn, partitionFunction,
getPartitions(columnPartitionMetadata));
+ }
+
+ /// Returns the shared partition function for the given metadata, or `null`
when it cannot be built (e.g. an unknown
+ /// function name written by a newer version, or a function whose plugin is
missing on this broker). The segment is
+ /// then treated as having no valid partition info, so it is never pruned,
instead of failing routing for the table.
+ @Nullable
+ private static PartitionFunction getPartitionFunction(String
tableNameWithType, String partitionColumn,
+ String segment, ColumnPartitionMetadata columnPartitionMetadata) {
+ try {
+ return getPartitionFunction(columnPartitionMetadata);
+ } catch (Exception e) {
Review Comment:
Minor / non-blocking: the broad `catch (Exception e)` is the right call here
since `PartitionFunctionFactory` can throw `IllegalArgumentException` (unknown
name) or `IllegalStateException`/other runtime errors (bad config), and you
want all of them to fall back rather than fail the table. One thing to keep in
mind: this WARN fires once per segment at load/refresh time, so a rolling
upgrade that writes an unknown function across many segments can produce a
burst of identical warnings. It matches the existing invalid-metadata logging
cadence, so fine to leave, but a throttled/aggregated log could be nicer if
this ever gets noisy.
--
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]
