From cc1c2b547b1e6fab01679e616c95daa22f3ab6c8 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Fri, 19 Jun 2026 03:22:02 +0000 Subject: [PATCH 1/4] Classify unsupported-feature errors as client errors (4xx) on the SQL path On the analytics-engine (Calcite) route, requests for features the engine does not implement leaked to clients as HTTP 500 "internal problem at backend": - SQL `vectorSearch(...)` (a table function) -> CalciteUnsupportedException - other CalciteUnsupportedException cases on the SQL `_sql` endpoint These are *client* errors ("you asked for something this engine does not support"), not backend faults. Returning 500 wrongly signals a server fault (prompting client retries) and pollutes the server-error metric / ops alarms. The PPL path (RestPPLQueryAction) already classifies these as 400; the SQL path (RestSqlAction) was missing the equivalent, so the two engines were inconsistent for the same exception. Changes: - RestSqlAction.isClientError(): add QueryEngineException (parent of CalciteUnsupportedException), mirroring RestPPLQueryAction so 4xx is consistent across SQL and PPL. - RestSqlAction / RestPPLQueryAction getRawErrorCode(): honor an ErrorReport's structured ErrorCode (UNSUPPORTED_OPERATION, FIELD_NOT_FOUND, PERMISSION_DENIED, ...) when mapping to HTTP status, instead of always unwrapping to the cause. Backend/unknown codes fall back to the existing cause-classification, so nothing regresses. This is the ErrorCode-based classification the PPL path's TODO anticipated. - CalciteRelNodeVisitor.visitTableFunction(): surface the unsupported table function as a structured ErrorReport coded UNSUPPORTED_OPERATION with a user suggestion, preserving the CalciteUnsupportedException as the cause so QueryService's v2-fallback detection still recognizes it. Tests: - RestSqlActionErrorStatusTest, RestPPLQueryActionErrorStatusTest: status mapping for QueryEngineException, ErrorReport codes, fallback-to-cause, and genuine 500s. - CalciteRelNodeVisitorSearchSimpleTest: visitTableFunction throws an ErrorReport(UNSUPPORTED_OPERATION) preserving the CalciteUnsupportedException cause. Signed-off-by: Jialiang Liang --- .../sql/calcite/CalciteRelNodeVisitor.java | 14 ++- ...CalciteRelNodeVisitorSearchSimpleTest.java | 26 +++++ .../sql/legacy/plugin/RestSqlAction.java | 47 ++++++++- .../plugin/RestSqlActionErrorStatusTest.java | 98 +++++++++++++++++++ .../sql/plugin/rest/RestPPLQueryAction.java | 44 +++++++-- .../RestPPLQueryActionErrorStatusTest.java | 56 +++++++++++ 6 files changed, 275 insertions(+), 10 deletions(-) create mode 100644 legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java create mode 100644 plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index b07f308f91f..58c07ff1e1b 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -3254,7 +3254,19 @@ private static boolean isComparableOrderKey(RexNode key) { @Override public RelNode visitTableFunction(TableFunction node, CalcitePlanContext context) { - throw new CalciteUnsupportedException("Table function is unsupported in Calcite"); + // Table functions (e.g. SQL `vectorSearch(...)`) are not implemented on the Calcite / + // analytics-engine path. Surface this as a structured ErrorReport with the + // UNSUPPORTED_OPERATION code so the REST layer classifies it as a client error (4xx) rather + // than a backend fault (5xx). The wrapped CalciteUnsupportedException is preserved as the + // cause so the v2-fallback detection in QueryService#isCalciteUnsupportedError still applies. + throw ErrorReport.wrap( + new CalciteUnsupportedException("Table function is unsupported in Calcite")) + .code(ErrorCode.UNSUPPORTED_OPERATION) + .location("while planning a table function on the analytics engine") + .suggestion( + "Table functions are not supported on the analytics engine; run this query against a" + + " non-analytics index.") + .build(); } /** diff --git a/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java b/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java index ed878ca622d..30a0e84f036 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java @@ -6,16 +6,22 @@ package org.opensearch.sql.calcite; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.util.List; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mock; import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.tree.Relation; import org.opensearch.sql.ast.tree.Search; +import org.opensearch.sql.ast.tree.TableFunction; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.datasource.DataSourceService; +import org.opensearch.sql.exception.CalciteUnsupportedException; /** * Simple tests for CalciteRelNodeVisitor.visitSearch method. Tests basic functionality without @@ -102,4 +108,24 @@ public void testSearchWithSpecialCharacters() { // Assert assertEquals(queryString, searchNode.getQueryString()); } + + /** + * Table functions (e.g. SQL {@code vectorSearch(...)}) are unsupported on the Calcite / + * analytics-engine path. The visitor must surface this as a structured {@link ErrorReport} coded + * {@link ErrorCode#UNSUPPORTED_OPERATION} (so the REST layer returns a 4xx, not a 500), while + * preserving the {@link CalciteUnsupportedException} cause so the v2-fallback detection in + * QueryService still recognizes it. + */ + @Test + public void testVisitTableFunctionThrowsUnsupportedOperationErrorReport() { + TableFunction tableFunction = + new TableFunction(AstDSL.qualifiedName("vectorSearch"), List.of()); + + ErrorReport report = + assertThrows(ErrorReport.class, () -> visitor.visitTableFunction(tableFunction, null)); + + assertEquals(ErrorCode.UNSUPPORTED_OPERATION, report.getCode()); + assertInstanceOf(CalciteUnsupportedException.class, report.getCause()); + assertNotNull(report.getSuggestion()); + } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 4064b73d4a4..99c61180f38 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -32,9 +32,11 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorCode; import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.utils.QueryContext; import org.opensearch.sql.exception.ExpressionEvaluationException; +import org.opensearch.sql.exception.QueryEngineException; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.legacy.antlr.OpenSearchLegacySqlAnalyzer; import org.opensearch.sql.legacy.antlr.SqlAnalysisConfig; @@ -216,9 +218,15 @@ public static RestStatus getRestStatus(Exception ex) { } private static int getRawErrorCode(Exception ex) { - // Recursively unwrap ErrorReport to get to the underlying cause - if (ex instanceof ErrorReport) { - return getRawErrorCode(((ErrorReport) ex).getCause()); + if (ex instanceof ErrorReport errorReport) { + // Prefer the structured ErrorCode the producing layer attached: it tells us whether this is + // a client error independent of the wrapped cause's runtime type. Fall back to unwrapping + // and classifying the cause when the code is unset/UNKNOWN. + Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); + if (codeStatus != null) { + return codeStatus; + } + return getRawErrorCode(errorReport.getCause()); } if (ex instanceof OpenSearchException) { return ((OpenSearchException) ex).status().getStatus(); @@ -229,6 +237,33 @@ private static int getRawErrorCode(Exception ex) { return 500; } + /** + * Map an {@link ErrorCode} to an HTTP status, or {@code null} when the code carries no status + * opinion (so the caller falls back to classifying the wrapped cause). Client-side codes are 4xx; + * backend codes return {@code null} rather than forcing a 5xx, since the cause may still be a + * recognized client error. + */ + private static Integer httpStatusForErrorCode(ErrorCode code) { + if (code == null) { + return null; + } + return switch (code) { + case FIELD_NOT_FOUND, + SYNTAX_ERROR, + AMBIGUOUS_FIELD, + SEMANTIC_ERROR, + EVALUATION_ERROR, + TYPE_ERROR, + UNSUPPORTED_OPERATION, + INDEX_NOT_FOUND, + RESOURCE_LIMIT_EXCEEDED -> + 400; + case PERMISSION_DENIED -> 403; + // PLANNING_ERROR, EXECUTION_ERROR, UNKNOWN: no opinion — classify the wrapped cause instead. + default -> null; + }; + } + /** * @param sqlRequest client request * @return true if this cursor was generated by the legacy engine, false otherwise. @@ -334,6 +369,12 @@ private static boolean isClientError(Exception e) { || e instanceof SqlAnalysisException || e instanceof SyntaxCheckException || e instanceof SemanticCheckException + // Unsupported-feature errors (e.g. CalciteUnsupportedException for a SQL table function + // routed to the analytics engine) are a QueryEngineException. They mean "the client asked + // for something this engine does not support" — a client error (4xx), not a backend fault + // (5xx). The PPL path (RestPPLQueryAction#isClientError) already classifies these as 400; + // mirror that here so the SQL path is consistent and these don't leak as HTTP 500. + || e instanceof QueryEngineException || e instanceof ExpressionEvaluationException; } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java new file mode 100644 index 00000000000..63f7251c1df --- /dev/null +++ b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java @@ -0,0 +1,98 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.legacy.plugin; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.index.IndexNotFoundException; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; +import org.opensearch.sql.exception.CalciteUnsupportedException; +import org.opensearch.sql.exception.QueryEngineException; + +/** + * Verifies {@link RestSqlAction#getRestStatus(Exception)} classifies unsupported-feature and other + * client errors as 4xx instead of letting them leak as HTTP 500. Covers both the exception-type + * path ({@link #isClientError}) and the structured {@link ErrorReport} / {@link ErrorCode} path. + */ +public class RestSqlActionErrorStatusTest { + + /** + * Regression for the analytics-engine cursor / vectorSearch() 500s: a {@link + * QueryEngineException} (e.g. {@link CalciteUnsupportedException} for an unsupported SQL table + * function) is a client error and must map to 400, matching the PPL path. + */ + @Test + public void queryEngineExceptionIsClientError400() { + QueryEngineException ex = new CalciteUnsupportedException("Table function is unsupported"); + assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(ex)); + } + + /** An ErrorReport carrying UNSUPPORTED_OPERATION should be a 400 regardless of cause type. */ + @Test + public void errorReportUnsupportedOperationIsClientError400() { + ErrorReport report = + ErrorReport.wrap(new RuntimeException("unsupported")) + .code(ErrorCode.UNSUPPORTED_OPERATION) + .build(); + assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(report)); + } + + /** + * An ErrorReport wrapping a CalciteUnsupportedException (the shape produced by + * visitTableFunction) maps to 400 — exercises both the ErrorCode path and the cause-unwrap + * fallback. + */ + @Test + public void errorReportWrappingCalciteUnsupportedIsClientError400() { + ErrorReport report = + ErrorReport.wrap(new CalciteUnsupportedException("Table function is unsupported")) + .code(ErrorCode.UNSUPPORTED_OPERATION) + .build(); + assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(report)); + } + + /** ErrorReport.PERMISSION_DENIED maps to 403. */ + @Test + public void errorReportPermissionDeniedIs403() { + ErrorReport report = + ErrorReport.wrap(new RuntimeException("denied")).code(ErrorCode.PERMISSION_DENIED).build(); + assertEquals(RestStatus.FORBIDDEN, RestSqlAction.getRestStatus(report)); + } + + /** + * When an ErrorReport's code carries no status opinion (e.g. EXECUTION_ERROR), classification + * falls back to the wrapped cause. A non-client cause stays 500. + */ + @Test + public void errorReportWithoutClientCodeFallsBackToCause500() { + ErrorReport report = + ErrorReport.wrap(new RuntimeException("boom")).code(ErrorCode.EXECUTION_ERROR).build(); + assertEquals(RestStatus.INTERNAL_SERVER_ERROR, RestSqlAction.getRestStatus(report)); + } + + /** + * Fallback path: an ErrorReport whose code carries no opinion but whose cause is a recognized + * client error (IndexNotFoundException is an OpenSearchException with NOT_FOUND status) keeps the + * cause's status. + */ + @Test + public void errorReportFallbackHonorsCauseStatus404() { + ErrorReport report = + ErrorReport.wrap(new IndexNotFoundException("nonexistent")).code(ErrorCode.UNKNOWN).build(); + assertEquals(RestStatus.NOT_FOUND, RestSqlAction.getRestStatus(report)); + } + + /** A genuine server fault (unrecognized runtime exception) still maps to 500. */ + @Test + public void unrecognizedExceptionIsServerError500() { + assertEquals( + RestStatus.INTERNAL_SERVER_ERROR, + RestSqlAction.getRestStatus(new RuntimeException("genuine backend fault"))); + } +} diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java index b6347bdf8e1..ad5fd5a76b1 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java @@ -24,6 +24,7 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.action.RestCancellableNodeClient; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorCode; import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.datasources.exceptions.DataSourceClientException; import org.opensearch.sql.exception.QueryEngineException; @@ -59,22 +60,53 @@ private static boolean isClientError(Exception ex) { || ex instanceof IllegalAccessException; } - private static int getRawErrorCode(Exception ex) { - if (ex instanceof ErrorReport) { - return getRawErrorCode(((ErrorReport) ex).getCause()); + // Package-private for unit testing of the error-status classification. + static int getRawErrorCode(Exception ex) { + if (ex instanceof ErrorReport errorReport) { + // Prefer the structured ErrorCode the producing layer attached: it identifies client errors + // at a finer granularity than exception types (see the note below). Fall back to unwrapping + // and classifying the cause when the code carries no status opinion. + Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); + if (codeStatus != null) { + return codeStatus; + } + return getRawErrorCode(errorReport.getCause()); } if (ex instanceof OpenSearchException) { return ((OpenSearchException) ex).status().getStatus(); } - // Possible future work: We currently do this on exception types, when we have more robust - // ErrorCodes in more locations it may be worth switching this to be based on those instead. - // That lets us identify specific error cases at a granularity higher than exception types. if (isClientError(ex)) { return 400; } return 500; } + /** + * Map an {@link ErrorCode} to an HTTP status, or {@code null} when the code carries no status + * opinion (so the caller falls back to classifying the wrapped cause). Client-side codes are 4xx; + * backend codes return {@code null} rather than forcing a 5xx, since the cause may still be a + * recognized client error. + */ + private static Integer httpStatusForErrorCode(ErrorCode code) { + if (code == null) { + return null; + } + return switch (code) { + case FIELD_NOT_FOUND, + SYNTAX_ERROR, + AMBIGUOUS_FIELD, + SEMANTIC_ERROR, + EVALUATION_ERROR, + TYPE_ERROR, + UNSUPPORTED_OPERATION, + INDEX_NOT_FOUND, + RESOURCE_LIMIT_EXCEEDED -> + 400; + case PERMISSION_DENIED -> 403; + default -> null; + }; + } + private static RestStatus loggedErrorCode(Exception ex) { int code = getRawErrorCode(ex); diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java new file mode 100644 index 00000000000..bbdaf2abf9b --- /dev/null +++ b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java @@ -0,0 +1,56 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.plugin.rest; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.opensearch.index.IndexNotFoundException; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; +import org.opensearch.sql.exception.CalciteUnsupportedException; +import org.opensearch.sql.exception.QueryEngineException; + +/** + * Verifies {@link RestPPLQueryAction#getRawErrorCode(Exception)} classifies unsupported-feature and + * other client errors as 4xx. Mirrors the SQL-path coverage so both engines stay consistent. + */ +public class RestPPLQueryActionErrorStatusTest { + + @Test + public void queryEngineExceptionIsClientError400() { + QueryEngineException ex = new CalciteUnsupportedException("AD command is unsupported"); + assertEquals(400, RestPPLQueryAction.getRawErrorCode(ex)); + } + + @Test + public void errorReportUnsupportedOperationIsClientError400() { + ErrorReport report = + ErrorReport.wrap(new RuntimeException("unsupported")) + .code(ErrorCode.UNSUPPORTED_OPERATION) + .build(); + assertEquals(400, RestPPLQueryAction.getRawErrorCode(report)); + } + + @Test + public void errorReportPermissionDeniedIs403() { + ErrorReport report = + ErrorReport.wrap(new RuntimeException("denied")).code(ErrorCode.PERMISSION_DENIED).build(); + assertEquals(403, RestPPLQueryAction.getRawErrorCode(report)); + } + + @Test + public void errorReportWithoutClientCodeFallsBackToCause() { + ErrorReport report = + ErrorReport.wrap(new IndexNotFoundException("nonexistent")).code(ErrorCode.UNKNOWN).build(); + assertEquals(404, RestPPLQueryAction.getRawErrorCode(report)); + } + + @Test + public void unrecognizedExceptionIsServerError500() { + assertEquals(500, RestPPLQueryAction.getRawErrorCode(new RuntimeException("genuine fault"))); + } +} From c90a8eb1d82bc939db7a1beafee0d3ddf78afff5 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Fri, 19 Jun 2026 03:45:25 +0000 Subject: [PATCH 2/4] Trim verbose comments Signed-off-by: Jialiang Liang --- .../sql/calcite/CalciteRelNodeVisitor.java | 7 ++----- .../sql/legacy/plugin/RestSqlAction.java | 18 ++++-------------- .../sql/plugin/rest/RestPPLQueryAction.java | 11 ++--------- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index 58c07ff1e1b..765f12e2126 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -3254,11 +3254,8 @@ private static boolean isComparableOrderKey(RexNode key) { @Override public RelNode visitTableFunction(TableFunction node, CalcitePlanContext context) { - // Table functions (e.g. SQL `vectorSearch(...)`) are not implemented on the Calcite / - // analytics-engine path. Surface this as a structured ErrorReport with the - // UNSUPPORTED_OPERATION code so the REST layer classifies it as a client error (4xx) rather - // than a backend fault (5xx). The wrapped CalciteUnsupportedException is preserved as the - // cause so the v2-fallback detection in QueryService#isCalciteUnsupportedError still applies. + // Unsupported on the Calcite path; report as UNSUPPORTED_OPERATION (4xx, not 5xx). Keep the + // CalciteUnsupportedException cause so QueryService's v2-fallback detection still applies. throw ErrorReport.wrap( new CalciteUnsupportedException("Table function is unsupported in Calcite")) .code(ErrorCode.UNSUPPORTED_OPERATION) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 99c61180f38..69d71c30b56 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -219,9 +219,7 @@ public static RestStatus getRestStatus(Exception ex) { private static int getRawErrorCode(Exception ex) { if (ex instanceof ErrorReport errorReport) { - // Prefer the structured ErrorCode the producing layer attached: it tells us whether this is - // a client error independent of the wrapped cause's runtime type. Fall back to unwrapping - // and classifying the cause when the code is unset/UNKNOWN. + // Prefer the structured ErrorCode; fall back to classifying the cause when it has no opinion. Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); if (codeStatus != null) { return codeStatus; @@ -237,12 +235,7 @@ private static int getRawErrorCode(Exception ex) { return 500; } - /** - * Map an {@link ErrorCode} to an HTTP status, or {@code null} when the code carries no status - * opinion (so the caller falls back to classifying the wrapped cause). Client-side codes are 4xx; - * backend codes return {@code null} rather than forcing a 5xx, since the cause may still be a - * recognized client error. - */ + /** Map a client-error {@link ErrorCode} to a 4xx, or {@code null} to defer to the cause. */ private static Integer httpStatusForErrorCode(ErrorCode code) { if (code == null) { return null; @@ -369,11 +362,8 @@ private static boolean isClientError(Exception e) { || e instanceof SqlAnalysisException || e instanceof SyntaxCheckException || e instanceof SemanticCheckException - // Unsupported-feature errors (e.g. CalciteUnsupportedException for a SQL table function - // routed to the analytics engine) are a QueryEngineException. They mean "the client asked - // for something this engine does not support" — a client error (4xx), not a backend fault - // (5xx). The PPL path (RestPPLQueryAction#isClientError) already classifies these as 400; - // mirror that here so the SQL path is consistent and these don't leak as HTTP 500. + // Unsupported-feature errors (e.g. CalciteUnsupportedException) are client errors, as the + // PPL path already treats them. Mirror it here so they don't leak as HTTP 500. || e instanceof QueryEngineException || e instanceof ExpressionEvaluationException; } diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java index ad5fd5a76b1..b4791dd47de 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java @@ -63,9 +63,7 @@ private static boolean isClientError(Exception ex) { // Package-private for unit testing of the error-status classification. static int getRawErrorCode(Exception ex) { if (ex instanceof ErrorReport errorReport) { - // Prefer the structured ErrorCode the producing layer attached: it identifies client errors - // at a finer granularity than exception types (see the note below). Fall back to unwrapping - // and classifying the cause when the code carries no status opinion. + // Prefer the structured ErrorCode; fall back to classifying the cause when it has no opinion. Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); if (codeStatus != null) { return codeStatus; @@ -81,12 +79,7 @@ static int getRawErrorCode(Exception ex) { return 500; } - /** - * Map an {@link ErrorCode} to an HTTP status, or {@code null} when the code carries no status - * opinion (so the caller falls back to classifying the wrapped cause). Client-side codes are 4xx; - * backend codes return {@code null} rather than forcing a 5xx, since the cause may still be a - * recognized client error. - */ + /** Map a client-error {@link ErrorCode} to a 4xx, or {@code null} to defer to the cause. */ private static Integer httpStatusForErrorCode(ErrorCode code) { if (code == null) { return null; From d376e6ec643e14c43977fae5c7ce5df402b35ec7 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Fri, 19 Jun 2026 03:59:59 +0000 Subject: [PATCH 3/4] Scope to unsupported-feature only; preserve 404 for index-not-found The earlier ErrorCode->HTTP switch was too broad: it mapped INDEX_NOT_FOUND->400 and PERMISSION_DENIED->403, which changed the existing index-not-found (404) contract that doctests assert (and that we agreed to leave to the team). Revert getRawErrorCode to upstream (unwrap ErrorReport to cause) on both paths, and revert RestPPLQueryAction entirely (the PPL path already classifies QueryEngineException as a client error). The fix is now just: - RestSqlAction.isClientError() += QueryEngineException (mirrors PPL) - visitTableFunction throws ErrorReport(UNSUPPORTED_OPERATION) wrapping CalciteUnsupportedException, which maps to 400 via the existing cause-unwrap. Tests updated to assert index-not-found stays 404 and only unsupported-feature errors become 400. Signed-off-by: Jialiang Liang --- .../sql/legacy/plugin/RestSqlAction.java | 33 +---------- .../plugin/RestSqlActionErrorStatusTest.java | 59 ++++++------------- .../sql/plugin/rest/RestPPLQueryAction.java | 37 ++---------- .../RestPPLQueryActionErrorStatusTest.java | 56 ------------------ 4 files changed, 28 insertions(+), 157 deletions(-) delete mode 100644 plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 69d71c30b56..016a5e72e0c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -32,7 +32,6 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.common.error.ErrorCode; import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.utils.QueryContext; import org.opensearch.sql.exception.ExpressionEvaluationException; @@ -218,13 +217,9 @@ public static RestStatus getRestStatus(Exception ex) { } private static int getRawErrorCode(Exception ex) { - if (ex instanceof ErrorReport errorReport) { - // Prefer the structured ErrorCode; fall back to classifying the cause when it has no opinion. - Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); - if (codeStatus != null) { - return codeStatus; - } - return getRawErrorCode(errorReport.getCause()); + // Recursively unwrap ErrorReport to get to the underlying cause + if (ex instanceof ErrorReport) { + return getRawErrorCode(((ErrorReport) ex).getCause()); } if (ex instanceof OpenSearchException) { return ((OpenSearchException) ex).status().getStatus(); @@ -235,28 +230,6 @@ private static int getRawErrorCode(Exception ex) { return 500; } - /** Map a client-error {@link ErrorCode} to a 4xx, or {@code null} to defer to the cause. */ - private static Integer httpStatusForErrorCode(ErrorCode code) { - if (code == null) { - return null; - } - return switch (code) { - case FIELD_NOT_FOUND, - SYNTAX_ERROR, - AMBIGUOUS_FIELD, - SEMANTIC_ERROR, - EVALUATION_ERROR, - TYPE_ERROR, - UNSUPPORTED_OPERATION, - INDEX_NOT_FOUND, - RESOURCE_LIMIT_EXCEEDED -> - 400; - case PERMISSION_DENIED -> 403; - // PLANNING_ERROR, EXECUTION_ERROR, UNKNOWN: no opinion — classify the wrapped cause instead. - default -> null; - }; - } - /** * @param sqlRequest client request * @return true if this cursor was generated by the legacy engine, false otherwise. diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java index 63f7251c1df..30817d1874d 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java @@ -16,16 +16,16 @@ import org.opensearch.sql.exception.QueryEngineException; /** - * Verifies {@link RestSqlAction#getRestStatus(Exception)} classifies unsupported-feature and other - * client errors as 4xx instead of letting them leak as HTTP 500. Covers both the exception-type - * path ({@link #isClientError}) and the structured {@link ErrorReport} / {@link ErrorCode} path. + * Verifies {@link RestSqlAction#getRestStatus(Exception)} classifies unsupported-feature errors as + * a client error (4xx) instead of letting them leak as HTTP 500, while leaving the existing + * index-not-found (404) and server-fault (500) behavior unchanged. */ public class RestSqlActionErrorStatusTest { /** - * Regression for the analytics-engine cursor / vectorSearch() 500s: a {@link - * QueryEngineException} (e.g. {@link CalciteUnsupportedException} for an unsupported SQL table - * function) is a client error and must map to 400, matching the PPL path. + * The fix: a {@link QueryEngineException} (e.g. {@link CalciteUnsupportedException} for an + * unsupported SQL table function such as {@code vectorSearch()}) is a client error and must map + * to 400, matching the PPL path. Previously it leaked as 500. */ @Test public void queryEngineExceptionIsClientError400() { @@ -33,20 +33,9 @@ public void queryEngineExceptionIsClientError400() { assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(ex)); } - /** An ErrorReport carrying UNSUPPORTED_OPERATION should be a 400 regardless of cause type. */ - @Test - public void errorReportUnsupportedOperationIsClientError400() { - ErrorReport report = - ErrorReport.wrap(new RuntimeException("unsupported")) - .code(ErrorCode.UNSUPPORTED_OPERATION) - .build(); - assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(report)); - } - /** - * An ErrorReport wrapping a CalciteUnsupportedException (the shape produced by - * visitTableFunction) maps to 400 — exercises both the ErrorCode path and the cause-unwrap - * fallback. + * An {@link ErrorReport} wrapping a {@link CalciteUnsupportedException} (the shape produced by + * {@code visitTableFunction}) unwraps to its client-error cause and maps to 400. */ @Test public void errorReportWrappingCalciteUnsupportedIsClientError400() { @@ -57,38 +46,28 @@ public void errorReportWrappingCalciteUnsupportedIsClientError400() { assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(report)); } - /** ErrorReport.PERMISSION_DENIED maps to 403. */ - @Test - public void errorReportPermissionDeniedIs403() { - ErrorReport report = - ErrorReport.wrap(new RuntimeException("denied")).code(ErrorCode.PERMISSION_DENIED).build(); - assertEquals(RestStatus.FORBIDDEN, RestSqlAction.getRestStatus(report)); - } - /** - * When an ErrorReport's code carries no status opinion (e.g. EXECUTION_ERROR), classification - * falls back to the wrapped cause. A non-client cause stays 500. + * Unchanged behavior: index-not-found stays 404. An {@link IndexNotFoundException} is an + * OpenSearchException whose status is NOT_FOUND; this PR must not flip that to 400. */ @Test - public void errorReportWithoutClientCodeFallsBackToCause500() { - ErrorReport report = - ErrorReport.wrap(new RuntimeException("boom")).code(ErrorCode.EXECUTION_ERROR).build(); - assertEquals(RestStatus.INTERNAL_SERVER_ERROR, RestSqlAction.getRestStatus(report)); + public void indexNotFoundStays404() { + assertEquals( + RestStatus.NOT_FOUND, + RestSqlAction.getRestStatus(new IndexNotFoundException("nonexistent"))); } /** - * Fallback path: an ErrorReport whose code carries no opinion but whose cause is a recognized - * client error (IndexNotFoundException is an OpenSearchException with NOT_FOUND status) keeps the - * cause's status. + * Unchanged behavior: an ErrorReport wrapping an IndexNotFoundException still unwraps to its + * cause's NOT_FOUND status (404), not 400. */ @Test - public void errorReportFallbackHonorsCauseStatus404() { - ErrorReport report = - ErrorReport.wrap(new IndexNotFoundException("nonexistent")).code(ErrorCode.UNKNOWN).build(); + public void errorReportWrappingIndexNotFoundStays404() { + ErrorReport report = ErrorReport.wrap(new IndexNotFoundException("nonexistent")).build(); assertEquals(RestStatus.NOT_FOUND, RestSqlAction.getRestStatus(report)); } - /** A genuine server fault (unrecognized runtime exception) still maps to 500. */ + /** Unchanged behavior: a genuine server fault still maps to 500. */ @Test public void unrecognizedExceptionIsServerError500() { assertEquals( diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java index b4791dd47de..b6347bdf8e1 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java @@ -24,7 +24,6 @@ import org.opensearch.rest.RestRequest; import org.opensearch.rest.action.RestCancellableNodeClient; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.common.error.ErrorCode; import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.datasources.exceptions.DataSourceClientException; import org.opensearch.sql.exception.QueryEngineException; @@ -60,46 +59,22 @@ private static boolean isClientError(Exception ex) { || ex instanceof IllegalAccessException; } - // Package-private for unit testing of the error-status classification. - static int getRawErrorCode(Exception ex) { - if (ex instanceof ErrorReport errorReport) { - // Prefer the structured ErrorCode; fall back to classifying the cause when it has no opinion. - Integer codeStatus = httpStatusForErrorCode(errorReport.getCode()); - if (codeStatus != null) { - return codeStatus; - } - return getRawErrorCode(errorReport.getCause()); + private static int getRawErrorCode(Exception ex) { + if (ex instanceof ErrorReport) { + return getRawErrorCode(((ErrorReport) ex).getCause()); } if (ex instanceof OpenSearchException) { return ((OpenSearchException) ex).status().getStatus(); } + // Possible future work: We currently do this on exception types, when we have more robust + // ErrorCodes in more locations it may be worth switching this to be based on those instead. + // That lets us identify specific error cases at a granularity higher than exception types. if (isClientError(ex)) { return 400; } return 500; } - /** Map a client-error {@link ErrorCode} to a 4xx, or {@code null} to defer to the cause. */ - private static Integer httpStatusForErrorCode(ErrorCode code) { - if (code == null) { - return null; - } - return switch (code) { - case FIELD_NOT_FOUND, - SYNTAX_ERROR, - AMBIGUOUS_FIELD, - SEMANTIC_ERROR, - EVALUATION_ERROR, - TYPE_ERROR, - UNSUPPORTED_OPERATION, - INDEX_NOT_FOUND, - RESOURCE_LIMIT_EXCEEDED -> - 400; - case PERMISSION_DENIED -> 403; - default -> null; - }; - } - private static RestStatus loggedErrorCode(Exception ex) { int code = getRawErrorCode(ex); diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java deleted file mode 100644 index bbdaf2abf9b..00000000000 --- a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLQueryActionErrorStatusTest.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.plugin.rest; - -import static org.junit.Assert.assertEquals; - -import org.junit.Test; -import org.opensearch.index.IndexNotFoundException; -import org.opensearch.sql.common.error.ErrorCode; -import org.opensearch.sql.common.error.ErrorReport; -import org.opensearch.sql.exception.CalciteUnsupportedException; -import org.opensearch.sql.exception.QueryEngineException; - -/** - * Verifies {@link RestPPLQueryAction#getRawErrorCode(Exception)} classifies unsupported-feature and - * other client errors as 4xx. Mirrors the SQL-path coverage so both engines stay consistent. - */ -public class RestPPLQueryActionErrorStatusTest { - - @Test - public void queryEngineExceptionIsClientError400() { - QueryEngineException ex = new CalciteUnsupportedException("AD command is unsupported"); - assertEquals(400, RestPPLQueryAction.getRawErrorCode(ex)); - } - - @Test - public void errorReportUnsupportedOperationIsClientError400() { - ErrorReport report = - ErrorReport.wrap(new RuntimeException("unsupported")) - .code(ErrorCode.UNSUPPORTED_OPERATION) - .build(); - assertEquals(400, RestPPLQueryAction.getRawErrorCode(report)); - } - - @Test - public void errorReportPermissionDeniedIs403() { - ErrorReport report = - ErrorReport.wrap(new RuntimeException("denied")).code(ErrorCode.PERMISSION_DENIED).build(); - assertEquals(403, RestPPLQueryAction.getRawErrorCode(report)); - } - - @Test - public void errorReportWithoutClientCodeFallsBackToCause() { - ErrorReport report = - ErrorReport.wrap(new IndexNotFoundException("nonexistent")).code(ErrorCode.UNKNOWN).build(); - assertEquals(404, RestPPLQueryAction.getRawErrorCode(report)); - } - - @Test - public void unrecognizedExceptionIsServerError500() { - assertEquals(500, RestPPLQueryAction.getRawErrorCode(new RuntimeException("genuine fault"))); - } -} From 18a5055677eb4f46fab27e65e5a2282179c1bb5f Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Fri, 19 Jun 2026 18:22:17 +0000 Subject: [PATCH 4/4] Normalize unsupported-feature errors in UnifiedQueryPlanner Move the fix into the planning boundary (UnifiedQueryPlanner.plan), which already normalizes Calcite internals into a clean error taxonomy (e.g. invalidTable -> SemanticCheckException). It previously re-threw CalciteUnsupportedException raw via the QueryEngineException branch, so the SQL REST path (which does not list QueryEngineException in isClientError) returned HTTP 500 for an unsupported feature such as the vectorSearch() table function, while PPL returned 400. Convert CalciteUnsupportedException to SemanticCheckException here so every consumer (SQL/PPL REST, Spark, CLI) classifies it consistently as a client error (4xx) with no REST-layer change: SemanticCheckException is already a client error on the SQL path and (as a QueryEngineException subclass) on the PPL path. The unified-planner path is independent of QueryService's v2-fallback, so this does not affect fallback behavior. Test: unsupportedFeatureIsRethrownAsSemanticCheckException in UnifiedQueryPlannerTest. Signed-off-by: Jialiang Liang --- .../sql/api/UnifiedQueryPlanner.java | 5 ++ .../sql/api/UnifiedQueryPlannerTest.java | 12 +++ .../sql/calcite/CalciteRelNodeVisitor.java | 11 +-- ...CalciteRelNodeVisitorSearchSimpleTest.java | 26 ------- .../sql/legacy/plugin/RestSqlAction.java | 4 - .../plugin/RestSqlActionErrorStatusTest.java | 77 ------------------- 6 files changed, 18 insertions(+), 117 deletions(-) delete mode 100644 legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java diff --git a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java index a84300e65f8..9440833503f 100644 --- a/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java +++ b/api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java @@ -27,6 +27,7 @@ import org.opensearch.sql.calcite.CalciteRelNodeVisitor; import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.error.ErrorReport; +import org.opensearch.sql.exception.CalciteUnsupportedException; import org.opensearch.sql.exception.QueryEngineException; import org.opensearch.sql.exception.SemanticCheckException; @@ -73,6 +74,10 @@ public RelNode plan(String query) { } return plan; }); + } catch (CalciteUnsupportedException e) { + // Unsupported feature (e.g. table functions) is an invalid query, i.e. a client error. + // Must precede the QueryEngineException branch as it is a subclass. + throw new SemanticCheckException(e.getMessage(), e); } catch (SyntaxCheckException | QueryEngineException | UnsupportedOperationException diff --git a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java index 296e9eb2519..bb2d1e4a53f 100644 --- a/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java +++ b/api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java @@ -16,6 +16,7 @@ import org.junit.Test; import org.opensearch.sql.common.antlr.SyntaxCheckException; import org.opensearch.sql.common.error.ErrorReport; +import org.opensearch.sql.exception.CalciteUnsupportedException; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.executor.QueryType; @@ -147,6 +148,17 @@ public void invalidTableIsRethrownAsSemanticCheckException() { .assertCauseType(CalciteException.class); } + @Test + public void unsupportedFeatureIsRethrownAsSemanticCheckException() { + // A feature unsupported on the analytics engine (here a PPL command that raises + // CalciteUnsupportedException; SQL table functions like vectorSearch() take the same path) is + // an invalid query, normalized to a SemanticCheckException so callers classify it as a 4xx. + givenInvalidQuery("source = catalog.employees | kmeans") + .assertErrorType(SemanticCheckException.class) + .assertCauseType(CalciteUnsupportedException.class) + .assertErrorMessageContains("unsupported in Calcite"); + } + @Test public void assertionErrorIsWrappedAsSemanticCheckException() { // Remove when the underlying Calcite assertion is fixed. diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index 765f12e2126..b07f308f91f 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -3254,16 +3254,7 @@ private static boolean isComparableOrderKey(RexNode key) { @Override public RelNode visitTableFunction(TableFunction node, CalcitePlanContext context) { - // Unsupported on the Calcite path; report as UNSUPPORTED_OPERATION (4xx, not 5xx). Keep the - // CalciteUnsupportedException cause so QueryService's v2-fallback detection still applies. - throw ErrorReport.wrap( - new CalciteUnsupportedException("Table function is unsupported in Calcite")) - .code(ErrorCode.UNSUPPORTED_OPERATION) - .location("while planning a table function on the analytics engine") - .suggestion( - "Table functions are not supported on the analytics engine; run this query against a" - + " non-analytics index.") - .build(); + throw new CalciteUnsupportedException("Table function is unsupported in Calcite"); } /** diff --git a/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java b/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java index 30a0e84f036..ed878ca622d 100644 --- a/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java +++ b/core/src/test/java/org/opensearch/sql/calcite/CalciteRelNodeVisitorSearchSimpleTest.java @@ -6,22 +6,16 @@ package org.opensearch.sql.calcite; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import java.util.List; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mock; import org.opensearch.sql.ast.dsl.AstDSL; import org.opensearch.sql.ast.tree.Relation; import org.opensearch.sql.ast.tree.Search; -import org.opensearch.sql.ast.tree.TableFunction; -import org.opensearch.sql.common.error.ErrorCode; -import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.datasource.DataSourceService; -import org.opensearch.sql.exception.CalciteUnsupportedException; /** * Simple tests for CalciteRelNodeVisitor.visitSearch method. Tests basic functionality without @@ -108,24 +102,4 @@ public void testSearchWithSpecialCharacters() { // Assert assertEquals(queryString, searchNode.getQueryString()); } - - /** - * Table functions (e.g. SQL {@code vectorSearch(...)}) are unsupported on the Calcite / - * analytics-engine path. The visitor must surface this as a structured {@link ErrorReport} coded - * {@link ErrorCode#UNSUPPORTED_OPERATION} (so the REST layer returns a 4xx, not a 500), while - * preserving the {@link CalciteUnsupportedException} cause so the v2-fallback detection in - * QueryService still recognizes it. - */ - @Test - public void testVisitTableFunctionThrowsUnsupportedOperationErrorReport() { - TableFunction tableFunction = - new TableFunction(AstDSL.qualifiedName("vectorSearch"), List.of()); - - ErrorReport report = - assertThrows(ErrorReport.class, () -> visitor.visitTableFunction(tableFunction, null)); - - assertEquals(ErrorCode.UNSUPPORTED_OPERATION, report.getCode()); - assertInstanceOf(CalciteUnsupportedException.class, report.getCause()); - assertNotNull(report.getSuggestion()); - } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 016a5e72e0c..4064b73d4a4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -35,7 +35,6 @@ import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.utils.QueryContext; import org.opensearch.sql.exception.ExpressionEvaluationException; -import org.opensearch.sql.exception.QueryEngineException; import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.legacy.antlr.OpenSearchLegacySqlAnalyzer; import org.opensearch.sql.legacy.antlr.SqlAnalysisConfig; @@ -335,9 +334,6 @@ private static boolean isClientError(Exception e) { || e instanceof SqlAnalysisException || e instanceof SyntaxCheckException || e instanceof SemanticCheckException - // Unsupported-feature errors (e.g. CalciteUnsupportedException) are client errors, as the - // PPL path already treats them. Mirror it here so they don't leak as HTTP 500. - || e instanceof QueryEngineException || e instanceof ExpressionEvaluationException; } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java deleted file mode 100644 index 30817d1874d..00000000000 --- a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSqlActionErrorStatusTest.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.legacy.plugin; - -import static org.junit.Assert.assertEquals; - -import org.junit.Test; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.index.IndexNotFoundException; -import org.opensearch.sql.common.error.ErrorCode; -import org.opensearch.sql.common.error.ErrorReport; -import org.opensearch.sql.exception.CalciteUnsupportedException; -import org.opensearch.sql.exception.QueryEngineException; - -/** - * Verifies {@link RestSqlAction#getRestStatus(Exception)} classifies unsupported-feature errors as - * a client error (4xx) instead of letting them leak as HTTP 500, while leaving the existing - * index-not-found (404) and server-fault (500) behavior unchanged. - */ -public class RestSqlActionErrorStatusTest { - - /** - * The fix: a {@link QueryEngineException} (e.g. {@link CalciteUnsupportedException} for an - * unsupported SQL table function such as {@code vectorSearch()}) is a client error and must map - * to 400, matching the PPL path. Previously it leaked as 500. - */ - @Test - public void queryEngineExceptionIsClientError400() { - QueryEngineException ex = new CalciteUnsupportedException("Table function is unsupported"); - assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(ex)); - } - - /** - * An {@link ErrorReport} wrapping a {@link CalciteUnsupportedException} (the shape produced by - * {@code visitTableFunction}) unwraps to its client-error cause and maps to 400. - */ - @Test - public void errorReportWrappingCalciteUnsupportedIsClientError400() { - ErrorReport report = - ErrorReport.wrap(new CalciteUnsupportedException("Table function is unsupported")) - .code(ErrorCode.UNSUPPORTED_OPERATION) - .build(); - assertEquals(RestStatus.BAD_REQUEST, RestSqlAction.getRestStatus(report)); - } - - /** - * Unchanged behavior: index-not-found stays 404. An {@link IndexNotFoundException} is an - * OpenSearchException whose status is NOT_FOUND; this PR must not flip that to 400. - */ - @Test - public void indexNotFoundStays404() { - assertEquals( - RestStatus.NOT_FOUND, - RestSqlAction.getRestStatus(new IndexNotFoundException("nonexistent"))); - } - - /** - * Unchanged behavior: an ErrorReport wrapping an IndexNotFoundException still unwraps to its - * cause's NOT_FOUND status (404), not 400. - */ - @Test - public void errorReportWrappingIndexNotFoundStays404() { - ErrorReport report = ErrorReport.wrap(new IndexNotFoundException("nonexistent")).build(); - assertEquals(RestStatus.NOT_FOUND, RestSqlAction.getRestStatus(report)); - } - - /** Unchanged behavior: a genuine server fault still maps to 500. */ - @Test - public void unrecognizedExceptionIsServerError500() { - assertEquals( - RestStatus.INTERNAL_SERVER_ERROR, - RestSqlAction.getRestStatus(new RuntimeException("genuine backend fault"))); - } -}