Copilot commented on code in PR #16556:
URL: https://github.com/apache/pinot/pull/16556#discussion_r2262322954
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -758,6 +765,13 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
serverBrokerRequest.getPinotQuery().getQueryOptions()
.put(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
}
+ // Optionally ignore missing segments based on broker config when not
explicitly set
+ if
(!_config.getProperty(CommonConstants.Broker.CONFIG_OF_IGNORE_MISSING_SEGMENTS,
+ CommonConstants.Broker.DEFAULT_IGNORE_MISSING_SEGMENTS)) {
+ // no-op
+ } else {
+ queryOptions.putIfAbsent(QueryOptionKey.IGNORE_MISSING_SEGMENTS,
"true");
+ }
Review Comment:
Duplicate logic from lines 749-754. This code block is identical and should
be extracted into a helper method to reduce duplication and improve
maintainability.
```suggestion
setIgnoreMissingSegmentsIfConfigured(queryOptions);
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -745,6 +745,13 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
serverBrokerRequest.getPinotQuery().getQueryOptions()
.put(QueryOptionKey.SERVER_RETURN_FINAL_RESULT, "true");
}
+ // Optionally ignore missing segments based on broker config when not
explicitly set
+ if
(!_config.getProperty(CommonConstants.Broker.CONFIG_OF_IGNORE_MISSING_SEGMENTS,
+ CommonConstants.Broker.DEFAULT_IGNORE_MISSING_SEGMENTS)) {
+ // no-op
+ } else {
Review Comment:
The logic for checking the broker config is inverted and unnecessarily
complex. Consider simplifying to `if (_config.getProperty(...)) {
queryOptions.putIfAbsent(...); }` to make the intent clearer.
```suggestion
if
(_config.getProperty(CommonConstants.Broker.CONFIG_OF_IGNORE_MISSING_SEGMENTS,
CommonConstants.Broker.DEFAULT_IGNORE_MISSING_SEGMENTS)) {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -463,7 +464,6 @@ private BrokerResponse
explain(QueryEnvironment.CompiledQuery query, long reques
QueryEnvironment.QueryPlannerResult queryPlanResult = callAsync(requestId,
query.getTextQuery(),
() -> query.explain(requestId, fragmentToPlanNode), timer);
String plan = queryPlanResult.getExplainPlan();
Review Comment:
[nitpick] The removal of the unused `tableNames` variable is good, but the
line numbers in the diff suggest this change is unrelated to the PR's main
purpose. Consider keeping unrelated changes in separate commits.
--
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]
