Copilot commented on code in PR #16651:
URL: https://github.com/apache/pinot/pull/16651#discussion_r2289499609
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LeafOperator.java:
##########
@@ -469,10 +473,9 @@ void execute(ThreadExecutionContext parentContext) {
CountDownLatch latch = new CountDownLatch(2);
for (int i = 0; i < 2; i++) {
ServerQueryRequest request = _requests.get(i);
- int taskId = i;
futures[i] = _executorService.submit(() -> {
Review Comment:
[nitpick] The lambda expression contains a large amount of code (lines
476-514). Consider extracting this logic into a separate method to improve
readability and maintainability.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LeafOperator.java:
##########
@@ -427,37 +427,41 @@ void execute(ThreadExecutionContext parentContext) {
ServerQueryLogger queryLogger = ServerQueryLogger.getInstance();
if (_requests.size() == 1) {
ServerQueryRequest request = _requests.get(0);
- Tracing.ThreadAccountantOps.setupWorker(1, parentContext);
-
- InstanceResponseBlock instanceResponseBlock =
- _queryExecutor.execute(request, _executorService,
resultsBlockConsumer);
- if (queryLogger != null) {
- queryLogger.logQuery(request, instanceResponseBlock,
"MultistageEngine");
- }
- // Collect the execution stats
- mergeExecutionStats(instanceResponseBlock.getResponseMetadata());
- // TODO: Revisit if we should treat all exceptions as query failure.
Currently MERGE_RESPONSE_ERROR and
- // SERVER_SEGMENT_MISSING_ERROR are counted as query failure.
- Map<Integer, String> exceptions = instanceResponseBlock.getExceptions();
- if (!exceptions.isEmpty()) {
-
setErrorBlock(ErrorMseBlock.fromMap(QueryErrorCode.fromKeyMap(exceptions)));
- } else {
- // NOTE: Instance response block might contain data (not metadata
only) when all the segments are pruned.
- // Add the results block if it contains data.
- BaseResultsBlock resultsBlock =
instanceResponseBlock.getResultsBlock();
- if (resultsBlock != null && resultsBlock.getNumRows() > 0) {
- try {
- addResultsBlock(resultsBlock);
- } catch (InterruptedException e) {
- setErrorBlock(CANCELLED_BLOCK);
- } catch (TimeoutException e) {
- setErrorBlock(TIMEOUT_BLOCK);
- } catch (Exception e) {
- if (!(e instanceof EarlyTerminationException)) {
- LOGGER.warn("Failed to add results block", e);
+ // NOTE: Treat this as SSE runner (anchor) thread.
+ Tracing.ThreadAccountantOps.setupRunner(parentContext.getQueryId(),
parentContext.getWorkloadName());
+ try {
Review Comment:
[nitpick] The try block wraps a large amount of code (lines 432-462).
Consider extracting the execution logic into a separate method to improve
readability and reduce the size of the try block.
--
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]
