[ahkcs]

Description

Async PPL responses now return the rows a query has produced so far: the submit response when wait_for_completion_timeout expires, and GET /_plugins/_async_query/{id} while the job is RUNNING.

{"id": "<opaque-id>", "status": "RUNNING", "progress": {"fraction_done": 0.42},
 "schema": [{"name": "n", "type": "int"}], "datarows": [[0], [1], [2]], "total": 3, "size": 3}

How it works

The rows a Calcite root ResultSet emits are already final rows in final order; they are never retracted. So no plan classification is needed:

• OpenSearchExecutionEngine.buildResultSet appends each converted row to a PartialRows buffer held by the async query's tracker (the same object that holds progress since [Feature][PPL] Report progress for asynchronous Calcite queries #5910). Sync queries have no tracker and are unchanged.
• A snapshot is the buffered rows plus a schema built by the same code as the final response (buildColumns, extracted). It is published only after the first row, so its schema equals the final schema. The buffer stops at the query size limit, as the final result does.
• Composite (stats ... by) pages reach the root as they are fetched, so they become visible without a composite-specific adapter.
• A step that must see all rows first (eventstats, stats without by, a sort that is not pushed down) returns rows only after it has read its whole input; a pushed-down sort streams like a scan.
• timewrap reshapes all rows at the end, so it publishes none while running.
• If execution fails or falls back to the legacy engine (any exception from the Calcite execute path), the buffered rows are discarded, so stale Calcite rows are never shown; a fallback query returns no rows until it finishes.

Every response is a complete replacement view; there is no delta protocol.

Lifecycle seam (core/job)

• QueryRunner.currentResult() (default empty), sampled outside the job monitor like progress().
• QueryJobStatus.current() and QueryResult.Running.current() carry the snapshot, present only while RUNNING.
• QueryRunner.close() (default no-op, idempotent), called once the job is terminal. The PPL runner drops its progress and rows suppliers and its cancellation hook (which holds the query task), so a job retained for keep_alive no longer holds the buffer.

Responses

• Submit and GET RUNNING bodies are now rendered by the same formatter (AsyncQueryResultResponseFormatter, with an id field set only on submit), so they cannot drift. Query-job responses are built through AsyncQueryResult.forQueryJob and marked explicitly, rather than inferred from other fields.
• Snapshots build their schema once and are reused until a row is added. They carry the warnings raised so far (e.g. PARTIAL_RESULT when a shard did not answer), as the final response does.
• A wait timeout completes on the JVM-wide CompletableFuture timer thread, so the RUNNING submit body, which can now hold thousands of rows, is formatted on the generic pool.
• A job's rows are sampled before its progress, so rows never come with a progress older than the work behind them.

Behavior changes to flag

• GET bodies of query jobs now match sync, as [Feature][PPL] Publish partial rows from root ResultSet and Composite pagination #5798 requires:
  • Type names use the PPL spec, as the sync response does: bigint, not long; int, not integer. AsyncPPLQueryLifecycleIT and AsyncPPLMultiNodeRoutingIT documented the old difference on purpose and are updated.
  • SUCCEEDED bodies now include warnings when present.
  • Spark responses are unchanged: they keep the SQL spec and have no warnings.
• GET RUNNING now includes schema, datarows, total, and size (empty when there are no rows yet). The submit RUNNING body gains size.

Observed

AsyncPPLPartialRowsIT, source=<fixture> | fields n (10,000 rows, 1,000 PIT pages), rows per RUNNING response polled every 20 ms: 0, 0, 789, 2057, 3179, 4699, 6329, 7939, 9769, then SUCCEEDED with all 10,000, equal to sync.

Tests

Suite	Result
AsyncPPLPartialRowsIT (new): scan and row-local eval/where return rows early; composite with 100-bucket pages returns rows before the last page; blocking eventstats returns rows only after reading all input; size limit 500; timewrap; failure after rows were visible	7 / 7
CalciteCrossClusterAsyncProgressIT, with a new remote partial-rows case	6 / 6
AsyncPPLProgressIT	6 / 6
AsyncPPLMultiNodeRoutingIT (asyncMultiNodeIT)	9 / 9
AsyncPPLSecurityIT	2 / 2
AsyncPPLQueryLifecycleIT	24 / 25; see below
check for core, opensearch, ppl, async-query-core, async-query, and the plugin	pass

Each AsyncPPLPartialRowsIT case asserts that the final schema and rows equal sync, and that every RUNNING response is a complete view whose rows are a prefix of the final rows, never shrink, and carry the final schema. New unit tests cover the job seam, PartialRows (including a concurrent writer/reader prefix check), the runner, routing, and the formatter.

AsyncPPLQueryLifecycleIT: one parity_* case fails per run, a different one each time. The same failure reproduces on a clean upstream/main, so it is pre-existing and not caused by this PR. With wait_for_completion_timeout=0, a fast analysis error now completes before the wait starts and is returned inline, instead of the RUNNING id the test expects. This likely changed with #5915; I'll follow up separately.

Related Issues

Resolves #5798
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 9db3b41)

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 Race Condition In status(), sampleCurrentResult() is called before acquiring the monitor, then the monitor is acquired to read state. If the job transitions to a terminal state between these two calls, runningOnly() may return stale rows from a RUNNING state even though the job is now terminal. This violates the invariant that current is present only in RUNNING state, enforced by the QueryJobStatus constructor. // Rows first, so progress covers them. Optional<QueryResult.Rows> current = sampleCurrentResult(); double sample = sampleRunnerProgress(); synchronized (this) { return new QueryJobStatus( id, state, submittedAtMillis, startedAtMillis, completedAtMillis, failure, result, progress(sample), runningOnly(current)); } Possible Issue In buildResultSet, if partialRows is non-null but CalcitePlanContext.warningCount() throws an exception (or peekWarnings() does), the method continues without updating warningsPublished. On the next iteration, it will call peekWarnings() again, potentially duplicating warnings in the partial rows buffer. If the exception is transient, warnings may be published multiple times. // Warnings first, so a row is never shown without them. if (CalcitePlanContext.warningCount() != warningsPublished) { warningsPublished = CalcitePlanContext.warningCount(); partialRows.warnings(CalcitePlanContext.peekWarnings()); } partialRows.add(tuple); Possible Issue In snapshot(), the check last.rows().size() != rows.size() rebuilds the snapshot only when the row count changes. If warnings() is called to update warnings without adding a row, last is set to null (line 42), but the next snapshot() call will see last == null and rebuild. However, if warnings change and then a row is added before the next snapshot, the size check passes and the stale last (with old warnings) is returned. The warnings update on line 42 sets last = null, but this is only effective if no row is added before the next snapshot. public synchronized Optional<QueryResult.Rows> snapshot() { if (schemaOf == null || rows.isEmpty()) { return Optional.empty(); } if (last == null || last.rows().size() != rows.size()) { List<ExprValue> copy = List.copyOf(rows); if (schema == null) { // Only the first row decides runtime-typed columns. schema = schemaOf.apply(copy); } last = new QueryResult.Rows(schema, copy, Cursor.None, warnings, 0); } return Optional.of(last); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9db3b41

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle race condition in state check The method releases the monitor before calling runner.currentResult(), allowing the state to transition from RUNNING to terminal between the check and the call. If the runner is closed after the state check but before currentResult() is invoked, the call may access invalid state. Consider checking the state again after obtaining the result or handling this race condition explicitly. core/src/main/java/org/opensearch/sql/job/QueryJob.java [315-328] private Optional<QueryResult.Rows> sampleCurrentResult() { synchronized (this) { if (state != QueryJobState.RUNNING) { return Optional.empty(); } } try { Optional<QueryResult.Rows> current = runner.currentResult(); + synchronized (this) { + if (state != QueryJobState.RUNNING) { + return Optional.empty(); + } + } return current == null ? Optional.empty() : current; } catch (RuntimeException e) { LOG.debug("Failed to read the rows of a running query job", e); return Optional.empty(); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition where the state could transition from RUNNING to terminal between the check and the currentResult() call. Adding a second state check after obtaining the result would prevent returning stale data from a closed runner.	Medium
	Broaden exception handling for thread pool The catch block only handles RuntimeException when submitting the respondRunning task to the thread pool. If the thread pool is shut down or in an invalid state, it may throw RejectedExecutionException (a RuntimeException), but other Error types could also occur. The fallback response is sent synchronously, which could block the timer thread. Consider handling this more robustly or documenting the assumption. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [369-380] case org.opensearch.sql.job.QueryResult.Running running -> { // Wait timeouts complete on the JVM-wide timer thread; format elsewhere. try { clientRef .threadPool() .generic() .execute(() -> respondRunning(running, ctxListener)); - } catch (RuntimeException e) { + } catch (Exception e) { + LOG.debug("Failed to submit running response formatting task", e); ctxListener.onResponse( new TransportPPLQueryResponse(formatRunning(running, false))); } } Suggestion importance[1-10]: 6 __ Why: The suggestion to catch Exception instead of just RuntimeException and add logging is reasonable for robustness. However, RejectedExecutionException is already a RuntimeException, so the functional change is mainly about catching other potential exceptions and adding logging.	Low
	Log exceptions during runner cleanup The safeCloseRunner() method silently ignores all RuntimeExceptions during runner cleanup. This could hide critical resource leaks or cleanup failures. Consider logging the exception at debug or warn level to aid troubleshooting while maintaining the best-effort semantics. core/src/main/java/org/opensearch/sql/job/QueryJob.java [359-365] private void safeCloseRunner() { try { runner.close(); - } catch (RuntimeException ignored) { - // Best-effort. + } catch (RuntimeException e) { + LOG.debug("Failed to close query runner", e); } } Suggestion importance[1-10]: 5 __ Why: Logging the exception would help with debugging resource cleanup issues while maintaining best-effort semantics. However, since the method is already documented as "best-effort" and the exception is intentionally ignored, this is a minor improvement.	Low
	Add logging for non-SQL exceptions The catch block for RuntimeException | Error calls discardPartialRows() but doesn't wrap or log the exception before rethrowing. This asymmetry with the SQLException handler could make debugging harder. Consider logging the exception or ensuring consistent error handling across both paths. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [344-363] } catch (SQLException e) { discardPartialRows(); if (isPitContextLimitReached(e)) { ... } throw new RuntimeException(e); } catch (RuntimeException | Error e) { discardPartialRows(); + LOG.debug("Query execution failed, partial rows discarded", e); throw e; } Suggestion importance[1-10]: 4 __ Why: Adding logging for consistency is a minor improvement. The asymmetry between the two catch blocks is noted, but since discardPartialRows() is called in both cases and the exception is rethrown, the functional impact is minimal.	Low

---

Previous suggestions

Suggestions up to commit da78e9d

Category	Suggestion	Impact
General	Prevent duplicate discard on SQLException The SQLException handler wraps the exception in RuntimeException after calling discardPartialRows(), but the second catch block also calls discardPartialRows() and will catch that wrapped exception. This causes discardPartialRows() to be invoked twice for SQL exceptions. Remove the duplicate call by handling discard only in the second catch block. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [344-363] } catch (SQLException e) { - discardPartialRows(); if (isPitContextLimitReached(e)) { ... } throw new RuntimeException(e); } catch (RuntimeException | Error e) { discardPartialRows(); throw e; } Suggestion importance[1-10]: 8 __ Why: Valid issue. The SQLException is wrapped in RuntimeException and then caught by the second catch block, causing discardPartialRows() to be called twice. This is inefficient and could lead to unexpected behavior if the discard operation is not idempotent.	Medium
	Prevent potential double listener invocation If execute() throws (e.g., thread pool rejection), the catch block calls ctxListener.onResponse() directly. However, respondRunning() also calls ctxListener.onResponse() when it succeeds. If the exception occurs after the task is queued but before it runs, both paths may invoke the listener, violating the single-response contract. Guard against double-response by checking if the listener has already been notified. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java [369-381] case org.opensearch.sql.job.QueryResult.Running running -> { try { clientRef .threadPool() .generic() .execute(() -> respondRunning(running, ctxListener)); } catch (RuntimeException e) { + // Only respond if the task wasn't queued; otherwise respondRunning will handle it ctxListener.onResponse( new TransportPPLQueryResponse(formatRunning(running, false))); } } Suggestion importance[1-10]: 3 __ Why: The concern about double-response is valid in theory, but the catch block only catches exceptions from execute() itself (e.g., rejection before queuing), not from the queued task. Once queued, respondRunning() will handle the response. The current code appears correct, though the comment suggestion adds clarity.	Low

Suggestions up to commit 81b5c13

Category	Suggestion	Impact
General	Close runner before completing future The runner should be closed before completing the future to prevent race conditions where observers of the completed future might still try to sample progress or current result from a closed runner. core/src/main/java/org/opensearch/sql/job/QueryJob.java [266-277] private void onRunnerSuccess(QueryResult value) { synchronized (this) { if (state != QueryJobState.RUNNING) { return; } state = QueryJobState.SUCCEEDED; completedAtMillis = OptionalLong.of(clock.millis()); result = Optional.of(value); } + safeCloseRunner(); completion.complete(value); - safeCloseRunner(); } Suggestion importance[1-10]: 4 __ Why: While the ordering concern is valid, the suggestion doesn't account for the fact that safeCloseRunner() is best-effort and the completion needs to happen regardless. The current order ensures the future completes even if closing fails, which is the safer approach.	Low
Possible issue	Guard RUNNING rows for query jobs The RUNNING state should only return rows for query jobs, not for Spark jobs. Add a check to ensure current is only used when the status represents a query job, otherwise return empty schema and rows like PENDING/CANCELLED states. async-query-core/src/main/java/org/opensearch/sql/spark/asyncquery/AsyncQueryExecutorServiceImpl.java [266-277] -// RUNNING carries the rows produced so far, if any. -QueryResult.Rows current = status.current().orElse(QueryResult.Rows.EMPTY); +// RUNNING carries the rows produced so far, if any, for query jobs only. +if (status.current().isPresent()) { + QueryResult.Rows current = status.current().get(); + return new AsyncQueryExecutionResponse( + status.state().name(), + current.schema(), + current.rows(), + null, + null, + null, + null, + status.progress(), + current.warnings()); +} return new AsyncQueryExecutionResponse( - status.state().name(), - current.schema(), - current.rows(), - null, - null, - null, - null, - status.progress(), - current.warnings()); + status.state().name(), EMPTY_SCHEMA, List.of(), null, null, null, null, status.progress(), List.of()); Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the code. The current field is only present in RUNNING state per the QueryJobStatus invariants (line 88-89 in QueryJobStatus.java), so the orElse(QueryResult.Rows.EMPTY) already handles the case when current is not present. The existing code is correct.	Low

Suggestions up to commit 146638b

Category	Suggestion	Impact
General	Cache repeated method call result The status.current() is called three times in sequence without caching the result. If the underlying implementation is not thread-safe or has side effects, this could lead to inconsistent data being returned (e.g., different schema, rows, or warnings from different snapshots). Cache the result of status.current() in a local variable before mapping. async-query-core/src/main/java/org/opensearch/sql/spark/asyncquery/AsyncQueryExecutorServiceImpl.java [267-276] +Optional<QueryResult.Rows> current = status.current(); return new AsyncQueryExecutionResponse( status.state().name(), - status.current().map(QueryResult.Rows::schema).orElse(EMPTY_SCHEMA), - status.current().map(QueryResult.Rows::rows).orElse(List.of()), + current.map(QueryResult.Rows::schema).orElse(EMPTY_SCHEMA), + current.map(QueryResult.Rows::rows).orElse(List.of()), null, null, null, null, status.progress(), - status.current().map(QueryResult.Rows::warnings).orElse(List.of())); + current.map(QueryResult.Rows::warnings).orElse(List.of())); Suggestion importance[1-10]: 7 __ Why: Caching status.current() prevents potential inconsistencies from multiple calls and improves code clarity. The suggestion correctly identifies that the method is called three times, which could lead to different snapshots if the underlying state changes between calls.	Medium
	Handle schema computation exceptions safely The schemaOf.apply(copy) is called while holding the monitor, but schemaOf is a user-provided function that could block or throw. If it blocks, it prevents other threads from adding rows or taking snapshots. If it throws, the monitor is released but schema remains null, causing subsequent snapshots to retry the same failing operation. Move the schema computation outside the synchronized block or handle exceptions explicitly. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/PartialRows.java [57-70] public synchronized Optional<QueryResult.Rows> snapshot() { if (schemaOf == null || rows.isEmpty()) { return Optional.empty(); } if (last == null || last.rows().size() != rows.size()) { List<ExprValue> copy = List.copyOf(rows); - if (schema == null) { - // Fixed once there is a row: only the first row decides runtime-typed columns. - schema = schemaOf.apply(copy); + Schema computedSchema = schema; + if (computedSchema == null) { + try { + computedSchema = schemaOf.apply(copy); + schema = computedSchema; + } catch (RuntimeException e) { + // Schema computation failed; return empty to avoid blocking future snapshots + return Optional.empty(); + } } - last = new QueryResult.Rows(schema, copy, Cursor.None, warnings, 0); + last = new QueryResult.Rows(computedSchema, copy, Cursor.None, warnings, 0); } return Optional.of(last); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that schemaOf.apply() could throw or block while holding the monitor. However, returning Optional.empty() on exception may not be the best approach as it silently swallows errors. The concern about blocking is valid and the suggestion to handle exceptions is reasonable, though the specific implementation could be improved.	Low
Possible issue	Prevent race condition in state sampling The sampleCurrentResult() and sampleRunnerProgress() are called outside the synchronized block, which means the job state could transition between these calls and the synchronized block. If the job completes after sampleCurrentResult() but before entering the synchronized block, the status could incorrectly include current rows for a non-RUNNING state. Consider sampling inside the synchronized block or validating state consistency. core/src/main/java/org/opensearch/sql/job/QueryJob.java [117-132] public QueryJobStatus status() { - // Rows first: progress read afterwards covers at least the work behind those rows. - Optional<QueryResult.Rows> current = sampleCurrentResult(); - double sample = sampleRunnerProgress(); synchronized (this) { + // Sample only if RUNNING to avoid race conditions with state transitions + Optional<QueryResult.Rows> current = + state == QueryJobState.RUNNING ? sampleCurrentResult() : Optional.empty(); + double sample = + state == QueryJobState.RUNNING ? sampleRunnerProgress() : 0D; return new QueryJobStatus( id, state, submittedAtMillis, startedAtMillis, completedAtMillis, failure, result, progress(sample), runningOnly(current)); } } Suggestion importance[1-10]: 3 __ Why: While the concern about race conditions is valid, the existing code already handles this through the runningOnly() method which filters current based on state inside the synchronized block. The suggested change would move expensive operations inside the monitor, potentially causing performance issues. The current design appears intentional.	Low

Suggestions up to commit 927a191

Category	Suggestion	Impact
General	Include warnings from partial results The RUNNING state response should include warnings from the current result if available. Currently, warnings are always set to an empty list, which may lose important diagnostic information that has been accumulated during partial execution. async-query-core/src/main/java/org/opensearch/sql/spark/asyncquery/AsyncQueryExecutorServiceImpl.java [267-276] return new AsyncQueryExecutionResponse( status.state().name(), status.current().map(QueryResult.Rows::schema).orElse(EMPTY_SCHEMA), status.current().map(QueryResult.Rows::rows).orElse(List.of()), null, null, null, null, status.progress(), - List.of()); + status.current().map(QueryResult.Rows::warnings).orElse(List.of())); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that warnings from current results should be included in the RUNNING state response. However, the impact is moderate since warnings are diagnostic information rather than critical functionality.	Medium
	Prevent race condition in state check The method releases the monitor before calling runner.currentResult(), but the state could transition from RUNNING to terminal between the check and the call. This creates a race where the runner might be closed while being sampled, potentially causing errors or inconsistent state. core/src/main/java/org/opensearch/sql/job/QueryJob.java [316-329] private Optional<QueryResult.Rows> sampleCurrentResult() { + QueryRunner currentRunner; synchronized (this) { if (state != QueryJobState.RUNNING) { return Optional.empty(); } + currentRunner = runner; } try { - Optional<QueryResult.Rows> current = runner.currentResult(); + Optional<QueryResult.Rows> current = currentRunner.currentResult(); return current == null ? Optional.empty() : current; } catch (RuntimeException e) { LOG.debug("Failed to read the rows of a running query job", e); return Optional.empty(); } } Suggestion importance[1-10]: 3 __ Why: While the suggestion identifies a potential race condition, the existing code already handles this gracefully by catching RuntimeException and logging it. The proposed change doesn't materially improve safety since runner is a final field and the method already handles exceptions from currentResult().	Low
	Avoid potential double discard of rows The discardPartialRows() call is only in the SQLException catch block, but the subsequent catch (RuntimeException | Error e) block also discards partial rows. If a SQLException is thrown and then re-thrown as a RuntimeException, partial rows could be discarded twice, potentially causing issues. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [344-363] +} catch (SQLException e) { + discardPartialRows(); + if (isPitContextLimitReached(e)) { + // reason (title) comes from the wrapped cause's message; keep it short and put the + // explanation and remedy in details. + ResourceLimitExceededException pitException = - Suggestion importance[1-10]: 2 __ Why: The concern about double discard is unfounded. The two catch blocks handle different exception types and are mutually exclusive - a SQLException caught in the first block won't reach the second block. The discardPartialRows() method is idempotent anyway, making this a non-issue.	Low

Suggestions up to commit 1122f45

Category	Suggestion	Impact
General	Include warnings from current result The RUNNING state response should include warnings from the current result when available. Currently, warnings are always set to an empty list, which may lose important diagnostic information that has been accumulated during query execution. async-query-core/src/main/java/org/opensearch/sql/spark/asyncquery/AsyncQueryExecutorServiceImpl.java [267-276] return new AsyncQueryExecutionResponse( status.state().name(), status.current().map(QueryResult.Rows::schema).orElse(EMPTY_SCHEMA), status.current().map(QueryResult.Rows::rows).orElse(List.of()), null, null, null, null, status.progress(), - List.of()); + status.current().map(QueryResult.Rows::warnings).orElse(List.of())); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that warnings from current result should be included in the RUNNING state response. However, the impact is moderate since warnings during execution are typically accumulated and reported in the final result, and the PR already handles warnings for SUCCEEDED state.	Medium
	Close runner before completing future The runner is closed after completing the future, which means concurrent threads may still call currentResult() or progress() on a closed runner. Close the runner before completing the future to ensure no thread accesses it after closure. core/src/main/java/org/opensearch/sql/job/QueryJob.java [260-271] private void onRunnerSuccess(QueryResult value) { synchronized (this) { if (state != QueryJobState.RUNNING) { return; } state = QueryJobState.SUCCEEDED; completedAtMillis = OptionalLong.of(clock.millis()); result = Optional.of(value); } + safeCloseRunner(); completion.complete(value); - safeCloseRunner(); } Suggestion importance[1-10]: 5 __ Why: While the suggestion raises a valid concurrency concern, the actual risk is low. The safeCloseRunner() method is designed to be safe even if called while other threads access the runner (it's best-effort). The current order (complete future, then close) allows waiting threads to receive the result before cleanup, which is reasonable. Reversing the order could introduce other race conditions.	Low
	Detect adding rows after discard After discard() is called, add() silently ignores rows because schemaOf is null. This could mask bugs where rows are added after discard. Consider adding a check to detect this condition and either throw an exception or log a warning. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/progress/PartialRows.java [34-38] public synchronized void add(ExprValue row) { if (schemaOf != null) { rows.add(row); + } else if (!rows.isEmpty()) { + // Rows were collected but then discarded; adding more is likely a bug + throw new IllegalStateException("Cannot add rows after discard"); } } Suggestion importance[1-10]: 4 __ Why: The suggestion attempts to detect a potential bug, but the proposed check (!rows.isEmpty()) is flawed. After discard(), rows is cleared (via reset()), so rows.isEmpty() would be true. The silent ignore behavior is intentional for the discard case, and adding rows after discard without prior open() is already handled correctly by the null check.	Low

[github-actions]

Persistent review updated to latest commit 1122f45

[github-actions]

Persistent review updated to latest commit 927a191

[github-actions]

Persistent review updated to latest commit 146638b

[github-actions]

Persistent review updated to latest commit 81b5c13

[github-actions]

Persistent review updated to latest commit da78e9d

[codecov]

Codecov Report

❌ Patch coverage is 74.53416% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.53%. Comparing base (760eead) to head (9db3b41).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...opensearch/executor/OpenSearchExecutionEngine.java	0.00%	35 Missing ⚠️
...src/main/java/org/opensearch/sql/job/QueryJob.java	91.66%	1 Missing and 1 partial ⚠️
...in/java/org/opensearch/sql/job/QueryJobStatus.java	33.33%	1 Missing and 1 partial ⚠️
.../sql/opensearch/executor/progress/PartialRows.java	96.55%	0 Missing and 1 partial ⚠️
...search/executor/progress/QueryProgressTracker.java	50.00%	1 Missing ⚠️

❌ Your project check has failed because the head coverage (75.53%) 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    #5920      +/-   ##
============================================
+ Coverage     75.51%   75.53%   +0.01%     
- Complexity    10439    10474      +35     
============================================
  Files           969      970       +1     
  Lines         41306    41446     +140     
  Branches       4711     4724      +13     
============================================
+ Hits          31193    31306     +113     
- Misses         8788     8812      +24     
- Partials       1325     1328       +3     

Flag	Coverage Δ
sql-engine	75.53% <74.53%> (+0.01%)	⬆️

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 9db3b41