Copilot commented on code in PR #17532:
URL: https://github.com/apache/pinot/pull/17532#discussion_r2713824794
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java:
##########
@@ -254,6 +263,30 @@ public void setCustomConfig(TableCustomConfig
customConfig) {
_customConfig = customConfig;
}
+ @JsonProperty(TABLE_SAMPLERS_KEY)
+ @Nullable
+ public List<TableSamplerConfig> getTableSamplers() {
+ return _tableSamplers;
+ }
+
+ public void setTableSamplers(@Nullable List<TableSamplerConfig>
tableSamplers) {
+ _tableSamplers = sanitizeTableSamplers(tableSamplers);
+ }
+
+ @Nullable
+ private static List<TableSamplerConfig> sanitizeTableSamplers(@Nullable
List<TableSamplerConfig> tableSamplers) {
+ if (tableSamplers == null || tableSamplers.isEmpty()) {
+ return tableSamplers;
+ }
+ List<TableSamplerConfig> sanitized = new ArrayList<>(tableSamplers.size());
+ for (TableSamplerConfig config : tableSamplers) {
+ if (config != null) {
+ sanitized.add(config);
+ }
+ }
+ return sanitized;
Review Comment:
The `sanitizeTableSamplers` method removes null entries but returns the
original list when empty, which could preserve an empty list instead of
returning null consistently. Consider returning `null` when the sanitized list
is empty to maintain consistency with the null-handling pattern, or document
why empty lists should be preserved differently from null inputs.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -1087,10 +1221,35 @@ private Map<ServerInstance, SegmentsToQuery>
getServerInstanceToSegmentsMap(Stri
@Override
public List<String> getSegments(BrokerRequest brokerRequest) {
String tableNameWithType = brokerRequest.getQuerySource().getTableName();
- RoutingEntry routingEntry = _routingEntryMap.get(tableNameWithType);
+ RoutingEntry routingEntry = getRoutingEntry(brokerRequest,
tableNameWithType);
return routingEntry != null ? routingEntry.getSegments(brokerRequest) :
null;
}
+ @Nullable
+ private RoutingEntry getRoutingEntry(BrokerRequest brokerRequest, String
tableNameWithType) {
+ String samplerName = extractSamplerName(brokerRequest);
+ if (StringUtils.isNotBlank(samplerName)) {
+ Map<String, RoutingEntry> samplerEntries =
_samplerRoutingEntryMap.get(tableNameWithType);
+ RoutingEntry samplerEntry = samplerEntries != null ?
samplerEntries.get(samplerName) : null;
+ if (samplerEntry != null) {
+ return samplerEntry;
+ }
+ }
+ return _routingEntryMap.get(tableNameWithType);
+ }
+
+ @Nullable
+ private static String extractSamplerName(@Nullable BrokerRequest
brokerRequest) {
+ if (brokerRequest == null || !brokerRequest.isSetPinotQuery()) {
+ return null;
+ }
+ org.apache.pinot.common.request.PinotQuery pinotQuery =
brokerRequest.getPinotQuery();
+ if (pinotQuery == null || !pinotQuery.isSetQueryOptions()) {
Review Comment:
The method checks `brokerRequest.isSetPinotQuery()` but then also checks if
`pinotQuery == null` on line 1247. If `isSetPinotQuery()` returns true, the
subsequent null check is redundant. Consider removing the null check on line
1247 or clarifying if the Thrift `isSet` method can return true with a null
value in this context.
```suggestion
if (!pinotQuery.isSetQueryOptions()) {
```
--
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]
