xiangfu0 commented on code in PR #19537:
URL: https://github.com/apache/pinot/pull/19537#discussion_r4240979023
##########
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) {
Review Comment:
**[CRITICAL] Optional segments never reach the server:
`WorkerMetadata._optionalTableSegmentsMap` is not serialized.**
`QueryPlanSerDeUtils.toProtoWorkerMetadata`/`fromProtoWorkerMetadata` only
carry `tableSegmentsMap` and `logicalTableSegmentsMap`; this new field is
neither written to the proto nor to the legacy JSON custom properties. Since
MSE always dispatches the stage plan over gRPC (even to a colocated server),
`getOptionalTableSegmentsMap()` is always `null` in `ServerPlanRequestUtils`,
so `InstanceRequest.optionalSegments` is never set and the whole feature is a
silent no-op in any real deployment. The new tests only inspect broker-side
objects, which is why they pass. The field needs to be added to the
`Worker.WorkerMetadata` proto (or custom properties for mixed-version compat)
and round-tripped in `QueryPlanSerDeUtils`.
--
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]
