xiangfu0 commented on code in PR #19537:
URL: https://github.com/apache/pinot/pull/19537#discussion_r4221910519
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerMetadata.java:
##########
@@ -155,4 +157,13 @@ private static Map<String, List<String>>
decodeSegmentsMapJson(String segmentsMa
throw new RuntimeException("Unable to deserialize segments map: " +
segmentsMapJson, e);
}
}
+
+ @Nullable
+ public Map<String, List<String>> getOptionalTableSegmentsMap() {
+ return _optionalTableSegmentsMap;
+ }
+
+ public void setOptionalTableSegmentsMap(Map<String, List<String>>
optionalTableSegmentsMap) {
+ _optionalTableSegmentsMap = optionalTableSegmentsMap;
Review Comment:
**`_optionalTableSegmentsMap` never reaches the servers.**
`QueryPlanSerDeUtils.toProtoWorkerMetadata` only encodes `tableSegmentsMap` /
`logicalTableSegmentsMap`, and `fromProtoWorkerMetadata` never restores this
field, so on any real dispatch `ServerPlanRequestUtils` sees
`getOptionalTableSegmentsMap() == null` and the optional segments are silently
dropped — the feature only works in broker-local tests. The proto encoding (and
the legacy JSON custom-property fallback for mixed-version compatibility, per
the repo guidance on wire formats) needs a corresponding optional-segments
entry, plus decode support on the server side.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1301,13 +1325,16 @@ private static void
assignTableSegmentsToWorkers(LogicalTableRouteInfo logicalTa
DispatchablePlanMetadata metadata) {
Map<ServerInstance, Map<String, List<String>>>
serverInstanceToLogicalSegmentsMap =
new HashMap<>();
+ Map<ServerInstance, Map<String, List<String>>>
serverInstanceToLogicalOptionalSegmentsMap =
Review Comment:
**Logical-table optional segments are collected but never used.**
`serverInstanceToLogicalOptionalSegmentsMap` is populated by
`transferToServerInstanceLogicalSegmentsMap`, but
`assignTableSegmentsToWorkers` never calls
`metadata.setWorkerIdToOptionalTableSegmentsMap(...)` (that setter has no
caller anywhere). As a result `DispatchablePlanContext`'s logical-optional
branch and the new `TableSegmentsInfo.setOptionalSegments` path in
`ServerPlanRequestUtils` are unreachable dead code, and optional segments for
logical tables are silently dropped.
--
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]
