Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
30 changes: 20 additions & 10 deletions core/src/main/java/org/opensearch/sql/executor/QueryService.java
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,16 @@ 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()) {
Throwable t = calciteFailure.get();
// Pass through Exceptions directly, wrap Errors in CalciteUnsupportedException
// to match the error handling pattern in executeWithCalcite
listener.onFailure(
t instanceof Exception
? (Exception) t
: new CalciteUnsupportedException(t.getMessage(), t));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
} else {
listener.onFailure(e);
}
Expand Down Expand Up @@ -207,11 +212,16 @@ 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()) {
Throwable t = calciteFailure.get();
// Pass through Exceptions directly, wrap Errors in CalciteUnsupportedException
// to match the error handling pattern in explainWithCalcite
listener.onFailure(
t instanceof Exception
? (Exception) t
: new CalciteUnsupportedException(t.getMessage(), t));
} else {
listener.onFailure(e);
}
Expand Down
118 changes: 118 additions & 0 deletions core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,124 @@ 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 directly (not wrapped), not the V2 error
assertNotNull(e);
assertTrue(e instanceof UnsupportedOperationException);
assertTrue(e.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 directly (not wrapped), not the V2 error
assertNotNull(e);
assertTrue(e instanceof UnsupportedOperationException);
assertTrue(e.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));
}

@Test
public void testExecuteWithLegacyShouldWrapCalciteErrorInCalciteUnsupportedException() {
AssertionError calciteError = new AssertionError("Calcite assertion failed");
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) {
// Errors should be wrapped in CalciteUnsupportedException
assertNotNull(e);
assertTrue(e instanceof org.opensearch.sql.exception.CalciteUnsupportedException);
assertTrue(e.getCause() instanceof AssertionError);
assertTrue(e.getMessage().contains("Calcite assertion failed"));
}
};

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(calciteError));
}

@Test
public void testExplainWithLegacyShouldWrapCalciteErrorInCalciteUnsupportedException() {
AssertionError calciteError = new AssertionError("Calcite assertion failed");
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) {
// Errors should be wrapped in CalciteUnsupportedException
assertNotNull(e);
assertTrue(e instanceof org.opensearch.sql.exception.CalciteUnsupportedException);
assertTrue(e.getCause() instanceof AssertionError);
assertTrue(e.getMessage().contains("Calcite assertion failed"));
}
};

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(calciteError));
}

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