Fix fallback error handling to show original Calcite error

[Swiddis]

Description

When Calcite falls back to V2 and V2 also fails, now correctly returns the original Calcite error instead of V2's. A good example of when this matters is when using a join and the ad command together: ad forces a V2 fallback, then join is only supported in V3, which gives an inconsistent error when V3 is already enabled:

$ # Before
$ echo "source=big5 | join event.id [source = big5] | ad time_field='@timestamp'" | ppl
{
  "error": {
    "reason": "There was internal problem at backend",
    "details": "Join is supported only when plugins.calcite.enabled=true",
    "type": "UnsupportedOperationException"
  },
  "status": 500
}

$ # After
$ echo "source=big5 | join event.id [source = big5] | ad time_field='@timestamp'" | ppl
{
  "error": {
    "reason": "Invalid Query",
    "details": "AD command is unsupported in Calcite",
    "type": "CalciteUnsupportedException"
  },
  "status": 400
}

Related Issues

Resolves #5060

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.
• 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.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Centralized Calcite error handling in QueryService by adding propagateCalciteError(Throwable, ResponseListener); execute/explain paths for Calcite and legacy now delegate to it. Tests added to verify direct propagation and wrapping of Calcite errors when both Calcite and legacy analyses fail.

Changes

Cohort / File(s)	Summary
Core executor logic core/src/main/java/org/opensearch/sql/executor/QueryService.java	Added propagateCalciteError(Throwable, ResponseListener) and replaced inline Calcite-vs-legacy error branches in executeWithCalcite, explainWithCalcite, executeWithLegacy, and explainWithLegacy to delegate to it. VM errors still rethrown; other Throwables are propagated to the listener or wrapped as CalciteUnsupportedException consistently.
Tests core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java	Added four tests covering legacy-path behavior when both Calcite and V2 (legacy) analyses fail: asserts for direct Calcite error surfacing and for wrapping into CalciteUnsupportedException for both execute and explain flows.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant QueryService
participant CalciteEngine
participant LegacyAnalyzer
participant ResponseListener

Client->>QueryService: request (execute/explain)
QueryService->>CalciteEngine: attempt Calcite analysis
alt Calcite succeeds
CalciteEngine-->>QueryService: calciteResult
QueryService->>ResponseListener: onResponse(calciteResult)
else Calcite fails (calciteFailure)
CalciteEngine-->>QueryService: calciteFailure
QueryService->>LegacyAnalyzer: fallback to legacy analysis
alt Legacy succeeds
LegacyAnalyzer-->>QueryService: legacyResult
QueryService->>ResponseListener: onResponse(legacyResult)
else Legacy fails (legacyFailure)
LegacyAnalyzer-->>QueryService: legacyFailure
QueryService->>QueryService: propagateCalciteError(calciteFailure, listener)
QueryService->>ResponseListener: onFailure(calciteError or wrapped CalciteUnsupportedException)
end
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing fallback error handling to preserve and show the original Calcite error.
Description check	✅ Passed	The description is well-related to the changeset, providing context about the bug, example behavior before/after, and the issue being resolved.
Linked Issues check	✅ Passed	The PR successfully addresses issue #5060 by centralizing error handling to preserve the original Calcite error when fallback occurs and V2 also fails.
Out of Scope Changes check	✅ Passed	All changes are focused on the stated objective: refactoring error handling in QueryService and adding comprehensive tests for fallback error scenarios.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@core/src/main/java/org/opensearch/sql/executor/QueryService.java`:
- Around line 177-180: The current code always wraps calciteFailure.get() in a
RuntimeException; change this so we preserve the original Calcite exception
type: check calciteFailure.get() and if it's an instance of Exception call
listener.onFailure((Exception) calciteFailure.get()), otherwise wrap the
Throwable in a RuntimeException and call listener.onFailure(new
RuntimeException(calciteFailure.get())). Update the logic around calciteFailure
and listener.onFailure to perform this instanceof check and conditional
propagation.

🧹 Nitpick comments (1)

core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java (1)

94-114: Align test expectations with preserved Calcite exception types.

If the production fix preserves the original Calcite exception, these assertions should expect the original type/message (not a RuntimeException wrapper).

🔧 Suggested update

-            assertTrue(e instanceof RuntimeException);
-            assertTrue(e.getCause() instanceof UnsupportedOperationException);
-            assertTrue(e.getCause().getMessage().contains("Calcite error"));
+            assertTrue(e instanceof UnsupportedOperationException);
+            assertTrue(e.getMessage().contains("Calcite error"));

As per coding guidelines, use specific exception types with meaningful messages; ensure proper wrapping and propagation of Calcite vs V2 errors as appropriate to the PR.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@core/src/main/java/org/opensearch/sql/executor/QueryService.java`:
- Around line 177-186: The current calciteFailure handling passes Exceptions
through but wraps Errors in CalciteUnsupportedException, which incorrectly wraps
ExceptionInInitializerError and loses the original Exception cause; update the
listener.onFailure logic in the calciteFailure handling (the block referencing
calciteFailure, listener.onFailure, and CalciteUnsupportedException) to: if t is
an Exception, pass it; else if t is an Error and t is an
ExceptionInInitializerError and t.getCause() is an Exception, pass t.getCause()
as the Exception; otherwise wrap in new
CalciteUnsupportedException(t.getMessage(), t). Apply the same change to the
other legacy handler block noted (the similar code around lines 215-224).

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@core/src/main/java/org/opensearch/sql/executor/QueryService.java`:
- Around line 85-95: propagateCalciteExplainError currently wraps all Errors
(masking VirtualMachineError) and casts non-Exception Throwables unsafely;
change it to follow propagateCalciteError's pattern: if t is a
VirtualMachineError rethrow it, else if t is an Error wrap it in
CalciteUnsupportedException and call listener.onFailure(...), else if t is an
Exception call listener.onFailure((Exception) t), otherwise wrap the Throwable
in a RuntimeException (or similar unchecked wrapper) and pass that to
listener.onFailure to avoid ClassCastException; update the method
propagateCalciteExplainError accordingly.
- Around line 67-83: The propagateCalciteExplainError method currently wraps all
Errors (including VirtualMachineError) into CalciteUnsupportedException; change
it to mirror propagateCalciteError by detecting and re-throwing
VirtualMachineError and by unwrapping ExceptionInInitializerError when
appropriate. Specifically, in propagateCalciteExplainError first check if t is
an Exception and call listener.onFailure((Exception)t); then check for
ExceptionInInitializerError whose getException() is an Exception and call
listener.onFailure on that; then check if t is a VirtualMachineError and
re-throw it; finally, for other Errors (e.g., AssertError), call
listener.onFailure(new CalciteUnsupportedException(..., t)). Ensure the same
exception types and listener.onFailure usage as in propagateCalciteError are
used.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@core/src/main/java/org/opensearch/sql/executor/QueryService.java`:
- Around line 67-82: Add unit tests for QueryService.propagateCalciteError to
cover the two missing branches: create a test that passes an
ExceptionInInitializerError which wraps an Exception and verify
ResponseListener.onFailure is invoked with the wrapped Exception (or its exact
type), and create a test that passes a subclass of VirtualMachineError (e.g.,
OutOfMemoryError) and assert that propagateCalciteError rethrows that error
instead of calling the listener; use a mocked ResponseListener to verify
interactions and place tests alongside other core tests under the same package
so they are committed with the new method.

[penghuo]

Could you add a yamlRestTest for the specific scenario described in the PR (join + ad command) to prevent regression?

The test should verify that when both Calcite and V2 fail, the error message correctly shows the Calcite error:

source=big5 | join event.id [source = big5] | ad time_field='@timestamp'

Expected error:

• type: CalciteUnsupportedException
• details: "AD command is unsupported in Calcite"
• status: 400