Copilot commented on code in PR #18649:
URL: https://github.com/apache/pinot/pull/18649#discussion_r3628759240
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -126,8 +128,12 @@ public static OpChain compileLeafStage(
}
int numRequests = instanceRequests.size();
List<ServerQueryRequest> serverQueryRequests = new
ArrayList<>(numRequests);
+ QueryExecutionContext queryExecutionContext =
QueryThreadContext.get().getExecutionContext();
for (InstanceRequest instanceRequest : instanceRequests) {
- serverQueryRequests.add(new ServerQueryRequest(instanceRequest,
ServerMetrics.get(), queryArrivalTimeMs, true));
+ ServerQueryRequest serverQueryRequest =
+ new ServerQueryRequest(instanceRequest, ServerMetrics.get(),
queryArrivalTimeMs, true);
+ serverQueryRequest.setExecutionContext(queryExecutionContext);
+ serverQueryRequests.add(serverQueryRequest);
Review Comment:
`QueryThreadContext.get()` will throw if a thread context is not open for
the current thread. This utility method previously didn’t depend on
thread-local context, so this introduces a hard runtime dependency that can
break callers/tests that compile server plans outside an active
`QueryThreadContext`. Prefer `QueryThreadContext.getIfAvailable()` with a no-op
fallback, or pass the `QueryExecutionContext` in explicitly from the call site
that owns the context.
##########
pinot-clients/pinot-cli/src/main/java/org/apache/pinot/cli/PinotCli.java:
##########
@@ -270,16 +284,22 @@ private void runSingle(Connection conn, String sql)
private void executeAndRender(Connection conn, String sql)
throws SQLException {
- String composed = prefixSessionOptions(sql);
+ String clientQueryId = "pinotcli" +
UUID.randomUUID().toString().replace("-", "");
+ String composed = prefixSessionOptions(sql, clientQueryId);
Instant start = Instant.now();
- Progress progress = new Progress();
+ boolean progressEnabled = _progressIntervalMs > 0 &&
isInteractiveProgressEnabled();
+ Progress progress = new Progress(getControllerProgressUrl(clientQueryId),
_headers, progressEnabled);
ScheduledExecutorService scheduler =
Executors.newSingleThreadScheduledExecutor();
- ScheduledFuture<?> spinner = scheduler.scheduleAtFixedRate(() ->
progress.tick(), 0, 120, TimeUnit.MILLISECONDS);
+ ScheduledFuture<?> spinner = null;
+ if (progressEnabled) {
+ spinner = scheduler.scheduleAtFixedRate(() -> progress.tick(), 0,
_progressIntervalMs, TimeUnit.MILLISECONDS);
+ }
Review Comment:
A dedicated scheduler thread is created even when progress is disabled
(non-interactive terminal or `--progress-interval-ms=0`). This adds unnecessary
thread overhead in batch/redirected usage. Create the
`ScheduledExecutorService` only when `progressEnabled` is true, and otherwise
skip scheduler creation entirely.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java:
##########
@@ -266,6 +278,38 @@ public Set<String> getRunningQueryIds() {
return new HashSet<>(_executionContexts.keySet());
}
+ @Nullable
+ public QueryProgressStats getQueryProgressStats(String queryId) {
+ Preconditions.checkState(_executionContexts != null &&
_completedProgressStats != null,
+ "Query cancellation is not enabled on server");
+ QueryExecutionContext executionContext = _executionContexts.get(queryId);
+ return executionContext != null ? executionContext.getProgressStats()
+ : _completedProgressStats.getIfPresent(queryId);
+ }
+
+ private void retainCompletedProgressStats(String queryId,
QueryExecutionContext executionContext,
+ boolean successful) {
+ QueryProgressStats progressStats = executionContext.getProgressStats();
+ if (progressStats == null) {
+ return;
+ }
+ if (successful) {
+ progressStats = getCompletedProgressStats(progressStats);
+ }
+ _completedProgressStats.put(queryId, progressStats);
+ }
+
+ private static QueryProgressStats
getCompletedProgressStats(QueryProgressStats progressStats) {
+ long totalWorkUnits = progressStats.getTotalWorkUnits();
+ long processedWorkUnits = totalWorkUnits >= 0 ?
Math.max(progressStats.getProcessedWorkUnits(), totalWorkUnits)
+ : progressStats.getProcessedWorkUnits();
+ long totalSegmentsToProcess = progressStats.getTotalSegmentsToProcess();
+ long processedSegments = totalSegmentsToProcess >= 0
+ ? Math.max(progressStats.getProcessedSegments(),
totalSegmentsToProcess) : progressStats.getProcessedSegments();
+ return new QueryProgressStats(processedWorkUnits, totalWorkUnits,
processedSegments, totalSegmentsToProcess,
+ progressStats.isEstimated());
+ }
Review Comment:
For successful queries, this can persist “completed” progress where
`processedWorkUnits > totalWorkUnits` (and similarly for segments). While
`progressPercent` is capped at 100, UIs that display `processed/total` can show
confusing values like `12/10`. Consider clamping `processedWorkUnits` and
`processedSegments` to `total*` on success (e.g., set processed to `total` when
totals are known) so the completed snapshot is internally consistent.
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -371,15 +447,37 @@ const QueryPage = () => {
})
}
- const results = await PinotMethodUtils.getQueryResults(params);
- setResultError(results.exceptions || []);
- setResultData(results.result || { columns: [], records: [] });
- setQueryStats(results.queryStats || { columns: QUERY_STATS_COLUMNS,
records: [] });
- setOutputResult(JSON.stringify(results.data, null, 2) || '');
- setStageStats(results?.data?.stageStats || {});
- setWarnings(extractWarnings(results));
- setQueryLoader(false);
- queryExecuted.current = false;
+ let progressStopped = false;
+ let progressTimer = 0;
+ const pollQueryProgress = async () => {
+ try {
+ const response = await getClientQueryProgress(clientQueryId,
QUERY_PROGRESS_POLL_INTERVAL_MS);
+ setQueryProgress(response.data);
+ } catch (error) {
+ // The query might not be registered yet, or may already have
completed.
+ } finally {
+ if (!progressStopped) {
+ progressTimer = window.setTimeout(pollQueryProgress,
QUERY_PROGRESS_POLL_INTERVAL_MS);
+ }
+ }
+ };
+ progressTimer = window.setTimeout(pollQueryProgress,
QUERY_PROGRESS_POLL_INTERVAL_MS);
Review Comment:
The polling loop is only stopped in the `finally` block of `handleRunNow`.
If the user navigates away/unmounts the component while a query is running, the
timer can keep firing and call `setQueryProgress` on an unmounted component.
Track the timer id in a `useRef` and add a `useEffect` cleanup to stop polling
on unmount (and optionally gate state updates behind an `isMounted` ref).
--
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]
