Skip to content

Fix fallback error handling to show original Calcite error #5133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Swiddis merged 6 commits into from
Feb 13, 2026
Merged

Fix fallback error handling to show original Calcite error #5133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03d691e
Fix fallback error handling to show original Calcite error
Swiddis Feb 10, 2026
2ada127
coderabbit: preserve exception types
Swiddis Feb 10, 2026
244f27b
Refactoring for coderabbit: consistent handling of calcite and legacy…
Swiddis Feb 11, 2026
3596379
Update explain error handling to match non-explain
Swiddis Feb 11, 2026
114c152
Minor tweaks: commenting & eval order
Swiddis Feb 11, 2026
e81cf26
Add a yaml test
Swiddis Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions core/src/main/java/org/opensearch/sql/executor/QueryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,10 @@ public void executeWithLegacy(
try {
executePlan(analyze(plan, queryType), PlanContext.emptyPlanContext(), listener);
} catch (Exception e) {
if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed(null)) {
// if there is a failure thrown from Calcite and execution after fallback V2
// keeps failure, we should throw the failure from Calcite.
calciteFailure.ifPresentOrElse(
t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e));
// if there is a failure thrown from Calcite and execution after fallback V2
// keeps failure, we should throw the failure from Calcite.
if (calciteFailure.isPresent()) {
listener.onFailure(new RuntimeException(calciteFailure.get()));
Comment thread
Swiddis marked this conversation as resolved.
Outdated
} else {
listener.onFailure(e);
}
Expand Down Expand Up @@ -207,11 +206,10 @@ public void explainWithLegacy(
}
executionEngine.explain(plan(analyze(plan, queryType)), listener);
} catch (Exception e) {
if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed(null)) {
// if there is a failure thrown from Calcite and execution after fallback V2
// keeps failure, we should throw the failure from Calcite.
calciteFailure.ifPresentOrElse(
t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e));
// if there is a failure thrown from Calcite and execution after fallback V2
// keeps failure, we should throw the failure from Calcite.
if (calciteFailure.isPresent()) {
listener.onFailure(new RuntimeException(calciteFailure.get()));
} else {
listener.onFailure(e);
}
Expand Down
61 changes: 61 additions & 0 deletions core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,67 @@ public void analyzeExceptionShouldBeCached() {
queryService().analyzeFail().handledByOnFailure();
}

@Test
public void testExecuteWithLegacyShouldReturnCalciteErrorWhenBothFail() {
UnsupportedOperationException calciteException =
new UnsupportedOperationException("Calcite error");
IllegalStateException v2Exception = new IllegalStateException("V2 error");

ResponseListener<ExecutionEngine.QueryResponse> responseListener =
new ResponseListener<>() {
@Override
public void onResponse(ExecutionEngine.QueryResponse pplQueryResponse) {
fail("Expected onFailure to be called");
}

@Override
public void onFailure(Exception e) {
// Should get the Calcite error wrapped in RuntimeException, not the V2 error
assertNotNull(e);
assertTrue(e instanceof RuntimeException);
assertTrue(e.getCause() instanceof UnsupportedOperationException);
assertTrue(e.getCause().getMessage().contains("Calcite error"));
}
};

lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);

QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
service.executeWithLegacy(ast, queryType, responseListener, Optional.of(calciteException));
}

@Test
public void testExplainWithLegacyShouldReturnCalciteErrorWhenBothFail() {
UnsupportedOperationException calciteException =
new UnsupportedOperationException("Calcite error");
IllegalStateException v2Exception = new IllegalStateException("V2 error");

ResponseListener<ExecutionEngine.ExplainResponse> responseListener =
new ResponseListener<>() {
@Override
public void onResponse(ExecutionEngine.ExplainResponse explainResponse) {
fail("Expected onFailure to be called");
}

@Override
public void onFailure(Exception e) {
// Should get the Calcite error wrapped in RuntimeException, not the V2 error
assertNotNull(e);
assertTrue(e instanceof RuntimeException);
assertTrue(e.getCause() instanceof UnsupportedOperationException);
assertTrue(e.getCause().getMessage().contains("Calcite error"));
}
};

lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);

QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
service.explainWithLegacy(
ast, queryType, responseListener, ExplainMode.STANDARD, Optional.of(calciteException));
}

Helper queryService() {
return new Helper();
}
Expand Down
Loading