[ahkcs]

Description

Adds progress.fraction_done to asynchronous Calcite PPL responses: the submit response when wait_for_completion_timeout expires, and GET /_plugins/_async_query/{id}.

{"id": "<opaque-id>", "status": "RUNNING", "progress": {"fraction_done": 0.42}, "schema": [], "datarows": [], "total": 0}

Contract (enforced by QueryJob, not the engine)

State	fraction_done
PENDING	0.0
RUNNING	finite, in [0.0, 0.95], never decreases between polls
SUCCEEDED	1.0
FAILED / CANCELLED	last running value

Synchronous responses, including async submissions that finish within the wait, keep the existing body without progress. Spark async-query responses are unchanged.

Lifecycle seam (core/job)

• QueryProgress record and QueryRunner.progress() (defaults to 0.0; called from any thread, must not block).
• QueryJob samples the runner outside its monitor, then clamps and keeps the maximum. Cancellation freezes the current value. Failure is not sampled, because a failing engine has already closed its scans, so FAILED keeps the last value observed while running. A runner that throws from progress() keeps the previous value.
• QueryJobStatus and QueryResult.Running carry the progress. Running samples it when the wait budget expires, not when the request arrives, through a cancellable timer. A job that finishes before the timer fires returns its outcome inline, success or failure.
• QueryRunner.progress() is a default method, so other engines need no change.

Engine (opensearch)

• A QueryProgressTracker is attached to PPLQueryTask only for async submissions. Synchronous queries create no tracker; their searches take the same path with no progress attached.
• Every CalciteEnumerableIndexScan position adds a source in implement() and passes it to scan(source) through the generated code, so all sources are known before the first search runs, a scan the planner shares (e.g. a self-join) gets one source per use, and no state lives on the RelNode. A plan whose root is a single scan runs without generated code; it registers through a new Scannable.scanAsRoot() default. Synchronous code generation is unchanged. The tracker holds no plan references, so a retained job does not keep its plan alive. Query progress is the mean of the source progress values.
• Each pass over a scan has its own estimate, and every tracked search is bound to its pass, so a late prefetch response cannot change a newer pass.
• OpenSearchNodeClient attaches a SearchProgressListener through a SearchRequest.createTask override (the same approach as the asynchronous-search plugin). It reuses the task propagation added in fix: Register background search tasks as children of parent #5888 for background searches.
• Progress is best-effort: an exception while tracking a search is logged and ignored, and the search runs untracked.
• Source progress uses signals that searches already return, and makes no _stats call. _stats would require indices:monitor/stats, and its index-level count would reveal approximate hidden-document volume under DLS.
• The first page of an async paged scan asks for a total hit count, on a copy of its source; later pages and all synchronous queries are unchanged. Composite counts exactly (track_total_hits=true), since its pages visit every matching document anyway. A PIT scan counts exactly only when it has no row limit, i.e. it reads every match anyway (its size limit sits above streamstats, stats, …); a PIT scan with a row limit keeps the default threshold, so its first page costs the same as a synchronous one. PIT pages read one snapshot, so one count is enough. Reviewers: this relaxes the issue's "no exact total" wording for async scans only; happy to revisit.

Source shape	Signal
Single request (search, non-composite aggregation)	Shards that finished the query phase ÷ total shards
PIT search	Hits read ÷ min(total hits, row limit)
Composite aggregation	Bucket doc_count read ÷ total hits
Paged source before its first page	Finished shards × (page size ÷ row limit), a lower bound
Multiple sources (append, join, …)	Mean of the sources
Blocking plans (sort, eventstats, …)	Source progress; the 0.95 cap reserves coordinator work

Observed progress

AsyncPPLProgressIT, polled every 20 ms (10,000-document fixture, max_result_window=10):

Query	Samples while RUNNING	Terminal
streamstats … | stats max(…) (1,000 PIT pages)	0 … 0.001, 0.045, 0.12, 0.19, 0.28, 0.36, 0.49, 0.65, 0.82, 0.95	SUCCEEDED, 1.0
Same query | append [ source=accounts | stats count() ]	0 … 0.03, 0.09, 0.18, 0.26, 0.36, 0.46, 0.49	SUCCEEDED, 1.0
fields n (root scan, 1,000 PIT pages)	0 … 0.022, 0.14, 0.24, 0.36, 0.47, 0.65, 0.77, 0.88, 0.95	SUCCEEDED, 1.0
stats count() by n (single composite page)	0 … 0.95	SUCCEEDED, 1.0
Overflowing eval after the full scan	bounded and monotonic	FAILED, ≥ last running value

Tests

Suite	Result
AsyncPPLProgressIT (new)	6 / 6
AsyncPPLQueryLifecycleIT (existing)	25 / 25
CalciteCrossClusterAsyncProgressIT (new; remote scan, remote blocking scan, remote stats by, remote single-request stats, local + remote source; async rows equal sync)	5 / 5
CalciteCrossClusterSearchIT (existing)	37 / 37
check for core, opensearch, ppl, async-query-core, async-query, and the plugin (unit tests, JaCoCo, Spotless)	pass

New unit coverage covers QueryJob progress invariants, SourceProgress math for each shape, the tracker, node-client listener attachment, the background scanner, enumerator completion, the formatter, routing, and PPLQueryRunner.

Known limitations

• A scan that is planned but never enumerated (for example, a short-circuited join side) stays at 0.0 and lowers the mean. Success still reports 1.0.
• After its first page, a composite estimate can fall below the first-page lower bound. Because the job never decreases its value, progress pauses until the scan catches up.
• A single-shard, single-page source can report only 0.0 → 0.95.
• Cross-cluster: remote PIT scans report per shard and per page. Remote single-request searches and composite pages run under ccs_minimize_roundtrips, where the remote cluster reduces its own shards, so they report only when the response arrives (0 → done; a single-page remote stats by shows 0 → 0.95). Turning minimize-roundtrips off for async only would change how CCS executes, so it is left as is.
• graphlookup traversal scans are not counted; a query that falls back to the legacy engine keeps the value Calcite reached.
• Composite progress counts bucket doc_count; a group-by on a multi-valued field counts a document once per bucket, so that source can reach the cap early.
• FAILED keeps the last value observed while running. A scan is complete only when exhausted or at its row limit, not when closed, because a failing query, or one falling back to the legacy engine, closes its scans too. A scan closed early by a limit that is not pushed down therefore under-reports until success.

Related Issues

Resolves #5797
Part of #5765

Check List

• New functionality includes testing.
• New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
• New PPL command checklist all confirmed. N/A
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ce8f7a9)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In track(), the method copies the SearchRequest but then calls pipeline(request.pipeline()) on the copy. The copy constructor already drops the pipeline (per the comment on line 63), but this line attempts to restore it. If request.pipeline() returns null (which is common), this could set the pipeline to null explicitly rather than leaving it unset, potentially changing behavior. The issue arises when the original request has no pipeline set—calling pipeline(null) may differ from not calling pipeline() at all, depending on OpenSearch's internal handling. SearchRequest tracked = new SearchRequest(request) { @Override public SearchTask createTask( long id, String type, String action, TaskId parentTaskId, Map<String, String> headers) { SearchTask task = super.createTask(id, type, action, parentTaskId, headers); task.setProgressListener(listener); return task; } }; if (firstPage) { // Copy: the scan reuses its builder for later pages. tracked.source( request.source().shallowCopy().trackTotalHitsUpTo(current.totalHitsToTrack(request))); } // The copy constructor drops the pipeline. return new Tracked(tracked.pipeline(request.pipeline()), current); Concurrency Issue In await(), a timer is created and cancelled via out.whenComplete((value, throwable) -> timer.cancel(false)). If the timer fires (completing out with running()) at nearly the same moment the completion fires (from success/failure), both callbacks may attempt to complete out. While CompletableFuture.complete() is thread-safe and only the first call wins, the running() method samples progress outside the monitor (line 173), then enters the monitor (line 175). A race exists: if the job completes between the sample and the synchronized block, running() may read stale state or compute progress from a state that no longer matches the job's actual terminal state. This could report incorrect progress in the Running result if the timer fires just as the job finishes. // Cancellable, so an early outcome does not keep the job reachable until the timeout. CompletableFuture<Void> timer = new CompletableFuture<>(); timer .completeOnTimeout(null, millis, TimeUnit.MILLISECONDS) .thenRun( () -> { if (!out.isDone()) { out.complete(running()); } }); out.whenComplete((value, throwable) -> timer.cancel(false)); } return out.minimalCompletionStage(); } private QueryResult.Running running() { double sample = sampleRunnerProgress(); synchronized (this) { double fraction = Math.min(MAX_RUNNING_FRACTION, progress(sample).fractionDone()); return new QueryResult.Running(id, new QueryProgress(fraction)); } } Possible Issue In Pass.onResponse(), the code updates totalUnits and totalExact based on the response's TotalHits. On line 161, it sets totalExact = true if total.relation() == EQUAL_TO or if !composite && total.value() >= rowLimit. However, if a later page reports a lower bound (GREATER_THAN_OR_EQUAL_TO) with a smaller value than rowLimit, the condition total.value() >= rowLimit may be false even though the total was previously considered exact. This could cause totalExact to flip from true to false across pages, violating the invariant that an exact total should remain exact. The logic assumes that once exact, it stays exact, but the condition does not enforce this. private void onResponse(SearchResponse response) { List<InternalComposite> composites = response.getAggregations() == null ? List.of() : response.getAggregations().asList().stream() .filter(InternalComposite.class::isInstance) .map(InternalComposite.class::cast) .toList(); long units; if (!composites.isEmpty()) { composite = true; units = composites.stream() .flatMap(agg -> agg.getBuckets().stream()) .mapToLong(InternalComposite.InternalBucket::getDocCount) .sum(); } else { units = response.getHits() == null ? 0L : response.getHits().getHits().length; } TotalHits total = response.getHits() == null ? null : response.getHits().getTotalHits(); // Later pages report only the default lower bound; keep the first page's total. if (total != null && !totalExact) { totalUnits = Math.max(totalUnits, total.value()); totalExact = total.relation() == TotalHits.Relation.EQUAL_TO || (!composite && total.value() >= rowLimit); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ce8f7a9

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard timeout callback against terminal states The timer.cancel(false) call may not prevent the timeout callback from running if it's already scheduled. If the job completes just before the timeout fires, both the completion and timeout handlers could race to call running(), potentially sampling progress after the job has transitioned to a terminal state. core/src/main/java/org/opensearch/sql/job/QueryJob.java [158-168] CompletableFuture<Void> timer = new CompletableFuture<>(); timer .completeOnTimeout(null, millis, TimeUnit.MILLISECONDS) .thenRun( () -> { - if (!out.isDone()) { - out.complete(running()); + synchronized (this) { + if (!out.isDone() && !state.isTerminal()) { + out.complete(running()); + } } }); out.whenComplete((value, throwable) -> timer.cancel(false)); Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a potential race condition where running() could be called after the job reaches a terminal state. However, the running() method already handles this by checking state synchronously and capping progress at MAX_RUNNING_FRACTION. The additional check would provide defense in depth but is not critical.	Low
Possible issue	Fix race condition in state check The method releases the monitor before calling runner.progress(), which means the state could transition from RUNNING to a terminal state between the check and the call. This could lead to sampling progress from a runner that has already completed or failed, potentially returning stale or incorrect values. core/src/main/java/org/opensearch/sql/job/QueryJob.java [301-313] private double sampleRunnerProgress() { + QueryJobState currentState; synchronized (this) { - if (state != QueryJobState.RUNNING) { - return 0D; - } + currentState = state; + } + if (currentState != QueryJobState.RUNNING) { + return 0D; } try { QueryProgress progress = runner.progress(); return progress == null ? 0D : progress.fractionDone(); } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential race condition, but the impact is minimal. The method already handles stale state by catching RuntimeException and returning 0D. The race window is small and the consequence is just a slightly stale progress value, not a correctness issue.	Low

---

Previous suggestions

Suggestions up to commit b605dc5

Category	Suggestion	Impact
Possible issue	Prevent division by zero in progress The estimate() method can return zero or negative values when totalUnits is -1 (initial state) and unitsRead + lastPageUnits is 0, causing division by zero or incorrect progress calculations in fractionDone(). Add validation to ensure the estimate is always positive before using it in calculations. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/SourceProgress.java [186-190] private long estimate() { long bound = totalExact ? totalUnits : Math.max(totalUnits, unitsRead + lastPageUnits); // The row limit counts hits, not composite doc counts. - return composite ? bound : Math.min(bound, rowLimit); + long result = composite ? bound : Math.min(bound, rowLimit); + return Math.max(1L, result); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue where estimate() could return zero or negative values, leading to division by zero in fractionDone(). However, the fix may not be optimal as returning Math.max(1L, result) could mask legitimate zero-result scenarios.	Medium
General	Fix race condition in progress sampling The method releases the monitor before calling runner.progress(), which means the state could transition from RUNNING to a terminal state between the check and the call. This could lead to sampling progress from a runner that has already completed or failed, potentially returning stale or incorrect values. core/src/main/java/org/opensearch/sql/job/QueryJob.java [300-313] private double sampleRunnerProgress() { + QueryJobState currentState; synchronized (this) { - if (state != QueryJobState.RUNNING) { - return 0D; - } + currentState = state; + } + if (currentState != QueryJobState.RUNNING) { + return 0D; } try { QueryProgress progress = runner.progress(); return progress == null ? 0D : progress.fractionDone(); } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a theoretical race condition, but the proposed fix doesn't actually solve it since the state could still change between reading currentState and calling runner.progress(). The current implementation is acceptable as it handles stale reads gracefully by returning 0D.	Low
	Simplify timer completion logic The timer cancellation in whenComplete happens after the completion stage fires, which means the timer's scheduled task may have already started executing. If the query completes just before the timeout, both the completion handler and the timer callback could race to call running(), potentially sampling progress twice unnecessarily. core/src/main/java/org/opensearch/sql/job/QueryJob.java [158-168] CompletableFuture<Void> timer = new CompletableFuture<>(); timer .completeOnTimeout(null, millis, TimeUnit.MILLISECONDS) .thenRun( () -> { - if (!out.isDone()) { - out.complete(running()); - } + QueryResult.Running result = running(); + out.complete(result); }); out.whenComplete((value, throwable) -> timer.cancel(false)); Suggestion importance[1-10]: 2 __ Why: The suggestion proposes removing the isDone() check, but this would cause running() to be called unconditionally even when the query has already completed, which is wasteful. The current implementation correctly avoids unnecessary work by checking isDone() first.	Low

Suggestions up to commit 18525b1

Category	Suggestion	Impact
General	Prevent progress sampling after completion The timer cancellation in whenComplete may not prevent the timeout callback from executing if it's already scheduled. This could lead to running() being called after the job completes, potentially sampling progress in a terminal state and violating the invariant that running progress stays below 1.0. core/src/main/java/org/opensearch/sql/job/QueryJob.java [154-168] if (millis <= 0L) { out.complete(running()); } else { - // Cancellable, so an early outcome does not keep the job reachable until the timeout. CompletableFuture<Void> timer = new CompletableFuture<>(); timer .completeOnTimeout(null, millis, TimeUnit.MILLISECONDS) .thenRun( () -> { - if (!out.isDone()) { - out.complete(running()); + synchronized (this) { + if (!out.isDone() && state == QueryJobState.RUNNING) { + out.complete(running()); + } } }); out.whenComplete((value, throwable) -> timer.cancel(false)); } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid edge case where the timeout callback might execute after completion. However, the running() method already handles terminal states by returning the last running value, so this is more of a defensive improvement than a critical fix.	Low
	Fix race condition in state check The method releases the monitor before calling runner.progress(), which means the state could transition from RUNNING to a terminal state between the check and the call. This creates a race condition where progress is sampled after the job has finished, potentially returning stale or incorrect values. core/src/main/java/org/opensearch/sql/job/QueryJob.java [301-313] private double sampleRunnerProgress() { + QueryJobState currentState; synchronized (this) { - if (state != QueryJobState.RUNNING) { - return 0D; - } + currentState = state; + } + if (currentState != QueryJobState.RUNNING) { + return 0D; } try { QueryProgress progress = runner.progress(); return progress == null ? 0D : progress.fractionDone(); } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential race condition, but the existing code already handles this correctly. The method is called outside the monitor intentionally (as documented), and returning 0D for non-RUNNING states is acceptable behavior. The suggested change doesn't meaningfully improve correctness.	Low

Suggestions up to commit ac83d61

Category	Suggestion	Impact
General	Handle negative total units estimate When totalUnits is -1 (initial value) and composite is true, the estimate becomes -1, which causes fractionDone() to return 0D even after reading units. Initialize totalUnits to 0L or handle the -1 case explicitly in the estimate calculation. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/SourceProgress.java [186-190] private long estimate() { long bound = totalExact ? totalUnits : Math.max(totalUnits, unitsRead + lastPageUnits); + if (bound < 0L) { + bound = unitsRead + lastPageUnits; + } // The row limit counts hits, not composite doc counts. return composite ? bound : Math.min(bound, rowLimit); } Suggestion importance[1-10]: 7 __ Why: This identifies a real issue where totalUnits starts at -1L and could cause incorrect estimates for composite aggregations before the first response. The suggested fix properly handles this edge case by ensuring bound is never negative.	Medium
	Validate state after sampling progress The method releases the monitor before calling runner.progress(), but then uses the returned value inside progress(sample) while holding the monitor. If the state transitions to terminal between the release and re-acquisition, the sampled value may be stale. Consider checking the state again after sampling and before updating runningFraction. core/src/main/java/org/opensearch/sql/job/QueryJob.java [301-313] private double sampleRunnerProgress() { synchronized (this) { if (state != QueryJobState.RUNNING) { return 0D; } } try { QueryProgress progress = runner.progress(); - return progress == null ? 0D : progress.fractionDone(); + double sample = progress == null ? 0D : progress.fractionDone(); + synchronized (this) { + return state == QueryJobState.RUNNING ? sample : 0D; + } } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a potential race condition where the state could transition between releasing and re-acquiring the monitor. However, the current implementation is acceptable because progress(sample) already handles non-RUNNING states by not updating runningFraction, and returning a slightly stale value is harmless for progress reporting.	Low
	Mark progress complete immediately on limit The queryCount >= maxResponseSize check happens before fetching the next batch, but queryCount is incremented after iterator.next(). If the last batch brings queryCount to exactly maxResponseSize, the next moveNext() call will mark complete and return false without checking if more data is available. This is correct, but the progress may not reflect the final batch until the subsequent call. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexEnumerator.java [126-161] @Override public boolean moveNext() { if (queryCount >= maxResponseSize) { markComplete(); return false; } ... if (iterator.hasNext()) { current = iterator.next(); queryCount++; + if (queryCount >= maxResponseSize) { + markComplete(); + } return true; } else { markComplete(); return false; } } Suggestion importance[1-10]: 3 __ Why: The suggestion proposes marking progress complete immediately when the limit is reached. While this would make progress reporting slightly more accurate, the current implementation is correct and the difference is negligible since markComplete() is called on the next moveNext() call anyway.	Low

Suggestions up to commit c6225d0

Category	Suggestion	Impact
General	Clarify row limit heuristic for exact total The condition total.value() >= rowLimit treats a lower-bound total that meets or exceeds the row limit as exact for PIT scans. However, if the actual total is much larger than rowLimit, this may cause the progress estimate to be inaccurate. Consider whether this heuristic is appropriate for all cases, or document the assumption that reaching the row limit is sufficient for progress purposes. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/SourceProgress.java [176-183] private void onResponse(SearchResponse response) { ... TotalHits total = response.getHits() == null ? null : response.getHits().getTotalHits(); if (total != null && !totalExact) { totalUnits = Math.max(totalUnits, total.value()); + // For PIT scans, treat the total as exact if it's truly exact or if the lower bound + // meets the row limit (since we won't read beyond rowLimit anyway) totalExact = total.relation() == TotalHits.Relation.EQUAL_TO || (!composite && total.value() >= rowLimit); } ... } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that treating total.value() >= rowLimit as exact is a heuristic that may not reflect the true total. However, this is intentional for progress estimation purposes, as documented in the class javadoc. The added comment clarifies the reasoning, which improves code maintainability without changing behavior.	Low
	Prevent stale progress sampling after termination The method releases the monitor before calling runner.progress(), which allows the job state to transition to terminal (e.g., SUCCEEDED or FAILED) after the check but before sampling. This can cause the runner to report progress after it has released resources, potentially returning misleading values. Consider re-checking the state after sampling or documenting this race condition. core/src/main/java/org/opensearch/sql/job/QueryJob.java [308-324] private double sampleRunnerProgress() { synchronized (this) { if (state != QueryJobState.RUNNING) { return 0D; } } try { QueryProgress progress = runner.progress(); - return progress == null ? 0D : progress.fractionDone(); + double sample = progress == null ? 0D : progress.fractionDone(); + synchronized (this) { + // Re-check state after sampling to avoid using stale progress from a terminated runner + return state == QueryJobState.RUNNING ? sample : 0D; + } } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential race condition where runner.progress() is called after releasing the monitor, allowing state transitions. However, the existing code already handles this by checking state before sampling and using the sample only within the monitor in progress(). The re-check after sampling adds minimal value since progress() already caps running values below 1.0.	Low
	Handle zero totalShards in progress calculation When totalShards is zero (e.g., no shards listed yet), inFlight is 0D, which can cause the first page of a paged source to report zero progress even when shards are finishing. This may underreport progress during the initial phase. Verify that totalShards is always set before finishedShards is incremented, or handle the zero case explicitly. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/SourceProgress.java [191-204] private double fractionDone() { double inFlight = totalShards > 0 ? (double) finishedShards / totalShards : 0D; if (!paged) { return responses > 0 ? 1D : inFlight; } if (responses == 0) { - return inFlight * firstPageShare; + // Ensure inFlight is valid before scaling by firstPageShare + return totalShards > 0 ? inFlight * firstPageShare : 0D; } if (composite && !totalExact) { return ratio(bucketsRead + inFlight * lastPageBuckets, rowLimit); } return ratio(unitsRead + inFlight * lastPageUnits, estimate()); } Suggestion importance[1-10]: 2 __ Why: The suggestion addresses a case where totalShards is zero, but the existing code already handles this by setting inFlight to 0D when totalShards <= 0. The proposed change adds an explicit check that is redundant with the existing ternary expression. The concern about underreporting is not valid since inFlight correctly evaluates to zero when no shards are known.	Low

Suggestions up to commit 4841c12

Category	Suggestion	Impact
Possible issue	Prevent race between state check and progress sampling The method releases the monitor before calling runner.progress(), which allows the job state to transition to terminal between the check and the call. If the runner releases resources on failure, reading progress after the state changed could access freed memory or throw. Consider sampling progress before checking the state, or accept stale samples when the race occurs. core/src/main/java/org/opensearch/sql/job/QueryJob.java [308-324] private double sampleRunnerProgress() { - synchronized (this) { - if (state != QueryJobState.RUNNING) { - return 0D; - } - } try { QueryProgress progress = runner.progress(); - return progress == null ? 0D : progress.fractionDone(); + double sample = progress == null ? 0D : progress.fractionDone(); + synchronized (this) { + return state == QueryJobState.RUNNING ? sample : 0D; + } } catch (RuntimeException e) { return 0D; } } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potential race condition where the job state could transition between the check and the runner.progress() call. However, the impact is limited since the method already handles exceptions and returns 0D for non-running states. The suggested fix samples progress before checking state, which is safer but adds minimal value given existing safeguards.	Medium

[github-actions]

Persistent review updated to latest commit ceb4b52

[github-actions]

Persistent review updated to latest commit 6b55e7f

[github-actions]

Persistent review updated to latest commit 825591a

[github-actions]

Persistent review updated to latest commit b15808c

[codecov]

Codecov Report

❌ Patch coverage is 85.34483% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.51%. Comparing base (665e1b2) to head (ce8f7a9).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...l/opensearch/executor/progress/SourceProgress.java	90.17%	2 Missing and 9 partials ⚠️
...earch/storage/scan/CalciteEnumerableIndexScan.java	0.00%	9 Missing ⚠️
...search/storage/scan/OpenSearchIndexEnumerator.java	66.66%	2 Missing and 2 partials ⚠️
...ch/sql/spark/transport/model/AsyncQueryResult.java	50.00%	2 Missing ⚠️
...src/main/java/org/opensearch/sql/job/QueryJob.java	93.75%	0 Missing and 2 partials ⚠️
...ch/sql/opensearch/client/OpenSearchNodeClient.java	88.88%	2 Missing ⚠️
...ava/org/opensearch/sql/calcite/plan/Scannable.java	0.00%	1 Missing ⚠️
...earch/sql/calcite/profile/ProfileScannableRel.java	0.00%	1 Missing ⚠️
...ensearch/sql/calcite/utils/CalciteToolsHelper.java	0.00%	1 Missing ⚠️
...search/sql/opensearch/client/OpenSearchClient.java	0.00%	1 Missing ⚠️

❌ Your project check has failed because the head coverage (75.51%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5910      +/-   ##
============================================
+ Coverage     75.38%   75.51%   +0.13%     
- Complexity    10385    10439      +54     
============================================
  Files           962      969       +7     
  Lines         41087    41306     +219     
  Branches       4682     4711      +29     
============================================
+ Hits          30973    31193     +220     
+ Misses         8806     8788      -18     
- Partials       1308     1325      +17     

Flag	Coverage Δ
sql-engine	75.51% <85.34%> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 4841c12

[github-actions]

Persistent review updated to latest commit c6225d0

[github-actions]

Persistent review updated to latest commit ac83d61

[penghuo]

Did we test cross-cluster cases

[penghuo]

unnecessary?

[ahkcs]

Agreed, reverted.

[github-actions]

Persistent review updated to latest commit 18525b1

[github-actions]

Persistent review updated to latest commit b605dc5

[github-actions]

Persistent review updated to latest commit ce8f7a9