Skip to content

Commit 2e42e3f

Browse files
Swiddisclaude
Swiddis
and
claude
authored
Fix fallback error handling to show original Calcite error (#5133)
* Fix fallback error handling to show original Calcite error When Calcite falls back to V2 and V2 also fails, now correctly returns the original Calcite error instead of V2's generic "only supported when calcite enabled" message, improving error clarity for users. Fixes #5060. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * coderabbit: preserve exception types Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Refactoring for coderabbit: consistent handling of calcite and legacy calcite errors Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Update explain error handling to match non-explain Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Minor tweaks: commenting & eval order Signed-off-by: Simeon Widdis <sawiddis@amazon.com> * Add a yaml test Signed-off-by: Simeon Widdis <sawiddis@amazon.com> --------- Signed-off-by: Simeon Widdis <sawiddis@amazon.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4587166 commit 2e42e3f

3 files changed

Lines changed: 214 additions & 28 deletions

File tree

‎core/src/main/java/org/opensearch/sql/executor/QueryService.java‎

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,24 @@ public class QueryService {
6464
@Getter(lazy = true)
6565
private final CalciteRelNodeVisitor relNodeVisitor = new CalciteRelNodeVisitor(dataSourceService);
6666

67+
/** Helper: depending on the type of error, either re-raise or propagate to the listener. */
68+
private void propagateCalciteError(Throwable t, ResponseListener<?> listener)
69+
throws VirtualMachineError {
70+
if (t instanceof VirtualMachineError) {
71+
// throw and fast fail the VM errors such as OOM (same with v2).
72+
throw (VirtualMachineError) t;
73+
}
74+
if (t instanceof Exception) {
75+
listener.onFailure((Exception) t);
76+
} else if (t instanceof ExceptionInInitializerError
77+
&& ((ExceptionInInitializerError) t).getException() instanceof Exception) {
78+
listener.onFailure((Exception) ((ExceptionInInitializerError) t).getException());
79+
} else {
80+
// Calcite may throw AssertError during query execution.
81+
listener.onFailure(new CalciteUnsupportedException(t.getMessage(), t));
82+
}
83+
}
84+
6785
/** Execute the {@link UnresolvedPlan}, using {@link ResponseListener} to get response.<br> */
6886
public void execute(
6987
UnresolvedPlan plan,
@@ -112,18 +130,7 @@ public void executeWithCalcite(
112130
log.warn("Fallback to V2 query engine since got exception", t);
113131
executeWithLegacy(plan, queryType, listener, Optional.of(t));
114132
} else {
115-
if (t instanceof Exception) {
116-
listener.onFailure((Exception) t);
117-
} else if (t instanceof ExceptionInInitializerError
118-
&& ((ExceptionInInitializerError) t).getException() instanceof Exception) {
119-
listener.onFailure((Exception) ((ExceptionInInitializerError) t).getException());
120-
} else if (t instanceof VirtualMachineError) {
121-
// throw and fast fail the VM errors such as OOM (same with v2).
122-
throw t;
123-
} else {
124-
// Calcite may throw AssertError during query execution.
125-
listener.onFailure(new CalciteUnsupportedException(t.getMessage(), t));
126-
}
133+
propagateCalciteError(t, listener);
127134
}
128135
}
129136
},
@@ -154,12 +161,7 @@ public void explainWithCalcite(
154161
log.warn("Fallback to V2 query engine since got exception", t);
155162
explainWithLegacy(plan, queryType, listener, mode, Optional.of(t));
156163
} else {
157-
if (t instanceof Error) {
158-
// Calcite may throw AssertError during query execution.
159-
listener.onFailure(new CalciteUnsupportedException(t.getMessage(), t));
160-
} else {
161-
listener.onFailure((Exception) t);
162-
}
164+
propagateCalciteError(t, listener);
163165
}
164166
}
165167
},
@@ -174,11 +176,11 @@ public void executeWithLegacy(
174176
try {
175177
executePlan(analyze(plan, queryType), PlanContext.emptyPlanContext(), listener);
176178
} catch (Exception e) {
177-
if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed(null)) {
178-
// if there is a failure thrown from Calcite and execution after fallback V2
179-
// keeps failure, we should throw the failure from Calcite.
180-
calciteFailure.ifPresentOrElse(
181-
t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e));
179+
if (calciteFailure.isPresent()) {
180+
// This happens if Calcite fell back to V2 due to some issue, and then V2 also failed.
181+
// Prefer the Calcite error.
182+
// https://github.com/opensearch-project/sql/issues/5060
183+
propagateCalciteError(calciteFailure.get(), listener);
182184
} else {
183185
listener.onFailure(e);
184186
}
@@ -207,11 +209,11 @@ public void explainWithLegacy(
207209
}
208210
executionEngine.explain(plan(analyze(plan, queryType)), listener);
209211
} catch (Exception e) {
210-
if (shouldUseCalcite(queryType) && isCalciteFallbackAllowed(null)) {
211-
// if there is a failure thrown from Calcite and execution after fallback V2
212-
// keeps failure, we should throw the failure from Calcite.
213-
calciteFailure.ifPresentOrElse(
214-
t -> listener.onFailure(new RuntimeException(t)), () -> listener.onFailure(e));
212+
if (calciteFailure.isPresent()) {
213+
// This happens if Calcite fell back to V2 due to some issue, and then V2 also failed.
214+
// Prefer the Calcite error.
215+
// https://github.com/opensearch-project/sql/issues/5060
216+
propagateCalciteError(calciteFailure.get(), listener);
215217
} else {
216218
listener.onFailure(e);
217219
}

‎core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,124 @@ public void analyzeExceptionShouldBeCached() {
9191
queryService().analyzeFail().handledByOnFailure();
9292
}
9393

94+
@Test
95+
public void testExecuteWithLegacyShouldReturnCalciteErrorWhenBothFail() {
96+
UnsupportedOperationException calciteException =
97+
new UnsupportedOperationException("Calcite error");
98+
IllegalStateException v2Exception = new IllegalStateException("V2 error");
99+
100+
ResponseListener<ExecutionEngine.QueryResponse> responseListener =
101+
new ResponseListener<>() {
102+
@Override
103+
public void onResponse(ExecutionEngine.QueryResponse pplQueryResponse) {
104+
fail("Expected onFailure to be called");
105+
}
106+
107+
@Override
108+
public void onFailure(Exception e) {
109+
// Should get the Calcite error directly (not wrapped), not the V2 error
110+
assertNotNull(e);
111+
assertTrue(e instanceof UnsupportedOperationException);
112+
assertTrue(e.getMessage().contains("Calcite error"));
113+
}
114+
};
115+
116+
lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
117+
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);
118+
119+
QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
120+
service.executeWithLegacy(ast, queryType, responseListener, Optional.of(calciteException));
121+
}
122+
123+
@Test
124+
public void testExplainWithLegacyShouldReturnCalciteErrorWhenBothFail() {
125+
UnsupportedOperationException calciteException =
126+
new UnsupportedOperationException("Calcite error");
127+
IllegalStateException v2Exception = new IllegalStateException("V2 error");
128+
129+
ResponseListener<ExecutionEngine.ExplainResponse> responseListener =
130+
new ResponseListener<>() {
131+
@Override
132+
public void onResponse(ExecutionEngine.ExplainResponse explainResponse) {
133+
fail("Expected onFailure to be called");
134+
}
135+
136+
@Override
137+
public void onFailure(Exception e) {
138+
// Should get the Calcite error directly (not wrapped), not the V2 error
139+
assertNotNull(e);
140+
assertTrue(e instanceof UnsupportedOperationException);
141+
assertTrue(e.getMessage().contains("Calcite error"));
142+
}
143+
};
144+
145+
lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
146+
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);
147+
148+
QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
149+
service.explainWithLegacy(
150+
ast, queryType, responseListener, ExplainMode.STANDARD, Optional.of(calciteException));
151+
}
152+
153+
@Test
154+
public void testExecuteWithLegacyShouldWrapCalciteErrorInCalciteUnsupportedException() {
155+
AssertionError calciteError = new AssertionError("Calcite assertion failed");
156+
IllegalStateException v2Exception = new IllegalStateException("V2 error");
157+
158+
ResponseListener<ExecutionEngine.QueryResponse> responseListener =
159+
new ResponseListener<>() {
160+
@Override
161+
public void onResponse(ExecutionEngine.QueryResponse pplQueryResponse) {
162+
fail("Expected onFailure to be called");
163+
}
164+
165+
@Override
166+
public void onFailure(Exception e) {
167+
// Errors should be wrapped in CalciteUnsupportedException
168+
assertNotNull(e);
169+
assertTrue(e instanceof org.opensearch.sql.exception.CalciteUnsupportedException);
170+
assertTrue(e.getCause() instanceof AssertionError);
171+
assertTrue(e.getMessage().contains("Calcite assertion failed"));
172+
}
173+
};
174+
175+
lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
176+
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);
177+
178+
QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
179+
service.executeWithLegacy(ast, queryType, responseListener, Optional.of(calciteError));
180+
}
181+
182+
@Test
183+
public void testExplainWithLegacyShouldWrapCalciteErrorInCalciteUnsupportedException() {
184+
AssertionError calciteError = new AssertionError("Calcite assertion failed");
185+
IllegalStateException v2Exception = new IllegalStateException("V2 error");
186+
187+
ResponseListener<ExecutionEngine.ExplainResponse> responseListener =
188+
new ResponseListener<>() {
189+
@Override
190+
public void onResponse(ExecutionEngine.ExplainResponse explainResponse) {
191+
fail("Expected onFailure to be called");
192+
}
193+
194+
@Override
195+
public void onFailure(Exception e) {
196+
// Errors should be wrapped in CalciteUnsupportedException
197+
assertNotNull(e);
198+
assertTrue(e instanceof org.opensearch.sql.exception.CalciteUnsupportedException);
199+
assertTrue(e.getCause() instanceof AssertionError);
200+
assertTrue(e.getMessage().contains("Calcite assertion failed"));
201+
}
202+
};
203+
204+
lenient().when(settings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
205+
lenient().when(analyzer.analyze(any(), any())).thenThrow(v2Exception);
206+
207+
QueryService service = new QueryService(analyzer, executionEngine, planner, null, settings);
208+
service.explainWithLegacy(
209+
ast, queryType, responseListener, ExplainMode.STANDARD, Optional.of(calciteError));
210+
}
211+
94212
Helper queryService() {
95213
return new Helper();
96214
}

‎integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5060.yml‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# Issue: https://github.com/opensearch-project/sql/issues/5060
2+
# PR: https://github.com/opensearch-project/sql/pull/5133
3+
# When Calcite falls back to V2 and V2 also fails, return the original Calcite error instead of V2's.
4+
#
5+
# The AD command forces a V2 fallback, then join is only supported in V3 (Calcite).
6+
# This test verifies that when both Calcite and V2 fail, the error message correctly shows
7+
# the Calcite error (CalciteUnsupportedException) rather than the V2 error.
8+
9+
setup:
10+
- do:
11+
query.settings:
12+
body:
13+
transient:
14+
plugins.calcite.enabled: true
15+
- do:
16+
indices.create:
17+
index: test_join_ad_error_5133
18+
body:
19+
mappings:
20+
properties:
21+
"event.id":
22+
type: keyword
23+
"@timestamp":
24+
type: date
25+
message:
26+
type: text
27+
- do:
28+
bulk:
29+
index: test_join_ad_error_5133
30+
refresh: true
31+
body:
32+
- '{"index": {}}'
33+
- '{"event.id": "evt1", "@timestamp": "2025-01-15T10:30:00Z", "message": "test message 1"}'
34+
- '{"index": {}}'
35+
- '{"event.id": "evt2", "@timestamp": "2025-01-15T10:31:00Z", "message": "test message 2"}'
36+
37+
---
38+
teardown:
39+
- do:
40+
query.settings:
41+
body:
42+
transient:
43+
plugins.calcite.enabled: false
44+
- do:
45+
indices.delete:
46+
index: test_join_ad_error_5133
47+
48+
---
49+
"Join with AD command should return Calcite error when both Calcite and V2 fail":
50+
- skip:
51+
features:
52+
- headers
53+
- allowed_warnings
54+
# Before the fix: Returns V2 error "Join is supported only when plugins.calcite.enabled=true" (status 500)
55+
# After the fix: Returns Calcite error "AD command is unsupported in Calcite" (status 400)
56+
- do:
57+
allowed_warnings:
58+
- 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
59+
catch: bad_request
60+
headers:
61+
Content-Type: 'application/json'
62+
ppl:
63+
body:
64+
query: source=test_join_ad_error_5133 | join `event.id` [source = test_join_ad_error_5133] | ad time_field='@timestamp'
65+
- match: { "$body": "/CalciteUnsupportedException/" }
66+
- match: { "$body": "/AD\\s+command\\s+is\\s+unsupported\\s+in\\s+Calcite/" }

0 commit comments

Comments
 (0)