From 0f63e350e2a5694841b1cfc589cc82b7449688d8 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 10 Feb 2026 00:30:38 +0000 Subject: [PATCH 01/13] Initial changes for reporting interface Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorCode.java | 48 +++ .../sql/common/error/ErrorReport.java | 286 ++++++++++++++++++ .../common/error/QueryProcessingStage.java | 79 +++++ .../sql/common/error/StageErrorHandler.java | 108 +++++++ .../sql/common/error/ErrorReportTest.java | 150 +++++++++ .../opensearch/sql/executor/QueryService.java | 26 +- .../response/error/ErrorMessage.java | 24 ++ .../response/error/ErrorMessageFactory.java | 8 +- 8 files changed, 725 insertions(+), 4 deletions(-) create mode 100644 common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java create mode 100644 common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java create mode 100644 common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java create mode 100644 common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java create mode 100644 common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java new file mode 100644 index 00000000000..95e81d04b71 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java @@ -0,0 +1,48 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +/** + * Machine-readable error codes for categorizing exceptions. These codes help clients handle + * specific error types programmatically. + */ +public enum ErrorCode { + /** Field not found in the index mapping */ + FIELD_NOT_FOUND, + + /** Syntax error in query parsing */ + SYNTAX_ERROR, + + /** Ambiguous field reference (multiple fields with same name) */ + AMBIGUOUS_FIELD, + + /** Generic semantic validation error */ + SEMANTIC_ERROR, + + /** Expression evaluation failed */ + EVALUATION_ERROR, + + /** Type mismatch or type validation error */ + TYPE_ERROR, + + /** Unsupported feature or operation */ + UNSUPPORTED_OPERATION, + + /** Resource limit exceeded (memory, CPU, etc.) */ + RESOURCE_LIMIT_EXCEEDED, + + /** Index or datasource not found */ + INDEX_NOT_FOUND, + + /** Query planning failed */ + PLANNING_ERROR, + + /** Query execution failed */ + EXECUTION_ERROR, + + /** Unknown or unclassified error */ + UNKNOWN +} diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java new file mode 100644 index 00000000000..020ccfd5a44 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -0,0 +1,286 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * Error report that wraps exceptions and accumulates contextual information as errors bubble up + * through system layers. + * + *

Inspired by Rust's anyhow/eyre libraries, this class allows each layer to add context without + * modifying the original exception message. + * + *

Example usage: + * + * 

+ * try {
+ *   resolveField(fieldName);
+ * } catch (IllegalArgumentException e) {
+ *   throw ErrorReport.wrap(e)
+ *     .code(ErrorCode.FIELD_NOT_FOUND)
+ *     .stage(QueryProcessingStage.ANALYZING)
+ *     .location("while resolving fields in the index mapping")
+ *     .suggestion("Did you mean: '" + suggestedField + "'?")
+ *     .context("index_pattern", indexPattern)
+ *     .context("position", cursorPosition)
+ *     .build();
+ * }
+ *
+ */ +public class ErrorReport extends RuntimeException { + + private final Throwable cause; + private final ErrorCode code; + private final QueryProcessingStage stage; + private final List locationChain; + private final Map context; + private final String suggestion; + private final String details; + + private ErrorReport(Builder builder) { + super(builder.cause.getMessage(), builder.cause); + this.cause = builder.cause; + this.code = builder.code; + this.stage = builder.stage; + this.locationChain = new ArrayList<>(builder.locationChain); + this.context = new LinkedHashMap<>(builder.context); + this.suggestion = builder.suggestion; + this.details = builder.details; + } + + /** + * Wraps an exception with an error report builder. If the exception is already an ErrorReport, + * returns a builder initialized with the existing report's data. + * + * @param cause The underlying exception + * @return A builder for constructing the error report + */ + public static Builder wrap(Throwable cause) { + if (cause instanceof ErrorReport) { + ErrorReport existing = (ErrorReport) cause; + return new Builder(existing.cause) + .code(existing.code) + .stage(existing.stage) + .details(existing.details) + .suggestion(existing.suggestion) + .addLocationChain(existing.locationChain) + .addContext(existing.context); + } + return new Builder(cause); + } + + public ErrorCode getCode() { + return code; + } + + public QueryProcessingStage getStage() { + return stage; + } + + public List getLocationChain() { + return new ArrayList<>(locationChain); + } + + public Map getContext() { + return new LinkedHashMap<>(context); + } + + public String getSuggestion() { + return suggestion; + } + + public String getDetails() { + return details; + } + + /** Get the original exception type name. */ + public String getExceptionType() { + return cause.getClass().getSimpleName(); + } + + /** + * Format as a detailed message with all context information. This is suitable for logging or + * detailed error displays. + */ + public String toDetailedMessage() { + StringBuilder sb = new StringBuilder(); + + sb.append("Error"); + if (code != null && code != ErrorCode.UNKNOWN) { + sb.append(" [").append(code).append("]"); + } + if (stage != null) { + sb.append(" at stage: ").append(stage.getDisplayName()); + } + sb.append("\n"); + + if (details != null) { + sb.append("Details: ").append(details).append("\n"); + } + + if (!locationChain.isEmpty()) { + sb.append("\nLocation chain:\n"); + for (int i = 0; i < locationChain.size(); i++) { + sb.append(" ").append(i + 1).append(". ").append(locationChain.get(i)).append("\n"); + } + } + + if (!context.isEmpty()) { + sb.append("\nContext:\n"); + context.forEach( + (key, value) -> sb.append(" ").append(key).append(": ").append(value).append("\n")); + } + + if (suggestion != null) { + sb.append("\nSuggestion: ").append(suggestion).append("\n"); + } + + return sb.toString(); + } + + /** + * Convert to JSON-compatible map structure for REST API responses. + * + * @return Map containing error information in structured format + */ + public Map toJsonMap() { + Map json = new LinkedHashMap<>(); + + json.put("type", getExceptionType()); + + if (code != null && code != ErrorCode.UNKNOWN) { + json.put("code", code.name()); + } + + if (details != null) { + json.put("details", details); + } + + if (!locationChain.isEmpty()) { + json.put("location", new ArrayList<>(locationChain)); + } + + // Build context with stage information included + if (!context.isEmpty() || stage != null) { + Map contextMap = new LinkedHashMap<>(context); + if (stage != null) { + contextMap.put("stage", stage.toJsonKey()); + contextMap.put("stage_name", stage.getDisplayName()); + } + json.put("context", contextMap); + } + + if (suggestion != null) { + json.put("suggestion", suggestion); + } + + return json; + } + + /** Builder for constructing error reports with contextual information. */ + public static class Builder { + private final Throwable cause; + private ErrorCode code = ErrorCode.UNKNOWN; + private QueryProcessingStage stage = null; + private final List locationChain = new ArrayList<>(); + private final Map context = new LinkedHashMap<>(); + private String suggestion = null; + private String details = null; + + private Builder(Throwable cause) { + this.cause = cause; + // Default details to the original exception message + this.details = cause.getLocalizedMessage(); + } + + /** Set the machine-readable error code. */ + public Builder code(ErrorCode code) { + this.code = code; + return this; + } + + /** Set the query processing stage where the error occurred. */ + public Builder stage(QueryProcessingStage stage) { + this.stage = stage; + return this; + } + + /** + * Add a location to the chain describing where the error occurred. Locations are added in order + * from innermost to outermost layer. + * + * @param location Description like "while resolving fields in index mapping" + */ + public Builder location(String location) { + this.locationChain.add(location); + return this; + } + + /** + * Add multiple locations from an existing chain. + * + * @param locations List of location descriptions + */ + private Builder addLocationChain(List locations) { + this.locationChain.addAll(locations); + return this; + } + + /** + * Add structured context data (index name, query, position, etc). + * + * @param key Context key + * @param value Context value (will be converted to string for serialization) + */ + public Builder context(String key, Object value) { + this.context.put(key, value); + return this; + } + + /** + * Add multiple context entries from an existing map. + * + * @param contextMap Map of context key-value pairs + */ + private Builder addContext(Map contextMap) { + this.context.putAll(contextMap); + return this; + } + + /** + * Set a suggestion for how to fix the error. + * + * @param suggestion User-facing suggestion like "Did you mean: 'foo'?" + */ + public Builder suggestion(String suggestion) { + this.suggestion = suggestion; + return this; + } + + /** + * Override the default details message. By default, uses the wrapped exception's message. + * + * @param details Custom details message + */ + public Builder details(String details) { + this.details = details; + return this; + } + + /** + * Build and throw the error report as an exception. + * + * @return The constructed error report (can be thrown) + */ + public ErrorReport build() { + return new ErrorReport(this); + } + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java new file mode 100644 index 00000000000..e3c18570ca7 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java @@ -0,0 +1,79 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +/** + * Enumeration of query processing stages for error location tracking. These stages represent the + * major phases of query execution in the Calcite query planner. + * + *

Stage progression for a typical query: PARSING → AST_BUILDING → ANALYZING → OPTIMIZING → + * PLAN_CONVERSION → EXECUTING → RESULT_PROCESSING + */ +public enum QueryProcessingStage { + /** + * PARSING stage: PPL/SQL syntax parsing via ANTLR lexer/parser. Errors: Syntax errors, unexpected + * tokens, malformed queries. + */ + PARSING("Validating Query Syntax"), + + /** + * AST_BUILDING stage: Abstract Syntax Tree construction from parse tree. Errors: Invalid AST + * structure, node creation failures. + */ + AST_BUILDING("Building Query Structure"), + + /** + * ANALYZING stage: Semantic validation and type checking. Errors: Field not found, type + * mismatches, semantic violations. + */ + ANALYZING("Checking Query Against Your Data"), + + /** + * OPTIMIZING stage: Logical plan optimization with transformation rules. Errors: Optimization + * failures, rule application errors. + */ + OPTIMIZING("Optimizing Query"), + + /** + * PLAN_CONVERSION stage: Conversion to Calcite execution plan with system limits. Errors: + * Unsupported operations, plan conversion failures. + */ + PLAN_CONVERSION("Preparing Query Execution"), + + /** + * EXECUTING stage: Query execution via OpenSearch engine. Errors: Execution failures, index + * access errors, resource limits. + */ + EXECUTING("Running Query on Cluster"), + + /** + * RESULT_PROCESSING stage: Result formatting and cursor management. Errors: Result formatting + * failures, cursor errors. + */ + RESULT_PROCESSING("Formatting Results"), + + /** + * CLUSTER_VALIDATION stage: Prevalidation that cluster is healthy before execution. Errors: + * Cluster unhealthy, nodes unavailable. + */ + CLUSTER_VALIDATION("Checking Cluster Health"); + + private final String displayName; + + QueryProcessingStage(String displayName) { + this.displayName = displayName; + } + + /** Get human-readable display name for this stage. */ + public String getDisplayName() { + return displayName; + } + + /** Get lowercase name suitable for JSON serialization. */ + public String toJsonKey() { + return name().toLowerCase(); + } +} diff --git a/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java new file mode 100644 index 00000000000..73e1f23e44a --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java @@ -0,0 +1,108 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +import java.util.function.Supplier; + +/** + * Utility class for handling errors at specific query processing stages. This provides a consistent + * way to wrap operations with stage-specific error context. + * + *

Example usage in QueryService: + * + * 

+ * RelNode relNode = StageErrorHandler.executeStage(
+ *   QueryProcessingStage.ANALYZING,
+ *   () -> analyze(plan, context),
+ *   "while analyzing query plan"
+ * );
+ *
+ */ +public class StageErrorHandler { + + /** + * Execute an operation and wrap any thrown exceptions with stage context. + * + * @param stage The query processing stage + * @param operation The operation to execute + * @param location Optional location description for error context + * @param Return type of the operation + * @return The result of the operation + * @throws ErrorReport if the operation throws an exception + */ + public static T executeStage( + QueryProcessingStage stage, Supplier operation, String location) { + try { + return operation.get(); + } catch (ErrorReport e) { + // If already an ErrorReport, just add stage if not set + throw ErrorReport.wrap(e).stage(stage).location(location).build(); + } catch (Exception e) { + throw ErrorReport.wrap(e).stage(stage).location(location).build(); + } + } + + /** + * Execute an operation and wrap any thrown exceptions with stage context (no location). + * + * @param stage The query processing stage + * @param operation The operation to execute + * @param Return type of the operation + * @return The result of the operation + * @throws ErrorReport if the operation throws an exception + */ + public static T executeStage(QueryProcessingStage stage, Supplier operation) { + return executeStage(stage, operation, null); + } + + /** + * Execute a void operation and wrap any thrown exceptions with stage context. + * + * @param stage The query processing stage + * @param operation The operation to execute + * @param location Optional location description for error context + * @throws ErrorReport if the operation throws an exception + */ + public static void executeStageVoid( + QueryProcessingStage stage, Runnable operation, String location) { + try { + operation.run(); + } catch (ErrorReport e) { + throw ErrorReport.wrap(e).stage(stage).location(location).build(); + } catch (Exception e) { + throw ErrorReport.wrap(e).stage(stage).location(location).build(); + } + } + + /** + * Execute a void operation and wrap any thrown exceptions with stage context (no location). + * + * @param stage The query processing stage + * @param operation The operation to execute + * @throws ErrorReport if the operation throws an exception + */ + public static void executeStageVoid(QueryProcessingStage stage, Runnable operation) { + executeStageVoid(stage, operation, null); + } + + /** + * Wrap an exception with stage context without executing an operation. Useful for re-throwing + * exceptions with additional context. + * + * @param stage The query processing stage + * @param e The exception to wrap + * @param location Optional location description + * @return ErrorReport with stage context + */ + public static ErrorReport wrapWithStage( + QueryProcessingStage stage, Throwable e, String location) { + ErrorReport.Builder builder = ErrorReport.wrap(e).stage(stage); + if (location != null) { + builder.location(location); + } + return builder.build(); + } +} diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java new file mode 100644 index 00000000000..97fcee312e2 --- /dev/null +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -0,0 +1,150 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +import static org.junit.jupiter.api.Assertions.*; + +import java.util.Map; +import org.junit.jupiter.api.Test; + +/** Unit tests for ErrorReport. */ +public class ErrorReportTest { + + @Test + public void testBasicErrorReport() { + Exception cause = new IllegalArgumentException("Field not found"); + + ErrorReport report = + ErrorReport.wrap(cause) + .code(ErrorCode.FIELD_NOT_FOUND) + .stage(QueryProcessingStage.ANALYZING) + .location("while resolving fields in projection") + .context("field_name", "timestamp") + .context("table", "logs") + .suggestion("Check that field exists") + .build(); + + assertEquals(ErrorCode.FIELD_NOT_FOUND, report.getCode()); + assertEquals(QueryProcessingStage.ANALYZING, report.getStage()); + assertEquals(1, report.getLocationChain().size()); + assertEquals("while resolving fields in projection", report.getLocationChain().get(0)); + assertEquals("timestamp", report.getContext().get("field_name")); + assertEquals("logs", report.getContext().get("table")); + assertEquals("Check that field exists", report.getSuggestion()); + assertEquals("Field not found", report.getDetails()); + } + + @Test + public void testErrorReportJsonMapWithStageInContext() { + Exception cause = new IllegalArgumentException("Field not found"); + + ErrorReport report = + ErrorReport.wrap(cause) + .code(ErrorCode.FIELD_NOT_FOUND) + .stage(QueryProcessingStage.ANALYZING) + .location("while analyzing query") + .context("field_name", "test") + .build(); + + Map json = report.toJsonMap(); + + // Check top-level fields + assertEquals("IllegalArgumentException", json.get("type")); + assertEquals("FIELD_NOT_FOUND", json.get("code")); + assertEquals("Field not found", json.get("details")); + + // Check location + assertTrue(json.containsKey("location")); + + // Check that stage is in context + assertTrue(json.containsKey("context")); + @SuppressWarnings("unchecked") + Map context = (Map) json.get("context"); + assertEquals("analyzing", context.get("stage")); + assertEquals("Checking Query Against Your Data", context.get("stage_name")); + assertEquals("test", context.get("field_name")); + } + + @Test + public void testIdempotentWrapping() { + Exception originalCause = new IllegalArgumentException("Original error"); + + ErrorReport firstWrap = + ErrorReport.wrap(originalCause) + .code(ErrorCode.FIELD_NOT_FOUND) + .stage(QueryProcessingStage.ANALYZING) + .context("field_name", "test") + .build(); + + // Wrap again with additional context + ErrorReport secondWrap = + ErrorReport.wrap(firstWrap) + .stage(QueryProcessingStage.PLAN_CONVERSION) + .location("during plan conversion") + .context("additional_context", "value") + .build(); + + // Original cause should still be the IllegalArgumentException + assertEquals("Original error", secondWrap.getDetails()); + + // Should have accumulated context + Map context = secondWrap.getContext(); + assertEquals("test", context.get("field_name")); + assertEquals("value", context.get("additional_context")); + + // Should have location from second wrap + assertTrue(secondWrap.getLocationChain().contains("during plan conversion")); + } + + @Test + public void testStageErrorHandler() { + // Test successful execution + String result = + StageErrorHandler.executeStage( + QueryProcessingStage.ANALYZING, () -> "success", "test operation"); + + assertEquals("success", result); + + // Test error wrapping + Exception thrown = + assertThrows( + ErrorReport.class, + () -> + StageErrorHandler.executeStage( + QueryProcessingStage.ANALYZING, + () -> { + throw new IllegalArgumentException("Test error"); + }, + "while testing")); + + ErrorReport report = (ErrorReport) thrown; + assertEquals(QueryProcessingStage.ANALYZING, report.getStage()); + assertTrue(report.getLocationChain().contains("while testing")); + } + + @Test + public void testToDetailedMessage() { + Exception cause = new IllegalArgumentException("Field not found"); + + ErrorReport report = + ErrorReport.wrap(cause) + .code(ErrorCode.FIELD_NOT_FOUND) + .stage(QueryProcessingStage.ANALYZING) + .location("while resolving fields") + .context("field_name", "test") + .suggestion("Check field name") + .build(); + + String message = report.toDetailedMessage(); + + assertTrue(message.contains("FIELD_NOT_FOUND")); + assertTrue(message.contains("Semantic Analysis")); + assertTrue(message.contains("Field not found")); + assertTrue(message.contains("while resolving fields")); + assertTrue(message.contains("field_name")); + assertTrue(message.contains("Check field name")); + } +} diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java index b2c219b0e4e..4e64b0c33cd 100644 --- a/core/src/main/java/org/opensearch/sql/executor/QueryService.java +++ b/core/src/main/java/org/opensearch/sql/executor/QueryService.java @@ -34,6 +34,8 @@ import org.opensearch.sql.calcite.SysLimit; import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit; import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit.SystemLimitType; +import org.opensearch.sql.common.error.QueryProcessingStage; +import org.opensearch.sql.common.error.StageErrorHandler; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.common.utils.QueryContext; @@ -103,10 +105,28 @@ public void executeWithCalcite( CalcitePlanContext context = CalcitePlanContext.create( buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); - RelNode relNode = analyze(plan, context); - RelNode calcitePlan = convertToCalcitePlan(relNode, context); + + // Wrap analyze with ANALYZING stage tracking + RelNode relNode = + StageErrorHandler.executeStage( + QueryProcessingStage.ANALYZING, + () -> analyze(plan, context), + "checking if fields and indices exist"); + + // Wrap plan conversion with PLAN_CONVERSION stage tracking + RelNode calcitePlan = + StageErrorHandler.executeStage( + QueryProcessingStage.PLAN_CONVERSION, + () -> convertToCalcitePlan(relNode, context), + "preparing to execute your query"); + analyzeMetric.set(System.nanoTime() - analyzeStart); - executionEngine.execute(calcitePlan, context, listener); + + // Wrap execution with EXECUTING stage tracking + StageErrorHandler.executeStageVoid( + QueryProcessingStage.EXECUTING, + () -> executionEngine.execute(calcitePlan, context, listener), + "running query on your cluster"); } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java index fbe6d3cd723..f9ac724276a 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java @@ -5,9 +5,11 @@ package org.opensearch.sql.opensearch.response.error; +import java.util.Map; import lombok.Getter; import org.json.JSONObject; import org.opensearch.core.rest.RestStatus; +import org.opensearch.sql.common.error.ErrorReport; /** Error Message. */ public class ErrorMessage { @@ -68,6 +70,28 @@ private JSONObject getErrorAsJson() { errorJson.put("reason", reason); errorJson.put("details", details); + if (exception instanceof ErrorReport) { + ErrorReport report = (ErrorReport) exception; + + if (report.getCode() != null + && report.getCode() != org.opensearch.sql.common.error.ErrorCode.UNKNOWN) { + errorJson.put("code", report.getCode().name()); + } + + if (!report.getLocationChain().isEmpty()) { + errorJson.put("location", report.getLocationChain()); + } + + Map jsonMap = report.toJsonMap(); + if (jsonMap.containsKey("context")) { + errorJson.put("context", jsonMap.get("context")); + } + + if (report.getSuggestion() != null) { + errorJson.put("suggestion", report.getSuggestion()); + } + } + return errorJson; } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java index 8617f264f06..b569276e3ee 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java @@ -7,19 +7,25 @@ import lombok.experimental.UtilityClass; import org.opensearch.OpenSearchException; +import org.opensearch.sql.common.error.ErrorReport; @UtilityClass public class ErrorMessageFactory { /** * Create error message based on the exception type. Exceptions of OpenSearch exception type and * exceptions with wrapped OpenSearch exception causes should create {@link - * OpenSearchErrorMessage} + * OpenSearchErrorMessage}. ErrorReport exceptions preserve their context information. * * @param e exception to create error message * @param status exception status code * @return error message */ public static ErrorMessage createErrorMessage(Throwable e, int status) { + // Check for ErrorReport BEFORE unwrapping - we want to preserve the context + if (e instanceof ErrorReport) { + return new ErrorMessage(e, status); + } + Throwable cause = unwrapCause(e); if (cause instanceof OpenSearchException) { OpenSearchException exception = (OpenSearchException) cause; From f46a4aa5184fad02eadb2881172d1a23c16b574b Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 10 Feb 2026 00:48:50 +0000 Subject: [PATCH 02/13] Add some ITs Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorReport.java | 2 +- .../sql/common/error/ErrorReportTest.java | 2 +- .../remote/CalciteErrorReportStageIT.java | 219 ++++++++++++++++++ 3 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java index 020ccfd5a44..74c8668f962 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -172,7 +172,7 @@ public Map toJsonMap() { Map contextMap = new LinkedHashMap<>(context); if (stage != null) { contextMap.put("stage", stage.toJsonKey()); - contextMap.put("stage_name", stage.getDisplayName()); + contextMap.put("stage_description", stage.getDisplayName()); } json.put("context", contextMap); } diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java index 97fcee312e2..ad123cacb29 100644 --- a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -64,7 +64,7 @@ public void testErrorReportJsonMapWithStageInContext() { @SuppressWarnings("unchecked") Map context = (Map) json.get("context"); assertEquals("analyzing", context.get("stage")); - assertEquals("Checking Query Against Your Data", context.get("stage_name")); + assertEquals("Checking Query Against Your Data", context.get("stage_description")); assertEquals("test", context.get("field_name")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java new file mode 100644 index 00000000000..355cd94dc91 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java @@ -0,0 +1,219 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; +import static org.opensearch.sql.util.TestUtils.getResponseBody; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +/** + * Integration tests for error report builder with stage tracking. Validates that errors include + * stage information and user-friendly messages. + */ +public class CalciteErrorReportStageIT extends PPLIntegTestCase { + + @Override + public void init() throws Exception { + super.init(); + loadIndex(Index.ACCOUNT); + enableCalcite(); + } + + @Test + public void testFieldNotFoundErrorIncludesStage() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // Verify error has context with stage information + assertTrue("Error should have context", error.has("context")); + JSONObject context = error.getJSONObject("context"); + + assertTrue("Context should have stage", context.has("stage")); + assertEquals("Stage should be 'analyzing'", "analyzing", context.getString("stage")); + + assertTrue("Context should have stage_description", context.has("stage_description")); + String stageDescription = context.getString("stage_description"); + assertTrue( + "Stage description should be user-friendly", + stageDescription.contains("Checking") || stageDescription.contains("Query")); + + // Verify error has location chain + assertTrue("Error should have location", error.has("location")); + assertTrue("Location should be an array", error.get("location") instanceof org.json.JSONArray); + + // Verify location message is user-friendly (not technical) + org.json.JSONArray locationArray = error.getJSONArray("location"); + assertTrue("Location array should not be empty", locationArray.length() > 0); + String location = locationArray.getString(0); + assertFalse( + "Location should not mention internal terms like 'Calcite'", location.contains("Calcite")); + assertFalse( + "Location should not mention internal terms like 'RelNode'", location.contains("RelNode")); + } + + @Test + public void testIndexNotFoundErrorIncludesStage() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, () -> executeQuery("source=nonexistent_index | fields age")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // Verify error has context with stage + assertTrue("Error should have context", error.has("context")); + JSONObject context = error.getJSONObject("context"); + assertTrue("Context should have stage", context.has("stage")); + + // Verify error has location + assertTrue("Error should have location", error.has("location")); + } + + @Test + public void testMultipleFieldErrorsIncludeStage() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> + executeQuery( + "source=" + + TEST_INDEX_ACCOUNT + + " | fields nonexistent1, nonexistent2, nonexistent3")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // Verify stage information is present + assertTrue("Error should have context", error.has("context")); + JSONObject context = error.getJSONObject("context"); + assertTrue("Context should have stage", context.has("stage")); + assertTrue("Context should have stage_description", context.has("stage_description")); + } + + @Test + public void testErrorReportTypeIsCorrect() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field_name")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // Verify error has type field + assertTrue("Error should have type", error.has("type")); + + // Verify error has details + assertTrue("Error should have details", error.has("details")); + } + + @Test + public void testErrorCodePresent() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields missing_field")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // Verify error may have code field (optional, but if present should be valid) + if (error.has("code")) { + String code = error.getString("code"); + assertFalse("Error code should not be empty", code.isEmpty()); + assertFalse("Error code should not be UNKNOWN", code.equals("UNKNOWN")); + } + } + + @Test + public void testLocationMessagesAreUserFriendly() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields xyz123")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + assertTrue("Error should have location", error.has("location")); + org.json.JSONArray locationArray = error.getJSONArray("location"); + + // Verify all location messages are user-friendly + for (int i = 0; i < locationArray.length(); i++) { + String location = locationArray.getString(i); + + // Should not contain technical terms + assertFalse( + "Location should not contain 'AST'", + location.toLowerCase().contains("ast") && !location.toLowerCase().contains("last")); + assertFalse("Location should not contain 'RelNode'", location.contains("RelNode")); + assertFalse( + "Location should not contain 'semantic analysis' (too technical)", + location.contains("semantic analysis")); + + // Should use user-friendly language + assertTrue( + "Location should mention query, fields, data, cluster, or execution", + location.toLowerCase().contains("query") + || location.toLowerCase().contains("field") + || location.toLowerCase().contains("data") + || location.toLowerCase().contains("cluster") + || location.toLowerCase().contains("execut")); + } + } + + @Test + public void testStageDescriptionIsUserFriendly() throws IOException { + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields undefined_field")); + + String responseBody = getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + assertTrue("Error should have context", error.has("context")); + JSONObject context = error.getJSONObject("context"); + assertTrue("Context should have stage_description", context.has("stage_description")); + + String stageDescription = context.getString("stage_description"); + + // Stage description should not use compiler/technical terminology + assertFalse( + "Stage description should not contain 'Semantic'", stageDescription.contains("Semantic")); + assertFalse( + "Stage description should not contain 'Calcite'", stageDescription.contains("Calcite")); + assertFalse( + "Stage description should not contain 'AST'", + stageDescription.contains("AST") && !stageDescription.contains("Last")); + + // Should use analyst-friendly language + assertTrue( + "Stage description should be user-friendly", + stageDescription.toLowerCase().contains("check") + || stageDescription.toLowerCase().contains("validat") + || stageDescription.toLowerCase().contains("prepar") + || stageDescription.toLowerCase().contains("run") + || stageDescription.toLowerCase().contains("query")); + } +} From 31a0f6f7ef7d7ac78ac7367880e5a8637f39ef8f Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 11 Feb 2026 19:50:46 +0000 Subject: [PATCH 03/13] Improve query stage names Signed-off-by: Simeon Widdis --- .../sql/common/error/QueryProcessingStage.java | 16 ++++++++-------- .../sql/common/error/ErrorReportTest.java | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java index e3c18570ca7..686ecb03e1a 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java +++ b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java @@ -17,49 +17,49 @@ public enum QueryProcessingStage { * PARSING stage: PPL/SQL syntax parsing via ANTLR lexer/parser. Errors: Syntax errors, unexpected * tokens, malformed queries. */ - PARSING("Validating Query Syntax"), + PARSING("Validating query syntax"), /** * AST_BUILDING stage: Abstract Syntax Tree construction from parse tree. Errors: Invalid AST * structure, node creation failures. */ - AST_BUILDING("Building Query Structure"), + AST_BUILDING("Assembling the query steps"), /** * ANALYZING stage: Semantic validation and type checking. Errors: Field not found, type * mismatches, semantic violations. */ - ANALYZING("Checking Query Against Your Data"), + ANALYZING("Validating the query's steps"), /** * OPTIMIZING stage: Logical plan optimization with transformation rules. Errors: Optimization * failures, rule application errors. */ - OPTIMIZING("Optimizing Query"), + OPTIMIZING("Optimizing the query"), /** * PLAN_CONVERSION stage: Conversion to Calcite execution plan with system limits. Errors: * Unsupported operations, plan conversion failures. */ - PLAN_CONVERSION("Preparing Query Execution"), + PLAN_CONVERSION("Preparing the query for physical execution"), /** * EXECUTING stage: Query execution via OpenSearch engine. Errors: Execution failures, index * access errors, resource limits. */ - EXECUTING("Running Query on Cluster"), + EXECUTING("Running the query"), /** * RESULT_PROCESSING stage: Result formatting and cursor management. Errors: Result formatting * failures, cursor errors. */ - RESULT_PROCESSING("Formatting Results"), + RESULT_PROCESSING("Processing the query results"), /** * CLUSTER_VALIDATION stage: Prevalidation that cluster is healthy before execution. Errors: * Cluster unhealthy, nodes unavailable. */ - CLUSTER_VALIDATION("Checking Cluster Health"); + CLUSTER_VALIDATION("Checking cluster health"); private final String displayName; diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java index ad123cacb29..16fe4769d88 100644 --- a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -64,7 +64,7 @@ public void testErrorReportJsonMapWithStageInContext() { @SuppressWarnings("unchecked") Map context = (Map) json.get("context"); assertEquals("analyzing", context.get("stage")); - assertEquals("Checking Query Against Your Data", context.get("stage_description")); + assertEquals("Validating the query's steps", context.get("stage_description")); assertEquals("test", context.get("field_name")); } From 07c6ce260e84c9dd2b2efea8eabcbd6caab69bc4 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 11 Feb 2026 23:31:40 +0000 Subject: [PATCH 04/13] Rename a step description again Signed-off-by: Simeon Widdis --- .../org/opensearch/sql/common/error/QueryProcessingStage.java | 2 +- .../java/org/opensearch/sql/common/error/ErrorReportTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java index 686ecb03e1a..4184966b851 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java +++ b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java @@ -29,7 +29,7 @@ public enum QueryProcessingStage { * ANALYZING stage: Semantic validation and type checking. Errors: Field not found, type * mismatches, semantic violations. */ - ANALYZING("Validating the query's steps"), + ANALYZING("Validating the query"), /** * OPTIMIZING stage: Logical plan optimization with transformation rules. Errors: Optimization diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java index 16fe4769d88..0bf2568f9fa 100644 --- a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -64,7 +64,7 @@ public void testErrorReportJsonMapWithStageInContext() { @SuppressWarnings("unchecked") Map context = (Map) json.get("context"); assertEquals("analyzing", context.get("stage")); - assertEquals("Validating the query's steps", context.get("stage_description")); + assertEquals("Validating the query", context.get("stage_description")); assertEquals("test", context.get("field_name")); } From 23c5fbbb97eda6e039f69af88dc049814b28993d Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 25 Mar 2026 16:40:13 -0700 Subject: [PATCH 05/13] Some general improvements to integrate reports better Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorReport.java | 42 ++-- .../error/ErrorReportStatusCodeHelper.java | 102 +++++++++ .../sql/calcite/QualifiedNameResolver.java | 9 +- .../remote/CalciteErrorReportStageIT.java | 3 +- .../sql/ppl/ErrorReportStatusCodeIT.java | 204 ++++++++++++++++++ .../response/error/ErrorMessage.java | 37 ++-- .../sql/plugin/rest/RestPPLQueryAction.java | 24 ++- 7 files changed, 354 insertions(+), 67 deletions(-) create mode 100644 common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java create mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java index 74c8668f962..44a0f53ea65 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -9,6 +9,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import lombok.Getter; /** * Error report that wraps exceptions and accumulates contextual information as errors bubble up @@ -37,12 +38,12 @@ public class ErrorReport extends RuntimeException { private final Throwable cause; - private final ErrorCode code; - private final QueryProcessingStage stage; + @Getter private final ErrorCode code; + @Getter private final QueryProcessingStage stage; private final List locationChain; private final Map context; - private final String suggestion; - private final String details; + @Getter private final String suggestion; + @Getter private final String details; private ErrorReport(Builder builder) { super(builder.cause.getMessage(), builder.cause); @@ -63,8 +64,7 @@ private ErrorReport(Builder builder) { * @return A builder for constructing the error report */ public static Builder wrap(Throwable cause) { - if (cause instanceof ErrorReport) { - ErrorReport existing = (ErrorReport) cause; + if (cause instanceof ErrorReport existing) { return new Builder(existing.cause) .code(existing.code) .stage(existing.stage) @@ -76,14 +76,6 @@ public static Builder wrap(Throwable cause) { return new Builder(cause); } - public ErrorCode getCode() { - return code; - } - - public QueryProcessingStage getStage() { - return stage; - } - public List getLocationChain() { return new ArrayList<>(locationChain); } @@ -92,14 +84,6 @@ public Map getContext() { return new LinkedHashMap<>(context); } - public String getSuggestion() { - return suggestion; - } - - public String getDetails() { - return details; - } - /** Get the original exception type name. */ public String getExceptionType() { return cause.getClass().getSimpleName(); @@ -155,7 +139,7 @@ public Map toJsonMap() { json.put("type", getExceptionType()); - if (code != null && code != ErrorCode.UNKNOWN) { + if (code != null) { json.put("code", code.name()); } @@ -168,12 +152,12 @@ public Map toJsonMap() { } // Build context with stage information included - if (!context.isEmpty() || stage != null) { - Map contextMap = new LinkedHashMap<>(context); - if (stage != null) { - contextMap.put("stage", stage.toJsonKey()); - contextMap.put("stage_description", stage.getDisplayName()); - } + Map contextMap = new LinkedHashMap<>(context); + if (stage != null) { + contextMap.put("stage", stage.toJsonKey()); + contextMap.put("stage_description", stage.getDisplayName()); + } + if (!contextMap.isEmpty()) { json.put("context", contextMap); } diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java new file mode 100644 index 00000000000..e3afa27756d --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java @@ -0,0 +1,102 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.common.error; + +import java.util.function.Predicate; + +/** + * Helper class for determining HTTP status codes from ErrorReport instances. Provides utilities to + * check if an exception (potentially wrapped in ErrorReport) should result in a client error (4xx) + * or server error (5xx) status code. + */ +public class ErrorReportStatusCodeHelper { + + /** + * Determines if an exception should be treated as a client error (400 BAD_REQUEST) based on: + * + *
    + *
  1. ErrorCode (if wrapped in ErrorReport) + *
  2. Underlying exception type (checked via provided predicate) + *
+ * + * @param e the exception to check + * @param exceptionTypeChecker predicate that checks if a given exception type is a client error + * @return true if this should result in a 4xx status code, false for 5xx + */ + public static boolean isClientError(Exception e, Predicate exceptionTypeChecker) { + // If wrapped in ErrorReport, check both the ErrorCode and the underlying cause + if (e instanceof ErrorReport) { + ErrorReport report = (ErrorReport) e; + + // Check if the ErrorCode indicates a client error + if (isClientErrorCode(report.getCode())) { + return true; + } + + // Fall through to check the underlying exception type + Throwable cause = e.getCause(); + if (cause instanceof Exception) { + return exceptionTypeChecker.test((Exception) cause); + } + } + + // Check the exception type directly + return exceptionTypeChecker.test(e); + } + + /** + * Maps ErrorCode values to client error (400) vs server error (500) status codes. + * + *

Client errors (400): + * + *

    + *
  • FIELD_NOT_FOUND - invalid field reference + *
  • SYNTAX_ERROR - query parsing failed + *
  • AMBIGUOUS_FIELD - multiple fields with same name + *
  • SEMANTIC_ERROR - semantic validation failed + *
  • TYPE_ERROR - type mismatch + *
  • UNSUPPORTED_OPERATION - feature not supported + *
  • INDEX_NOT_FOUND - index doesn't exist + *
+ * + *

Server errors (500): + * + *

    + *
  • EVALUATION_ERROR - runtime evaluation failed + *
  • PLANNING_ERROR - query planning failed + *
  • EXECUTION_ERROR - query execution failed + *
  • RESOURCE_LIMIT_EXCEEDED - system resource limit + *
  • UNKNOWN - unclassified error + *
+ * + * @param code the ErrorCode to check + * @return true if this ErrorCode indicates a client error (400), false for server error (500) + */ + private static boolean isClientErrorCode(ErrorCode code) { + if (code == null) { + return false; + } + + switch (code) { + case FIELD_NOT_FOUND: + case SYNTAX_ERROR: + case AMBIGUOUS_FIELD: + case SEMANTIC_ERROR: + case TYPE_ERROR: + case UNSUPPORTED_OPERATION: + case INDEX_NOT_FOUND: + return true; + + case EVALUATION_ERROR: + case PLANNING_ERROR: + case EXECUTION_ERROR: + case RESOURCE_LIMIT_EXCEEDED: + case UNKNOWN: + default: + return false; + } + } +} diff --git a/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java b/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java index 0e5ac4a6e05..225af58d6cf 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java +++ b/core/src/main/java/org/opensearch/sql/calcite/QualifiedNameResolver.java @@ -16,6 +16,8 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.sql.ast.expression.QualifiedName; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.expression.function.PPLFuncImpTable; @@ -322,7 +324,10 @@ private static Optional replaceWithNullLiteralInCoalesce(CalcitePlanCon return Optional.empty(); } - private static RuntimeException getNotFoundException(QualifiedName node) { - return new IllegalArgumentException(String.format("Field [%s] not found.", node.toString())); + private static ErrorReport getNotFoundException(QualifiedName node) { + return ErrorReport.wrap( + new IllegalArgumentException(String.format("Field [%s] not found.", node.toString()))) + .code(ErrorCode.FIELD_NOT_FOUND) + .build(); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java index 355cd94dc91..2b907a03561 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java @@ -49,7 +49,8 @@ public void testFieldNotFoundErrorIncludesStage() throws IOException { String stageDescription = context.getString("stage_description"); assertTrue( "Stage description should be user-friendly", - stageDescription.contains("Checking") || stageDescription.contains("Query")); + stageDescription.toLowerCase().contains("checking") + || stageDescription.toLowerCase().contains("query")); // Verify error has location chain assertTrue("Error should have location", error.has("location")); diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java new file mode 100644 index 00000000000..76ee4e869d5 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java @@ -0,0 +1,204 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.util.TestUtils; + +/** + * Integration tests for ErrorReport status code handling. Validates that exceptions wrapped in + * ErrorReport are correctly mapped to HTTP status codes based on the ErrorCode. + */ +public class ErrorReportStatusCodeIT extends PPLIntegTestCase { + + @Override + public void init() throws Exception { + super.init(); + loadIndex(Index.ACCOUNT); + } + + @Test + public void testFieldNotFoundReturns400() throws IOException { + // Field not found should be a client error (400 BAD_REQUEST) + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field")); + + assertThat( + "Field not found should return 400 status", + exception.getResponse().getStatusLine().getStatusCode(), + equalTo(400)); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + + assertThat("Response should have error object", response.has("error"), equalTo(true)); + JSONObject error = response.getJSONObject("error"); + + // Verify the error includes the ErrorCode + if (error.has("code")) { + assertThat( + "Error code should be FIELD_NOT_FOUND", + error.getString("code"), + equalTo("FIELD_NOT_FOUND")); + } + } + + @Test + public void testIndexNotFoundReturns404() throws IOException { + // Index not found should return 404 NOT_FOUND + ResponseException exception = + assertThrows( + ResponseException.class, () -> executeQuery("source=nonexistent_index | fields age")); + + assertThat( + "Index not found should return 404 status", + exception.getResponse().getStatusLine().getStatusCode(), + equalTo(404)); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + + assertThat("Response should have error object", response.has("error"), equalTo(true)); + JSONObject error = response.getJSONObject("error"); + + // Verify error details mention the index + assertThat( + "Error details should mention nonexistent index", + error.getString("details"), + containsString("nonexistent_index")); + } + + @Test + public void testSyntaxErrorReturns400() throws IOException { + // Syntax error should be a client error (400 BAD_REQUEST) + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | invalid_command")); + + assertThat( + "Syntax error should return 400 status", + exception.getResponse().getStatusLine().getStatusCode(), + equalTo(400)); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + + assertThat("Response should have error object", response.has("error"), equalTo(true)); + } + + @Test + public void testSemanticErrorReturns400() throws IOException { + // Semantic errors (like type mismatches) should be client errors (400 BAD_REQUEST) + ResponseException exception = + assertThrows( + ResponseException.class, + () -> + executeQuery( + "source=" + TEST_INDEX_ACCOUNT + " | where age + 'string' > 10 | fields age")); + + assertThat( + "Semantic error should return 400 status", + exception.getResponse().getStatusLine().getStatusCode(), + equalTo(400)); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + + assertThat("Response should have error object", response.has("error"), equalTo(true)); + } + + @Test + public void testErrorReportPreservesContextInformation() throws IOException { + // Verify that ErrorReport context is included in the response + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field")); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // ErrorReport should include context information + if (error.has("context")) { + JSONObject context = error.getJSONObject("context"); + assertTrue( + "Context should include stage information", + context.has("stage") || context.has("stage_description")); + } + + // ErrorReport should include location chain if available + if (error.has("location")) { + assertTrue( + "Location should be an array", error.get("location") instanceof org.json.JSONArray); + } + } + + @Test + public void testErrorReportTypeIsErrorCodeNotExceptionClass() throws IOException { + // Verify that the "type" field is the ErrorCode, not "ErrorReport" + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field")); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // The type should be the ErrorCode, not "ErrorReport" + assertTrue("Error should have a type field", error.has("type")); + String type = error.getString("type"); + assertThat( + "Type should be an ErrorCode (e.g., FIELD_NOT_FOUND), not ErrorReport", + type, + not(equalTo("ErrorReport"))); + + // Context should include the underlying cause exception type + if (error.has("context")) { + JSONObject context = error.getJSONObject("context"); + assertTrue( + "Context should include the underlying cause exception type", context.has("cause")); + } + } + + @Test + public void testErrorReportContextIncludesCause() throws IOException { + // Verify that when ErrorReport is used, the context includes the underlying cause exception + // type + ResponseException exception = + assertThrows( + ResponseException.class, + () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field")); + + String responseBody = TestUtils.getResponseBody(exception.getResponse()); + JSONObject response = new JSONObject(responseBody); + JSONObject error = response.getJSONObject("error"); + + // If the error has context (meaning it's wrapped in ErrorReport with stage info), + // then it should include the cause + if (error.has("context")) { + JSONObject context = error.getJSONObject("context"); + assertTrue("Context should include cause field", context.has("cause")); + + // The cause should be a simple class name (not null or empty) + String cause = context.getString("cause"); + assertFalse("Cause should not be empty", cause.isEmpty()); + assertFalse("Cause should not contain package names", cause.contains(".")); + } + } +} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java index f9ac724276a..289e09645c8 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java @@ -5,7 +5,6 @@ package org.opensearch.sql.opensearch.response.error; -import java.util.Map; import lombok.Getter; import org.json.JSONObject; import org.opensearch.core.rest.RestStatus; @@ -35,6 +34,14 @@ public ErrorMessage(Throwable exception, int status) { } private String fetchType() { + // For ErrorReport, use the ErrorCode as the type (if present and not UNKNOWN) + if (exception instanceof ErrorReport) { + ErrorReport report = (ErrorReport) exception; + if (report.getCode() != null + && report.getCode() != org.opensearch.sql.common.error.ErrorCode.UNKNOWN) { + return report.getCode().name(); + } + } return exception.getClass().getSimpleName(); } @@ -64,34 +71,14 @@ public String toString() { } private JSONObject getErrorAsJson() { - JSONObject errorJson = new JSONObject(); + if (exception instanceof ErrorReport) { + return new JSONObject(((ErrorReport) exception).toJsonMap()); + } + JSONObject errorJson = new JSONObject(); errorJson.put("type", type); errorJson.put("reason", reason); errorJson.put("details", details); - - if (exception instanceof ErrorReport) { - ErrorReport report = (ErrorReport) exception; - - if (report.getCode() != null - && report.getCode() != org.opensearch.sql.common.error.ErrorCode.UNKNOWN) { - errorJson.put("code", report.getCode().name()); - } - - if (!report.getLocationChain().isEmpty()) { - errorJson.put("location", report.getLocationChain()); - } - - Map jsonMap = report.toJsonMap(); - if (jsonMap.containsKey("context")) { - errorJson.put("context", jsonMap.get("context")); - } - - if (report.getSuggestion() != null) { - errorJson.put("suggestion", report.getSuggestion()); - } - } - return errorJson; } } 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 ffdd90504f7..4457c80a854 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 @@ -25,6 +25,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReportStatusCodeHelper; import org.opensearch.sql.datasources.exceptions.DataSourceClientException; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.QueryEngineException; @@ -50,16 +51,19 @@ public RestPPLQueryAction() { } private static boolean isClientError(Exception e) { - return e instanceof NullPointerException - // NPE is hard to differentiate but more likely caused by bad query - || e instanceof IllegalArgumentException - || e instanceof IndexNotFoundException - || e instanceof SemanticCheckException - || e instanceof ExpressionEvaluationException - || e instanceof QueryEngineException - || e instanceof SyntaxCheckException - || e instanceof DataSourceClientException - || e instanceof IllegalAccessException; + return ErrorReportStatusCodeHelper.isClientError( + e, + ex -> + ex instanceof NullPointerException + // NPE is hard to differentiate but more likely caused by bad query + || ex instanceof IllegalArgumentException + || ex instanceof IndexNotFoundException + || ex instanceof SemanticCheckException + || ex instanceof ExpressionEvaluationException + || ex instanceof QueryEngineException + || ex instanceof SyntaxCheckException + || ex instanceof DataSourceClientException + || ex instanceof IllegalAccessException); } @Override From 22982f11e7ce05fc24fbcb5583f58e30a82ec6c1 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 10:59:30 -0700 Subject: [PATCH 06/13] (WIP) Fix some immediate issues with report integration Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorReport.java | 8 +- .../error/ErrorReportStatusCodeHelper.java | 102 ------------------ .../sql/common/error/StageErrorHandler.java | 4 +- .../standalone/CalcitePPLIntegTestCase.java | 30 +++--- .../sql/ppl/ErrorReportStatusCodeIT.java | 5 +- .../executor/cursor/CursorResultExecutor.java | 2 +- .../client/OpenSearchNodeClient.java | 8 +- .../sql/plugin/rest/RestPPLQueryAction.java | 81 +++++++------- 8 files changed, 69 insertions(+), 171 deletions(-) delete mode 100644 common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java index 44a0f53ea65..f2072da2e55 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -37,7 +37,7 @@ */ public class ErrorReport extends RuntimeException { - private final Throwable cause; + @Getter private final Exception cause; @Getter private final ErrorCode code; @Getter private final QueryProcessingStage stage; private final List locationChain; @@ -63,7 +63,7 @@ private ErrorReport(Builder builder) { * @param cause The underlying exception * @return A builder for constructing the error report */ - public static Builder wrap(Throwable cause) { + public static Builder wrap(Exception cause) { if (cause instanceof ErrorReport existing) { return new Builder(existing.cause) .code(existing.code) @@ -170,7 +170,7 @@ public Map toJsonMap() { /** Builder for constructing error reports with contextual information. */ public static class Builder { - private final Throwable cause; + private final Exception cause; private ErrorCode code = ErrorCode.UNKNOWN; private QueryProcessingStage stage = null; private final List locationChain = new ArrayList<>(); @@ -178,7 +178,7 @@ public static class Builder { private String suggestion = null; private String details = null; - private Builder(Throwable cause) { + private Builder(Exception cause) { this.cause = cause; // Default details to the original exception message this.details = cause.getLocalizedMessage(); diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java deleted file mode 100644 index e3afa27756d..00000000000 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReportStatusCodeHelper.java +++ /dev/null @@ -1,102 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.common.error; - -import java.util.function.Predicate; - -/** - * Helper class for determining HTTP status codes from ErrorReport instances. Provides utilities to - * check if an exception (potentially wrapped in ErrorReport) should result in a client error (4xx) - * or server error (5xx) status code. - */ -public class ErrorReportStatusCodeHelper { - - /** - * Determines if an exception should be treated as a client error (400 BAD_REQUEST) based on: - * - *
    - *
  1. ErrorCode (if wrapped in ErrorReport) - *
  2. Underlying exception type (checked via provided predicate) - *
- * - * @param e the exception to check - * @param exceptionTypeChecker predicate that checks if a given exception type is a client error - * @return true if this should result in a 4xx status code, false for 5xx - */ - public static boolean isClientError(Exception e, Predicate exceptionTypeChecker) { - // If wrapped in ErrorReport, check both the ErrorCode and the underlying cause - if (e instanceof ErrorReport) { - ErrorReport report = (ErrorReport) e; - - // Check if the ErrorCode indicates a client error - if (isClientErrorCode(report.getCode())) { - return true; - } - - // Fall through to check the underlying exception type - Throwable cause = e.getCause(); - if (cause instanceof Exception) { - return exceptionTypeChecker.test((Exception) cause); - } - } - - // Check the exception type directly - return exceptionTypeChecker.test(e); - } - - /** - * Maps ErrorCode values to client error (400) vs server error (500) status codes. - * - *

Client errors (400): - * - *

    - *
  • FIELD_NOT_FOUND - invalid field reference - *
  • SYNTAX_ERROR - query parsing failed - *
  • AMBIGUOUS_FIELD - multiple fields with same name - *
  • SEMANTIC_ERROR - semantic validation failed - *
  • TYPE_ERROR - type mismatch - *
  • UNSUPPORTED_OPERATION - feature not supported - *
  • INDEX_NOT_FOUND - index doesn't exist - *
- * - *

Server errors (500): - * - *

    - *
  • EVALUATION_ERROR - runtime evaluation failed - *
  • PLANNING_ERROR - query planning failed - *
  • EXECUTION_ERROR - query execution failed - *
  • RESOURCE_LIMIT_EXCEEDED - system resource limit - *
  • UNKNOWN - unclassified error - *
- * - * @param code the ErrorCode to check - * @return true if this ErrorCode indicates a client error (400), false for server error (500) - */ - private static boolean isClientErrorCode(ErrorCode code) { - if (code == null) { - return false; - } - - switch (code) { - case FIELD_NOT_FOUND: - case SYNTAX_ERROR: - case AMBIGUOUS_FIELD: - case SEMANTIC_ERROR: - case TYPE_ERROR: - case UNSUPPORTED_OPERATION: - case INDEX_NOT_FOUND: - return true; - - case EVALUATION_ERROR: - case PLANNING_ERROR: - case EXECUTION_ERROR: - case RESOURCE_LIMIT_EXCEEDED: - case UNKNOWN: - default: - return false; - } - } -} diff --git a/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java index 73e1f23e44a..2a9b720481a 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java +++ b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java @@ -70,8 +70,6 @@ public static void executeStageVoid( QueryProcessingStage stage, Runnable operation, String location) { try { operation.run(); - } catch (ErrorReport e) { - throw ErrorReport.wrap(e).stage(stage).location(location).build(); } catch (Exception e) { throw ErrorReport.wrap(e).stage(stage).location(location).build(); } @@ -98,7 +96,7 @@ public static void executeStageVoid(QueryProcessingStage stage, Runnable operati * @return ErrorReport with stage context */ public static ErrorReport wrapWithStage( - QueryProcessingStage stage, Throwable e, String location) { + QueryProcessingStage stage, Exception e, String location) { ErrorReport.Builder builder = ErrorReport.wrap(e).stage(stage); if (location != null) { builder.location(location); diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java index d47656471b0..b9f8c264a87 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java @@ -30,6 +30,7 @@ import org.opensearch.sql.analysis.Analyzer; import org.opensearch.sql.analysis.ExpressionAnalyzer; import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.datasource.DataSourceService; @@ -187,21 +188,20 @@ public void onResponse(ExecutionEngine.QueryResponse response) { @Override public void onFailure(Exception e) { - if (e instanceof SyntaxCheckException) { - throw (SyntaxCheckException) e; - } else if (e instanceof QueryEngineException) { - throw (QueryEngineException) e; - } else if (e instanceof UnsupportedCursorRequestException) { - throw (UnsupportedCursorRequestException) e; - } else if (e instanceof NoCursorException) { - throw (NoCursorException) e; - } else if (e instanceof UnsupportedOperationException) { - throw (UnsupportedOperationException) e; - } else if (e instanceof IllegalArgumentException) { - // most exceptions thrown by Calcite when resolve a plan. - throw (IllegalArgumentException) e; - } else { - throw new IllegalStateException("Exception happened during execution", e); + switch (e) { + case ErrorReport errorReport -> throw errorReport; + case SyntaxCheckException syntaxCheckException -> throw syntaxCheckException; + case QueryEngineException queryEngineException -> throw queryEngineException; + case UnsupportedCursorRequestException unsupportedCursorRequestException -> + throw unsupportedCursorRequestException; + case NoCursorException noCursorException -> throw noCursorException; + case UnsupportedOperationException unsupportedOperationException -> + throw unsupportedOperationException; + case IllegalArgumentException illegalArgumentException -> + // most exceptions thrown by Calcite when resolve a plan. + throw illegalArgumentException; + case null, default -> + throw new IllegalStateException("Exception happened during execution", e); } } }, diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java index 76ee4e869d5..d3a2cf91ab3 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java @@ -14,16 +14,17 @@ import org.json.JSONObject; import org.junit.Test; import org.opensearch.client.ResponseException; +import org.opensearch.sql.calcite.standalone.CalcitePPLIntegTestCase; import org.opensearch.sql.util.TestUtils; /** * Integration tests for ErrorReport status code handling. Validates that exceptions wrapped in * ErrorReport are correctly mapped to HTTP status codes based on the ErrorCode. */ -public class ErrorReportStatusCodeIT extends PPLIntegTestCase { +public class ErrorReportStatusCodeIT extends CalcitePPLIntegTestCase { @Override - public void init() throws Exception { + public void init() throws IOException { super.init(); loadIndex(Index.ACCOUNT); } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java index 8adffea526e..ae8267c8c38 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java @@ -68,7 +68,7 @@ public void execute(Client client, Map params, RestChannel chann "Malformed cursor: unable to extract cursor information"), RestStatus.BAD_REQUEST.getStatus()) .toString())); - } catch (OpenSearchException e) { + } catch (OpenSearchException e) { // RESUME: need to leverage this logic with error reports int status = (e.status().getStatus()); if (status > 399 && status < 500) { Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java index dab4b1e8ff1..8c786979b31 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java @@ -30,6 +30,8 @@ import org.opensearch.common.settings.Settings; import org.opensearch.index.IndexNotFoundException; import org.opensearch.index.IndexSettings; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.opensearch.mapping.IndexMapping; import org.opensearch.sql.opensearch.request.OpenSearchRequest; import org.opensearch.sql.opensearch.request.OpenSearchScrollRequest; @@ -99,7 +101,11 @@ public Map getIndexMappings(String... indexExpression) { Map.Entry::getKey, cursor -> new IndexMapping(cursor.getValue()))); } catch (IndexNotFoundException | OpenSearchSecurityException e) { // Re-throw directly to be treated as client error finally - throw e; + throw ErrorReport.wrap(e) + .code(ErrorCode.INDEX_NOT_FOUND) + .location("while fetching index mappings") + .context("index_name", indexExpression[0]) + .build(); } catch (Exception e) { throw new IllegalStateException( "Failed to read mapping for index pattern [" + indexExpression + "]", e); 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 4457c80a854..92855cc1f3c 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 @@ -5,8 +5,6 @@ package org.opensearch.sql.plugin.rest; -import static org.opensearch.core.rest.RestStatus.BAD_REQUEST; -import static org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.opensearch.core.rest.RestStatus.OK; import com.google.common.collect.ImmutableList; @@ -25,11 +23,9 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.common.error.ErrorReportStatusCodeHelper; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.datasources.exceptions.DataSourceClientException; -import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.QueryEngineException; -import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.legacy.metrics.MetricName; import org.opensearch.sql.legacy.metrics.Metrics; import org.opensearch.sql.opensearch.response.error.ErrorMessageFactory; @@ -50,20 +46,39 @@ public RestPPLQueryAction() { super(); } - private static boolean isClientError(Exception e) { - return ErrorReportStatusCodeHelper.isClientError( - e, - ex -> - ex instanceof NullPointerException - // NPE is hard to differentiate but more likely caused by bad query - || ex instanceof IllegalArgumentException - || ex instanceof IndexNotFoundException - || ex instanceof SemanticCheckException - || ex instanceof ExpressionEvaluationException - || ex instanceof QueryEngineException - || ex instanceof SyntaxCheckException - || ex instanceof DataSourceClientException - || ex instanceof IllegalAccessException); + private static boolean isClientError(Exception ex) { + return ex instanceof IllegalArgumentException + || ex instanceof IndexNotFoundException + || ex instanceof QueryEngineException + || ex instanceof SyntaxCheckException + || ex instanceof DataSourceClientException + || ex instanceof IllegalAccessException; + } + + private static int getRawErrorCode(Exception ex) { + if (ex instanceof ErrorReport) { + return getRawErrorCode(((ErrorReport) ex).getCause()); + } + if (ex instanceof OpenSearchException) { + return ((OpenSearchException) ex).status().getStatus(); + } + if (isClientError(ex)) { + return 400; + } + return 500; + } + + private static RestStatus loggedErrorCode(Exception ex) { + int code = getRawErrorCode(ex); + + // If we hit neither branch, no-op as false alarm error? I don't believe we can ever hit this + // scenario. + if (400 <= code && code < 500) { + Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS).increment(); + } else if (500 <= code && code < 600) { + Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS).increment(); + } + return RestStatus.fromCode(code); } @Override @@ -102,33 +117,13 @@ public void onResponse(TransportPPLQueryResponse response) { @Override public void onFailure(Exception e) { + RestStatus status = loggedErrorCode(e); if (transportPPLQueryRequest.isExplainRequest()) { - LOG.error("Error happened during explain", e); - if (isClientError(e)) { - reportError(channel, e, BAD_REQUEST); - } else { - reportError(channel, e, INTERNAL_SERVER_ERROR); - } - } else if (e instanceof OpenSearchException) { - Metrics.getInstance() - .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS) - .increment(); - OpenSearchException exception = (OpenSearchException) e; - reportError(channel, exception, exception.status()); + LOG.error("Error happened during explain (status {})", status, e); } else { - LOG.error("Error happened during query handling", e); - if (isClientError(e)) { - Metrics.getInstance() - .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS) - .increment(); - reportError(channel, e, BAD_REQUEST); - } else { - Metrics.getInstance() - .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS) - .increment(); - reportError(channel, e, INTERNAL_SERVER_ERROR); - } + LOG.error("Error happened during query handling (status {})", status, e); } + reportError(channel, e, status); } }); } From b34528dfa1f670b8512bac3e093b0eea738121c6 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 12:04:11 -0700 Subject: [PATCH 07/13] Fix some tests Signed-off-by: Simeon Widdis --- .../opensearch/client/OpenSearchNodeClientTest.java | 10 ++++++++-- .../sql/ppl/calcite/CalcitePPLBasicTest.java | 5 +++-- .../opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java | 5 +++-- .../sql/ppl/calcite/CalcitePPLFieldFormatTest.java | 5 +++-- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java index 81261aa7a70..3cca018548f 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java @@ -66,6 +66,7 @@ import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.data.model.ExprIntegerValue; import org.opensearch.sql.data.model.ExprTupleValue; import org.opensearch.sql.data.model.ExprValue; @@ -244,7 +245,12 @@ void get_index_mappings_with_IOException() { @Test void get_index_mappings_with_index_patterns() { mockNodeClientIndicesMappings("", null); - assertThrows(IndexNotFoundException.class, () -> client.getIndexMappings("test*")); + ErrorReport report = assertThrows(ErrorReport.class, () -> client.getIndexMappings("test*")); + assertTrue( + report.getMessage().contains("test*") && report.getMessage().contains("no such index"), + "expected index-not-found error message \"" + + report.getMessage() + + "\" to resemble \"no such index [index]\""); } @Test @@ -252,7 +258,7 @@ void get_index_mappings_with_non_exist_index() { when(nodeClient.admin().indices().prepareGetMappings(any()).setLocal(anyBoolean()).get()) .thenThrow(IndexNotFoundException.class); - assertThrows(IndexNotFoundException.class, () -> client.getIndexMappings("non_exist_index")); + assertThrows(ErrorReport.class, () -> client.getIndexMappings("non_exist_index")); } @Test diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java index 784fedc2ede..472e77e2d29 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBasicTest.java @@ -14,6 +14,7 @@ import org.apache.calcite.test.CalciteAssert; import org.junit.Ignore; import org.junit.Test; +import org.opensearch.sql.common.error.ErrorReport; public class CalcitePPLBasicTest extends CalcitePPLAbstractTest { @@ -201,9 +202,9 @@ public void testFieldsPlusThenMinus() { @Test public void testFieldsMinusThenPlusShouldThrowException() { String ppl = "source=EMP | fields - DEPTNO, SAL | fields + EMPNO, DEPTNO, SAL"; - IllegalArgumentException e = + ErrorReport e = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> { RelNode root = getRelNode(ppl); }); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java index 70b53d3c6fc..9b37ab5b407 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLEvalTest.java @@ -12,6 +12,7 @@ import org.apache.calcite.rel.RelNode; import org.apache.calcite.test.CalciteAssert; import org.junit.Test; +import org.opensearch.sql.common.error.ErrorReport; public class CalcitePPLEvalTest extends CalcitePPLAbstractTest { @@ -337,9 +338,9 @@ public void testComplexEvalCommands4() { "source=EMP | eval col1 = SAL | sort - col1 | head 3 | fields ENAME, col1 | eval col2 =" + " col1 | sort + col2 | fields ENAME, col2 | eval col3 = col2 | head 2 | fields" + " HIREDATE, col3"; - IllegalArgumentException e = + ErrorReport e = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> { RelNode root = getRelNode(ppl); }); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFieldFormatTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFieldFormatTest.java index e20bd1b0e47..5bef9c397eb 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFieldFormatTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFieldFormatTest.java @@ -12,6 +12,7 @@ import org.apache.calcite.rel.RelNode; import org.apache.calcite.test.CalciteAssert; import org.junit.Test; +import org.opensearch.sql.common.error.ErrorReport; public class CalcitePPLFieldFormatTest extends CalcitePPLAbstractTest { @@ -218,9 +219,9 @@ public void testComplexFieldFormatCommands4() { "source=EMP | fieldformat col1 = SAL | sort - col1 | head 3 | fields ENAME, col1 |" + " fieldformat col2 = col1 | sort + col2 | fields ENAME, col2 | fieldformat col3 =" + " col2 | head 2 | fields HIREDATE, col3"; - IllegalArgumentException e = + ErrorReport e = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> { RelNode root = getRelNode(ppl); }); From 309936e5ceb361ec5c2924af54717a08b392f03a Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 12:19:42 -0700 Subject: [PATCH 08/13] Fix some more tests Signed-off-by: Simeon Widdis --- common/build.gradle | 11 +++++++++++ .../opensearch/sql/common/error/ErrorReportTest.java | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/common/build.gradle b/common/build.gradle index d839466f886..233eb30d797 100644 --- a/common/build.gradle +++ b/common/build.gradle @@ -32,6 +32,15 @@ repositories { mavenCentral() } +test { + maxParallelForks = Runtime.runtime.availableProcessors() + useJUnitPlatform() + testLogging { + events "passed", "skipped", "failed" + exceptionFormat "full" + } +} + dependencies { api "org.antlr:antlr4-runtime:4.13.2" api group: 'com.google.guava', name: 'guava', version: "${guava_version}" @@ -52,6 +61,8 @@ dependencies { testImplementation group: 'org.mockito', name: 'mockito-core', version: "${mockito_version}" testImplementation group: 'org.mockito', name: 'mockito-junit-jupiter', version: "${mockito_version}" testImplementation group: 'com.squareup.okhttp3', name: 'mockwebserver', version: '4.12.0' + + testRuntimeOnly('org.junit.platform:junit-platform-launcher') } diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java index 0bf2568f9fa..85e3fd67dc3 100644 --- a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -141,7 +141,7 @@ public void testToDetailedMessage() { String message = report.toDetailedMessage(); assertTrue(message.contains("FIELD_NOT_FOUND")); - assertTrue(message.contains("Semantic Analysis")); + assertTrue(message.contains("Validating the query")); assertTrue(message.contains("Field not found")); assertTrue(message.contains("while resolving fields")); assertTrue(message.contains("field_name")); From 9ae8b8fdd24a896cd85efc2cd3f875824482d5e5 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 13:45:00 -0700 Subject: [PATCH 09/13] Fully fix: unit tests, yaml tests, doctests Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorReport.java | 12 +- .../sql/calcite/CalciteRelNodeVisitor.java | 24 +- .../opensearch/sql/executor/QueryService.java | 36 +-- .../expression/parse/RegexCommonUtils.java | 16 +- .../parse/RegexCommonUtilsTest.java | 61 +++--- docs/user/ppl/cmd/mvcombine.md | 2 +- docs/user/ppl/cmd/mvexpand.md | 2 +- docs/user/ppl/cmd/rex.md | 2 +- docs/user/ppl/interfaces/protocol.md | 13 +- .../sql/ppl/ErrorReportStatusCodeIT.java | 205 ------------------ .../rest-api-spec/test/ppl/error_handling.yml | 116 ++++++++++ 11 files changed, 220 insertions(+), 269 deletions(-) delete mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/error_handling.yml diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java index f2072da2e55..25f52956f0e 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -112,7 +112,13 @@ public String toDetailedMessage() { if (!locationChain.isEmpty()) { sb.append("\nLocation chain:\n"); for (int i = 0; i < locationChain.size(); i++) { - sb.append(" ").append(i + 1).append(". ").append(locationChain.get(i)).append("\n"); + // The location chain is typically appended to as we traverse up the stack, but for reading + // the error it makes more sense to go down the stack. So we reverse it. + sb.append(" ") + .append(i + 1) + .append(". ") + .append(locationChain.get(locationChain.size() - i - 1)) + .append("\n"); } } @@ -148,7 +154,9 @@ public Map toJsonMap() { } if (!locationChain.isEmpty()) { - json.put("location", new ArrayList<>(locationChain)); + // The location chain is typically appended to as we traverse up the stack, but for reading + // the error it makes more sense to go down the stack. So we reverse it. + json.put("location", locationChain.reversed()); } // Build context with stage information included 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 66f4adfa73b..b4eaafa7326 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -174,6 +174,8 @@ import org.opensearch.sql.calcite.utils.PlanUtils; import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils; import org.opensearch.sql.calcite.utils.WildcardUtils; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.patterns.PatternUtils; import org.opensearch.sql.common.utils.StringUtils; import org.opensearch.sql.datasource.DataSourceService; @@ -344,7 +346,7 @@ public RelNode visitRegex(Regex node, CalcitePlanContext context) { return context.relBuilder.peek(); } - public RelNode visitRex(Rex node, CalcitePlanContext context) { + private RelNode innerRex(Rex node, CalcitePlanContext context) { visitChildren(node, context); RexNode fieldRex = rexVisitor.analyze(node.getField(), context); @@ -409,6 +411,17 @@ public RelNode visitRex(Rex node, CalcitePlanContext context) { return context.relBuilder.peek(); } + public RelNode visitRex(Rex node, CalcitePlanContext context) { + try { + return innerRex(node, context); + } catch (RuntimeException ex) { + throw ErrorReport.wrap(ex) + .location("while processing the rex command") + .context("command", "rex") + .build(); + } + } + private boolean containsSubqueryExpression(Node expr) { if (expr == null) { return false; @@ -3717,8 +3730,13 @@ public RelNode visitMvExpand(MvExpand mvExpand, CalcitePlanContext context) { inputType.getField(fieldName, /*caseSensitive*/ true, /*elideRecord*/ false); if (inputField == null) { - throw new SemanticCheckException( - String.format("Field '%s' not found in the schema", fieldName)); + throw ErrorReport.wrap( + new SemanticCheckException( + String.format("Field '%s' not found in the schema", fieldName))) + .code(ErrorCode.FIELD_NOT_FOUND) + .location("while evaluating the input field for mvexpand") + .context("command", "mvexpand") + .build(); } final RexInputRef arrayFieldRex = (RexInputRef) rexVisitor.analyze(field, context); diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java index d63a76afcc7..3b5de8a1f42 100644 --- a/core/src/main/java/org/opensearch/sql/executor/QueryService.java +++ b/core/src/main/java/org/opensearch/sql/executor/QueryService.java @@ -35,6 +35,7 @@ import org.opensearch.sql.calcite.SysLimit; import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit; import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit.SystemLimitType; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.common.error.QueryProcessingStage; import org.opensearch.sql.common.error.StageErrorHandler; import org.opensearch.sql.common.response.ResponseListener; @@ -152,14 +153,14 @@ public void executeWithCalcite( StageErrorHandler.executeStage( QueryProcessingStage.ANALYZING, () -> analyze(plan, context), - "checking if fields and indices exist"); + "while preparing and validating the query plan"); // Wrap plan conversion with PLAN_CONVERSION stage tracking RelNode calcitePlan = StageErrorHandler.executeStage( QueryProcessingStage.PLAN_CONVERSION, () -> convertToCalcitePlan(relNode, context), - "preparing to execute your query"); + "while preparing to execute the query"); analyzeMetric.set(System.nanoTime() - analyzeStart); @@ -167,7 +168,7 @@ public void executeWithCalcite( StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, () -> executionEngine.execute(calcitePlan, context, listener), - "running query on your cluster"); + "while running the query on the cluster"); } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); @@ -312,22 +313,31 @@ public PhysicalPlan plan(LogicalPlan plan) { return planner.plan(plan); } + private boolean isCalciteUnsupportedError(@Nullable Throwable t) { + return switch (t) { + case null -> false; + case CalciteUnsupportedException calciteUnsupportedException -> true; + case ErrorReport errorReport when t.getCause() instanceof CalciteUnsupportedException -> true; + default -> false; + }; + } + private boolean isCalciteFallbackAllowed(@Nullable Throwable t) { // We always allow fallback the query failed with CalciteUnsupportedException. // This is for avoiding breaking changes when enable Calcite by default. - if (t instanceof CalciteUnsupportedException) { + if (isCalciteUnsupportedError(t)) { return true; - } else { - if (settings != null) { - Boolean fallback_allowed = settings.getSettingValue(Settings.Key.CALCITE_FALLBACK_ALLOWED); - if (fallback_allowed == null) { - return false; - } - return fallback_allowed; - } else { - return true; + } + + if (settings != null) { + Boolean fallback_allowed = settings.getSettingValue(Settings.Key.CALCITE_FALLBACK_ALLOWED); + if (fallback_allowed == null) { + return false; } + return fallback_allowed; } + + return true; } private boolean isCalciteEnabled(Settings settings) { diff --git a/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java b/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java index 7e194dfbf22..599f0cce410 100644 --- a/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java +++ b/core/src/main/java/org/opensearch/sql/expression/parse/RegexCommonUtils.java @@ -13,6 +13,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; /** * Common utilities for regex operations. Provides pattern caching and consistent matching behavior. @@ -69,11 +71,15 @@ public static List getNamedGroupCandidates(String pattern) { String groupName = anyGroupMatcher.group(1); if (!isValidJavaRegexGroupName(groupName)) { - throw new IllegalArgumentException( - String.format( - "Invalid capture group name '%s'. Java regex group names must start with a letter" - + " and contain only letters and digits.", - groupName)); + throw ErrorReport.wrap( + new IllegalArgumentException( + String.format("Invalid capture group name '%s'.", groupName))) + .code(ErrorCode.SYNTAX_ERROR) + .location("while validating the capture groups for the pattern") + .suggestion( + "Java Regex capture groups must be alphanumeric and start with a letter. Update the" + + " capture group to be alphanumeric.") + .build(); } } diff --git a/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java b/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java index 2503b3929f1..e20c149d86b 100644 --- a/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/parse/RegexCommonUtilsTest.java @@ -11,6 +11,7 @@ import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; import org.junit.jupiter.api.Test; +import org.opensearch.sql.common.error.ErrorReport; public class RegexCommonUtilsTest { @@ -197,10 +198,8 @@ public void testGetNamedGroupCandidatesWithNumericNames() { public void testGetNamedGroupCandidatesWithInvalidCharactersThrowsException() { // Test that groups with invalid characters throw exception (even if some are valid) String pattern = "(?[a-z]+)\\s+(?<123invalid>[0-9]+)\\s+(?.*)"; - IllegalArgumentException exception = - assertThrows( - IllegalArgumentException.class, - () -> RegexCommonUtils.getNamedGroupCandidates(pattern)); + ErrorReport exception = + assertThrows(ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(pattern)); // Should fail on the first invalid group name found assertTrue(exception.getMessage().contains("Invalid capture group name")); } @@ -217,74 +216,65 @@ public void testGetNamedGroupCandidatesValidAlphanumeric() { @Test public void testGetNamedGroupCandidatesWithUnderscore() { - // Test that underscores in named groups throw IllegalArgumentException + // Test that underscores in named groups throw ErrorReport String patternWithUnderscore = ".+@(?.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithUnderscore)); assertTrue(exception.getMessage().contains("Invalid capture group name 'domain_name'")); - assertTrue( - exception - .getMessage() - .contains("must start with a letter and contain only letters and digits")); + assertTrue(exception.getSuggestion().contains("must be alphanumeric")); } @Test public void testGetNamedGroupCandidatesWithHyphen() { - // Test that hyphens in named groups throw IllegalArgumentException + // Test that hyphens in named groups throw ErrorReport String patternWithHyphen = ".+@(?.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, - () -> RegexCommonUtils.getNamedGroupCandidates(patternWithHyphen)); + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithHyphen)); assertTrue(exception.getMessage().contains("Invalid capture group name 'domain-name'")); - assertTrue( - exception - .getMessage() - .contains("must start with a letter and contain only letters and digits")); + assertTrue(exception.getSuggestion().contains("must be alphanumeric")); } @Test public void testGetNamedGroupCandidatesWithDot() { - // Test that dots in named groups throw IllegalArgumentException + // Test that dots in named groups throw ErrorReport String patternWithDot = ".+@(?.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, - () -> RegexCommonUtils.getNamedGroupCandidates(patternWithDot)); + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithDot)); assertTrue(exception.getMessage().contains("Invalid capture group name 'domain.name'")); } @Test public void testGetNamedGroupCandidatesWithSpace() { - // Test that spaces in named groups throw IllegalArgumentException + // Test that spaces in named groups throw ErrorReport String patternWithSpace = ".+@(?.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, - () -> RegexCommonUtils.getNamedGroupCandidates(patternWithSpace)); + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithSpace)); assertTrue(exception.getMessage().contains("Invalid capture group name 'domain name'")); } @Test public void testGetNamedGroupCandidatesStartingWithDigit() { - // Test that group names starting with digit throw IllegalArgumentException + // Test that group names starting with digit throw ErrorReport String patternStartingWithDigit = ".+@(?<1domain>.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternStartingWithDigit)); assertTrue(exception.getMessage().contains("Invalid capture group name '1domain'")); } @Test public void testGetNamedGroupCandidatesWithSpecialCharacters() { - // Test that special characters in named groups throw IllegalArgumentException + // Test that special characters in named groups throw ErrorReport String patternWithSpecialChar = ".+@(?.+)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithSpecialChar)); assertTrue(exception.getMessage().contains("Invalid capture group name 'domain@name'")); } @@ -304,10 +294,9 @@ public void testGetNamedGroupCandidatesWithMixedInvalidValid() { // Test that even one invalid group name fails the entire validation String patternWithMixed = "(?[a-z]+)\\s+(?[0-9]+)\\s+(?.*)"; - IllegalArgumentException exception = + ErrorReport exception = assertThrows( - IllegalArgumentException.class, - () -> RegexCommonUtils.getNamedGroupCandidates(patternWithMixed)); + ErrorReport.class, () -> RegexCommonUtils.getNamedGroupCandidates(patternWithMixed)); assertTrue(exception.getMessage().contains("Invalid capture group name 'invalid_name'")); } } diff --git a/docs/user/ppl/cmd/mvcombine.md b/docs/user/ppl/cmd/mvcombine.md index 4ccad724ca7..d8fa63ada67 100644 --- a/docs/user/ppl/cmd/mvcombine.md +++ b/docs/user/ppl/cmd/mvcombine.md @@ -124,6 +124,6 @@ source=mvcombine_data Expected output: ```text -{'reason': 'Invalid Query', 'details': 'Field [does_not_exist] not found.', 'type': 'IllegalArgumentException'} +{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query'}, 'details': 'Field [does_not_exist] not found.', 'location': ['while preparing and validating the query plan'], 'code': 'FIELD_NOT_FOUND', 'type': 'IllegalArgumentException'} Error: Query returned no data ``` \ No newline at end of file diff --git a/docs/user/ppl/cmd/mvexpand.md b/docs/user/ppl/cmd/mvexpand.md index 6fdd9bca365..a2c418691ba 100644 --- a/docs/user/ppl/cmd/mvexpand.md +++ b/docs/user/ppl/cmd/mvexpand.md @@ -132,6 +132,6 @@ source=people Expected output: ```text -{'reason': 'Invalid Query', 'details': "Field 'tags' not found in the schema", 'type': 'SemanticCheckException'} +{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query', 'command': 'mvexpand'}, 'details': "Field 'tags' not found in the schema", 'location': ['while preparing and validating the query plan', 'while evaluating the input field for mvexpand'], 'code': 'FIELD_NOT_FOUND', 'type': 'SemanticCheckException'} Error: Query returned no data ``` \ No newline at end of file diff --git a/docs/user/ppl/cmd/rex.md b/docs/user/ppl/cmd/rex.md index b4fe706f489..b8a16128b00 100644 --- a/docs/user/ppl/cmd/rex.md +++ b/docs/user/ppl/cmd/rex.md @@ -228,7 +228,7 @@ source=accounts The query returns the following results: ```text -{'reason': 'Invalid Query', 'details': "Invalid capture group name 'user_name'. Java regex group names must start with a letter and contain only letters and digits.", 'type': 'IllegalArgumentException'} +{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query', 'command': 'rex'}, 'details': "Invalid capture group name 'user_name'.", 'location': ['while preparing and validating the query plan', 'while processing the rex command', 'while validating the capture groups for the pattern'], 'code': 'SYNTAX_ERROR', 'type': 'IllegalArgumentException', 'suggestion': 'Java Regex capture groups must be alphanumeric and start with a letter. Update the capture group to be alphanumeric.'} Error: Query returned no data ``` diff --git a/docs/user/ppl/interfaces/protocol.md b/docs/user/ppl/interfaces/protocol.md index 680f01fd379..f7c57171031 100644 --- a/docs/user/ppl/interfaces/protocol.md +++ b/docs/user/ppl/interfaces/protocol.md @@ -120,8 +120,17 @@ Expected output: ```json { "error": { - "reason": "Error occurred in OpenSearch engine: no such index [unknown]", - "details": "[unknown] IndexNotFoundException[no such index [unknown]]\nFor more details, please send request for Json format to see the raw response from OpenSearch engine.", + "context": { + "stage": "analyzing", + "index_name": "unknown", + "stage_description": "Validating the query" + }, + "details": "no such index [unknown]", + "location": [ + "while preparing and validating the query plan", + "while fetching index mappings" + ], + "code": "INDEX_NOT_FOUND", "type": "IndexNotFoundException" }, "status": 404 diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java deleted file mode 100644 index d3a2cf91ab3..00000000000 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/ErrorReportStatusCodeIT.java +++ /dev/null @@ -1,205 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.ppl; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.not; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; - -import java.io.IOException; -import org.json.JSONObject; -import org.junit.Test; -import org.opensearch.client.ResponseException; -import org.opensearch.sql.calcite.standalone.CalcitePPLIntegTestCase; -import org.opensearch.sql.util.TestUtils; - -/** - * Integration tests for ErrorReport status code handling. Validates that exceptions wrapped in - * ErrorReport are correctly mapped to HTTP status codes based on the ErrorCode. - */ -public class ErrorReportStatusCodeIT extends CalcitePPLIntegTestCase { - - @Override - public void init() throws IOException { - super.init(); - loadIndex(Index.ACCOUNT); - } - - @Test - public void testFieldNotFoundReturns400() throws IOException { - // Field not found should be a client error (400 BAD_REQUEST) - ResponseException exception = - assertThrows( - ResponseException.class, - () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field")); - - assertThat( - "Field not found should return 400 status", - exception.getResponse().getStatusLine().getStatusCode(), - equalTo(400)); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - - assertThat("Response should have error object", response.has("error"), equalTo(true)); - JSONObject error = response.getJSONObject("error"); - - // Verify the error includes the ErrorCode - if (error.has("code")) { - assertThat( - "Error code should be FIELD_NOT_FOUND", - error.getString("code"), - equalTo("FIELD_NOT_FOUND")); - } - } - - @Test - public void testIndexNotFoundReturns404() throws IOException { - // Index not found should return 404 NOT_FOUND - ResponseException exception = - assertThrows( - ResponseException.class, () -> executeQuery("source=nonexistent_index | fields age")); - - assertThat( - "Index not found should return 404 status", - exception.getResponse().getStatusLine().getStatusCode(), - equalTo(404)); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - - assertThat("Response should have error object", response.has("error"), equalTo(true)); - JSONObject error = response.getJSONObject("error"); - - // Verify error details mention the index - assertThat( - "Error details should mention nonexistent index", - error.getString("details"), - containsString("nonexistent_index")); - } - - @Test - public void testSyntaxErrorReturns400() throws IOException { - // Syntax error should be a client error (400 BAD_REQUEST) - ResponseException exception = - assertThrows( - ResponseException.class, - () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | invalid_command")); - - assertThat( - "Syntax error should return 400 status", - exception.getResponse().getStatusLine().getStatusCode(), - equalTo(400)); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - - assertThat("Response should have error object", response.has("error"), equalTo(true)); - } - - @Test - public void testSemanticErrorReturns400() throws IOException { - // Semantic errors (like type mismatches) should be client errors (400 BAD_REQUEST) - ResponseException exception = - assertThrows( - ResponseException.class, - () -> - executeQuery( - "source=" + TEST_INDEX_ACCOUNT + " | where age + 'string' > 10 | fields age")); - - assertThat( - "Semantic error should return 400 status", - exception.getResponse().getStatusLine().getStatusCode(), - equalTo(400)); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - - assertThat("Response should have error object", response.has("error"), equalTo(true)); - } - - @Test - public void testErrorReportPreservesContextInformation() throws IOException { - // Verify that ErrorReport context is included in the response - ResponseException exception = - assertThrows( - ResponseException.class, - () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field")); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - JSONObject error = response.getJSONObject("error"); - - // ErrorReport should include context information - if (error.has("context")) { - JSONObject context = error.getJSONObject("context"); - assertTrue( - "Context should include stage information", - context.has("stage") || context.has("stage_description")); - } - - // ErrorReport should include location chain if available - if (error.has("location")) { - assertTrue( - "Location should be an array", error.get("location") instanceof org.json.JSONArray); - } - } - - @Test - public void testErrorReportTypeIsErrorCodeNotExceptionClass() throws IOException { - // Verify that the "type" field is the ErrorCode, not "ErrorReport" - ResponseException exception = - assertThrows( - ResponseException.class, - () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field")); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - JSONObject error = response.getJSONObject("error"); - - // The type should be the ErrorCode, not "ErrorReport" - assertTrue("Error should have a type field", error.has("type")); - String type = error.getString("type"); - assertThat( - "Type should be an ErrorCode (e.g., FIELD_NOT_FOUND), not ErrorReport", - type, - not(equalTo("ErrorReport"))); - - // Context should include the underlying cause exception type - if (error.has("context")) { - JSONObject context = error.getJSONObject("context"); - assertTrue( - "Context should include the underlying cause exception type", context.has("cause")); - } - } - - @Test - public void testErrorReportContextIncludesCause() throws IOException { - // Verify that when ErrorReport is used, the context includes the underlying cause exception - // type - ResponseException exception = - assertThrows( - ResponseException.class, - () -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field")); - - String responseBody = TestUtils.getResponseBody(exception.getResponse()); - JSONObject response = new JSONObject(responseBody); - JSONObject error = response.getJSONObject("error"); - - // If the error has context (meaning it's wrapped in ErrorReport with stage info), - // then it should include the cause - if (error.has("context")) { - JSONObject context = error.getJSONObject("context"); - assertTrue("Context should include cause field", context.has("cause")); - - // The cause should be a simple class name (not null or empty) - String cause = context.getString("cause"); - assertFalse("Cause should not be empty", cause.isEmpty()); - assertFalse("Cause should not contain package names", cause.contains(".")); - } - } -} diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/error_handling.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/error_handling.yml new file mode 100644 index 00000000000..74219956c3c --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/error_handling.yml @@ -0,0 +1,116 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + +--- +"Test field not found returns 400 bad_request": + - skip: + features: + - headers + - allowed_warnings + - do: + bulk: + index: test_error_handling + refresh: true + body: + - '{"index": {}}' + - '{"age": 25, "name": "John"}' + - '{"index": {}}' + - '{"age": 30, "name": "Jane"}' + + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_error_handling | fields nonexistent_field + - match: {"$body": "/[Ff]ield|[Cc]olumn/"} + +--- +"Test index not found returns 404 missing": + - skip: + features: + - headers + - do: + catch: missing + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=nonexistent_index_12345 | fields age + +--- +"Test syntax error returns 400 bad_request": + - skip: + features: + - headers + - do: + bulk: + index: test_error_syntax + refresh: true + body: + - '{"index": {}}' + - '{"age": 25}' + + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_error_syntax | invalid_command_xyz + +--- +"Test semantic error returns 400 bad_request": + - skip: + features: + - headers + - do: + bulk: + index: test_error_semantic + refresh: true + body: + - '{"index": {}}' + - '{"age": 25, "name": "John"}' + + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_error_semantic | where age IN ('a', 'b', 'c') | fields age + +--- +"Test aggregation validation error returns 400 bad_request": + - skip: + features: + - headers + - do: + bulk: + index: test_error_agg + refresh: true + body: + - '{"index": {}}' + - '{"age": 25, "name": "John"}' + + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_error_agg | stats count(eval(age)) as cnt + - match: {"$body": "/[Cc]ondition.*boolean|[Bb]oolean.*expected/"} From 0ad2a951eca2d5fe33d695dc3e70cf01b32d88c4 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 16:06:30 -0700 Subject: [PATCH 10/13] Fix Calcite integ tests Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorCode.java | 10 ++- .../sql/calcite/utils/CalciteToolsHelper.java | 70 ++++++++++++++++--- .../opensearch/sql/executor/QueryService.java | 4 +- .../remote/CalcitePPLNestedAggregationIT.java | 3 +- .../calcite/remote/CalciteParseCommandIT.java | 23 +++--- .../calcite/remote/CalciteRexCommandIT.java | 14 ++-- .../CalciteEnumerableNestedAggregate.java | 3 +- 7 files changed, 90 insertions(+), 37 deletions(-) diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java index 95e81d04b71..3a4ac7b287d 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java @@ -7,7 +7,10 @@ /** * Machine-readable error codes for categorizing exceptions. These codes help clients handle - * specific error types programmatically. + * specific error types programmatically.
+ *
+ * Not a complete list, currently seeded with some initial values. Feel free to add variants or + * remove dead variants over time. */ public enum ErrorCode { /** Field not found in the index mapping */ @@ -43,6 +46,9 @@ public enum ErrorCode { /** Query execution failed */ EXECUTION_ERROR, - /** Unknown or unclassified error */ + /** + * Unknown or unclassified error -- don't set this manually, it's filled in as the default if no + * other code applies. + */ UNKNOWN } diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java index a6d57ea01f6..e5f3ecc5021 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java @@ -30,6 +30,7 @@ import static java.util.Objects.requireNonNull; import static org.opensearch.sql.monitor.profile.MetricName.OPTIMIZE; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import java.lang.reflect.Type; import java.sql.Connection; @@ -104,6 +105,8 @@ import org.opensearch.sql.calcite.plan.rule.OpenSearchRules; import org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule; import org.opensearch.sql.calcite.profile.PlanProfileBuilder; +import org.opensearch.sql.common.error.ErrorCode; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.expression.function.PPLBuiltinOperators; import org.opensearch.sql.monitor.profile.ProfileContext; import org.opensearch.sql.monitor.profile.ProfileMetric; @@ -398,6 +401,54 @@ protected RelFieldTrimmer newFieldTrimmer() { } public static class OpenSearchRelRunners { + private static boolean isNonPushdownEnumerableAggregate(String message) { + return message.contains("Error while preparing plan") + && message.contains("CalciteEnumerableNestedAggregate"); + } + + // Detect if error is due to window functions in unsupported context (bins on time fields) + private static boolean isWindowBinOnTimeField(SQLException e) { + String errorMsg = e.getMessage(); + return errorMsg != null + && errorMsg.contains("Error while preparing plan") + && errorMsg.contains("WIDTH_BUCKET"); + } + + // Traverse Calcite SQL exceptions in search of the root cause, since Calcite's outer error + // messages aren't really usable for users + private static String rootCauseMessage(Throwable e) { + String rc = null; + if (e.getCause() != null) { + rc = rootCauseMessage(e.getCause()); + } + for (int i = 0; rc == null && i < e.getSuppressed().length; i++) { + rc = rootCauseMessage(e.getSuppressed()[i]); + } + return rc != null ? rc : e.getMessage(); + } + + private static void enrichErrorsForSpecialCases(ErrorReport.Builder report, SQLException e) { + if (e.getMessage().contains("Error while preparing plan [") && e.getCause() != null) { + // Generic 'something went wrong' planning error, try to get the cause + int planStart = e.getMessage().indexOf('['); + int planEnd = e.getMessage().lastIndexOf(']'); + report + .context("plan", e.getMessage().substring(planStart + 1, planEnd)) + .details(rootCauseMessage(e)); + } + if (isWindowBinOnTimeField(e)) { + report + .details( + "The 'bins' parameter on timestamp fields requires: (1) pushdown to be enabled" + + " (controlled by plugins.calcite.pushdown.enabled, enabled by default), and" + + " (2) the timestamp field to be used as an aggregation bucket (e.g., 'stats" + + " count() by @timestamp').") + .code(ErrorCode.UNSUPPORTED_OPERATION) + .context("is_window_bin_on_time_field", true) + .suggestion("check pushdown is enabled and review the aggregation"); + } + } + /** * Runs a relational expression by existing connection. This class copied from {@link * org.apache.calcite.tools.RelRunners#run(RelNode)} @@ -429,18 +480,15 @@ public RelNode visit(TableScan scan) { optimizeTime.set(System.nanoTime() - startTime); return preparedStatement; } catch (SQLException e) { + Throwables.throwIfUnchecked(e); + // Detect if error is due to window functions in unsupported context (bins on time fields) - String errorMsg = e.getMessage(); - if (errorMsg != null - && errorMsg.contains("Error while preparing plan") - && errorMsg.contains("WIDTH_BUCKET")) { - throw new UnsupportedOperationException( - "The 'bins' parameter on timestamp fields requires: (1) pushdown to be enabled" - + " (controlled by plugins.calcite.pushdown.enabled, enabled by default), and" - + " (2) the timestamp field to be used as an aggregation bucket (e.g., 'stats" - + " count() by @timestamp')."); - } - throw Util.throwAsRuntime(e); + ErrorReport.Builder report = + ErrorReport.wrap(e) + .location("while compiling the optimized query plan for physical execution") + .code(ErrorCode.PLANNING_ERROR); + enrichErrorsForSpecialCases(report, e); + throw report.build(); } } } diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java index 3b5de8a1f42..b294a168cd8 100644 --- a/core/src/main/java/org/opensearch/sql/executor/QueryService.java +++ b/core/src/main/java/org/opensearch/sql/executor/QueryService.java @@ -160,7 +160,7 @@ public void executeWithCalcite( StageErrorHandler.executeStage( QueryProcessingStage.PLAN_CONVERSION, () -> convertToCalcitePlan(relNode, context), - "while preparing to execute the query"); + "while converting the query to an executable plan"); analyzeMetric.set(System.nanoTime() - analyzeStart); @@ -168,7 +168,7 @@ public void executeWithCalcite( StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, () -> executionEngine.execute(calcitePlan, context, listener), - "while running the query on the cluster"); + "while running the query"); } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLNestedAggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLNestedAggregationIT.java index faaae541d1e..c7ec6434744 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLNestedAggregationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLNestedAggregationIT.java @@ -17,6 +17,7 @@ import java.io.IOException; import org.json.JSONObject; import org.junit.jupiter.api.Test; +import org.opensearch.sql.common.error.ErrorReport; import org.opensearch.sql.ppl.PPLIntegTestCase; public class CalcitePPLNestedAggregationIT extends PPLIntegTestCase { @@ -175,7 +176,7 @@ public void testNestedAggregationThrowExceptionIfPushdownCannotApplied() throws enabledOnlyWhenPushdownIsEnabled(); Throwable t = assertThrowsWithReplace( - UnsupportedOperationException.class, + ErrorReport.class, () -> executeQuery( String.format( diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java index e25470a6e53..d5030ffa181 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteParseCommandIT.java @@ -9,9 +9,12 @@ import java.io.IOException; import org.junit.Test; +import org.opensearch.client.ResponseException; import org.opensearch.sql.ppl.ParseCommandIT; public class CalciteParseCommandIT extends ParseCommandIT { + private static final String SUGGESTION_MATCHING_CONTENT = "capture groups must be alphanumeric"; + @Override public void init() throws Exception { super.init(); @@ -25,10 +28,9 @@ public void testParseErrorInvalidGroupNameUnderscore() throws IOException { String.format( "source=%s | parse email '.+@(?.+)' | fields email", TEST_INDEX_BANK)); fail("Should have thrown an exception for underscore in named capture group"); - } catch (Exception e) { + } catch (ResponseException e) { assertTrue(e.getMessage().contains("Invalid capture group name 'host_name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -39,10 +41,9 @@ public void testParseErrorInvalidGroupNameHyphen() throws IOException { String.format( "source=%s | parse email '.+@(?.+)' | fields email", TEST_INDEX_BANK)); fail("Should have thrown an exception for hyphen in named capture group"); - } catch (Exception e) { + } catch (ResponseException e) { assertTrue(e.getMessage().contains("Invalid capture group name 'host-name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -53,10 +54,9 @@ public void testParseErrorInvalidGroupNameStartingWithDigit() throws IOException String.format( "source=%s | parse email '.+@(?<1host>.+)' | fields email", TEST_INDEX_BANK)); fail("Should have thrown an exception for group name starting with digit"); - } catch (Exception e) { + } catch (ResponseException e) { assertTrue(e.getMessage().contains("Invalid capture group name '1host'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -67,10 +67,9 @@ public void testParseErrorInvalidGroupNameSpecialCharacter() throws IOException String.format( "source=%s | parse email '.+@(?.+)' | fields email", TEST_INDEX_BANK)); fail("Should have thrown an exception for special character in named capture group"); - } catch (Exception e) { + } catch (ResponseException e) { assertTrue(e.getMessage().contains("Invalid capture group name 'host@name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java index f7a50ee0676..eca08b1fc11 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteRexCommandIT.java @@ -14,6 +14,8 @@ import org.opensearch.sql.ppl.PPLIntegTestCase; public class CalciteRexCommandIT extends PPLIntegTestCase { + private static final String SUGGESTION_MATCHING_CONTENT = "capture groups must be alphanumeric"; + @Override public void init() throws Exception { super.init(); @@ -61,8 +63,7 @@ public void testRexErrorInvalidGroupNameUnderscore() throws IOException { fail("Should have thrown an exception for underscore in named capture group"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid capture group name 'user_name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -77,8 +78,7 @@ public void testRexErrorInvalidGroupNameHyphen() throws IOException { fail("Should have thrown an exception for hyphen in named capture group"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid capture group name 'user-name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -93,8 +93,7 @@ public void testRexErrorInvalidGroupNameStartingWithDigit() throws IOException { fail("Should have thrown an exception for group name starting with digit"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid capture group name '1user'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } @@ -109,8 +108,7 @@ public void testRexErrorInvalidGroupNameSpecialCharacter() throws IOException { fail("Should have thrown an exception for special character in named capture group"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid capture group name 'user@name'")); - assertTrue( - e.getMessage().contains("must start with a letter and contain only letters and digits")); + assertTrue(e.getMessage().contains(SUGGESTION_MATCHING_CONTENT)); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableNestedAggregate.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableNestedAggregate.java index ef569fc2989..58dc62a8469 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableNestedAggregate.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableNestedAggregate.java @@ -79,7 +79,8 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) { // TODO implement an enumerable nested aggregate throw new UnsupportedOperationException( String.format( - "Cannot execute nested aggregation on %s since pushdown cannot be applied.", aggCalls)); + "Cannot execute nested aggregation on %s since plugins.calcite.pushdown is disabled.", + aggCalls)); } @Override From 709e4f5e731d5ab6562ef25806330b49fca2530a Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Thu, 26 Mar 2026 16:59:09 -0700 Subject: [PATCH 11/13] Improve debug logging for DataSourceEnabledIT Signed-off-by: Simeon Widdis --- .../org/opensearch/sql/datasource/DataSourceEnabledIT.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceEnabledIT.java b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceEnabledIT.java index a53c04d8710..f014ab587de 100644 --- a/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceEnabledIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceEnabledIT.java @@ -9,6 +9,7 @@ import java.io.IOException; import lombok.SneakyThrows; +import org.apache.hc.core5.http.io.entity.EntityUtils; import org.json.JSONObject; import org.junit.After; import org.junit.Assert; @@ -150,8 +151,11 @@ private void assertDataSourceCount(int expected) { @SneakyThrows private Response performRequest(Request request) { try { - return client().performRequest(request); + Response response = client().performRequest(request); + System.err.println("Successful response: " + EntityUtils.toString(response.getEntity())); + return response; } catch (ResponseException e) { + System.err.println("Failed response: " + EntityUtils.toString(e.getResponse().getEntity())); return e.getResponse(); } } From d59d81a431716a728df62a9be8e225408e1e17bc Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 27 Mar 2026 10:27:42 -0700 Subject: [PATCH 12/13] Fix handling for some error reports that propagate to the legacy endpoint Signed-off-by: Simeon Widdis --- .../sql/legacy/plugin/RestSqlAction.java | 39 +++++++++++++------ .../sql/plugin/rest/RestPPLQueryAction.java | 2 + 2 files changed, 29 insertions(+), 12 deletions(-) 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 9be2367dcaa..676dee1751a 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 @@ -5,8 +5,6 @@ package org.opensearch.sql.legacy.plugin; -import static org.opensearch.core.rest.RestStatus.BAD_REQUEST; -import static org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.opensearch.core.rest.RestStatus.OK; import com.alibaba.druid.sql.parser.ParserException; @@ -33,6 +31,7 @@ import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestRequest; import org.opensearch.sql.common.antlr.SyntaxCheckException; +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.SemanticCheckException; @@ -168,14 +167,28 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } private void handleException(RestChannel restChannel, Exception exception) { - logAndPublishMetrics(exception); - if (exception instanceof OpenSearchException) { - OpenSearchException openSearchException = (OpenSearchException) exception; - reportError(restChannel, openSearchException, openSearchException.status()); - } else { - reportError( - restChannel, exception, isClientError(exception) ? BAD_REQUEST : INTERNAL_SERVER_ERROR); + RestStatus status = getRestStatus(exception); + logAndPublishMetrics(status, exception); + reportError(restChannel, exception, status); + } + + private static RestStatus getRestStatus(Exception ex) { + int code = getRawErrorCode(ex); + return RestStatus.fromCode(code); + } + + 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 OpenSearchException) { + return ((OpenSearchException) ex).status().getStatus(); + } + if (isClientError(ex)) { + return 400; + } + return 500; } /** @@ -208,13 +221,15 @@ private void handleCursorRequest( cursorRestExecutor.execute(client, request.params(), channel); } - private static void logAndPublishMetrics(final Exception e) { - if (isClientError(e)) { + private static void logAndPublishMetrics(final RestStatus status, final Exception e) { + if (400 <= status.getStatus() && status.getStatus() < 500) { LOG.error(QueryContext.getRequestId() + " Client side error during query execution", e); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); - } else { + } else if (500 <= status.getStatus() && status.getStatus() < 600) { LOG.error(QueryContext.getRequestId() + " Server side error during query execution", e); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); + } else { + LOG.warn("Got an exception returning non-error status {}", status, e); } } 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 92855cc1f3c..ee85772e30f 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 @@ -77,6 +77,8 @@ private static RestStatus loggedErrorCode(Exception ex) { Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS).increment(); } else if (500 <= code && code < 600) { Metrics.getInstance().getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS).increment(); + } else { + LOG.warn("Got an exception returning non-error status {}", RestStatus.fromCode(code), ex); } return RestStatus.fromCode(code); } From 6f8d67ade1afdef8e4723a1bc588add196406a7e Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Fri, 27 Mar 2026 12:46:48 -0700 Subject: [PATCH 13/13] Misc. refactoring after code self-review Signed-off-by: Simeon Widdis --- .../sql/common/error/ErrorReport.java | 8 ++- .../common/error/QueryProcessingStage.java | 49 +++---------------- .../sql/common/error/StageErrorHandler.java | 3 -- .../sql/common/error/ErrorReportTest.java | 16 +++--- docs/user/ppl/cmd/mvcombine.md | 2 +- docs/user/ppl/cmd/mvexpand.md | 2 +- docs/user/ppl/cmd/rex.md | 2 +- docs/user/ppl/interfaces/protocol.md | 2 +- .../remote/CalciteErrorReportStageIT.java | 13 ++--- .../standalone/CalcitePPLIntegTestCase.java | 3 +- .../executor/cursor/CursorResultExecutor.java | 2 +- .../response/error/ErrorMessage.java | 8 --- .../sql/plugin/rest/RestPPLQueryAction.java | 6 +++ 13 files changed, 40 insertions(+), 76 deletions(-) diff --git a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java index 25f52956f0e..1430af5ed16 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -189,7 +189,8 @@ public static class Builder { private Builder(Exception cause) { this.cause = cause; // Default details to the original exception message - this.details = cause.getLocalizedMessage(); + this.details = + cause.getLocalizedMessage() != null ? cause.getLocalizedMessage() : cause.getMessage(); } /** Set the machine-readable error code. */ @@ -200,7 +201,10 @@ public Builder code(ErrorCode code) { /** Set the query processing stage where the error occurred. */ public Builder stage(QueryProcessingStage stage) { - this.stage = stage; + // Don't overwrite more-specific stages with less-specific ones + if (this.stage == null) { + this.stage = stage; + } return this; } diff --git a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java index 4184966b851..98da1db5880 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java +++ b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java @@ -5,37 +5,20 @@ package org.opensearch.sql.common.error; +import lombok.Getter; + /** * Enumeration of query processing stages for error location tracking. These stages represent the - * major phases of query execution in the Calcite query planner. - * - *

Stage progression for a typical query: PARSING → AST_BUILDING → ANALYZING → OPTIMIZING → - * PLAN_CONVERSION → EXECUTING → RESULT_PROCESSING + * major phases of query execution in the Calcite query planner. May not be a complete list, add + * stages if needed. */ +@Getter public enum QueryProcessingStage { - /** - * PARSING stage: PPL/SQL syntax parsing via ANTLR lexer/parser. Errors: Syntax errors, unexpected - * tokens, malformed queries. - */ - PARSING("Validating query syntax"), - - /** - * AST_BUILDING stage: Abstract Syntax Tree construction from parse tree. Errors: Invalid AST - * structure, node creation failures. - */ - AST_BUILDING("Assembling the query steps"), - /** * ANALYZING stage: Semantic validation and type checking. Errors: Field not found, type * mismatches, semantic violations. */ - ANALYZING("Validating the query"), - - /** - * OPTIMIZING stage: Logical plan optimization with transformation rules. Errors: Optimization - * failures, rule application errors. - */ - OPTIMIZING("Optimizing the query"), + ANALYZING("Parsing and validating the query"), /** * PLAN_CONVERSION stage: Conversion to Calcite execution plan with system limits. Errors: @@ -47,31 +30,15 @@ public enum QueryProcessingStage { * EXECUTING stage: Query execution via OpenSearch engine. Errors: Execution failures, index * access errors, resource limits. */ - EXECUTING("Running the query"), - - /** - * RESULT_PROCESSING stage: Result formatting and cursor management. Errors: Result formatting - * failures, cursor errors. - */ - RESULT_PROCESSING("Processing the query results"), - - /** - * CLUSTER_VALIDATION stage: Prevalidation that cluster is healthy before execution. Errors: - * Cluster unhealthy, nodes unavailable. - */ - CLUSTER_VALIDATION("Checking cluster health"); + EXECUTING("Running the query"); + /** -- GETTER -- Get human-readable display name for this stage. */ private final String displayName; QueryProcessingStage(String displayName) { this.displayName = displayName; } - /** Get human-readable display name for this stage. */ - public String getDisplayName() { - return displayName; - } - /** Get lowercase name suitable for JSON serialization. */ public String toJsonKey() { return name().toLowerCase(); diff --git a/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java index 2a9b720481a..2827293a9e2 100644 --- a/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java +++ b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java @@ -37,9 +37,6 @@ public static T executeStage( QueryProcessingStage stage, Supplier operation, String location) { try { return operation.get(); - } catch (ErrorReport e) { - // If already an ErrorReport, just add stage if not set - throw ErrorReport.wrap(e).stage(stage).location(location).build(); } catch (Exception e) { throw ErrorReport.wrap(e).stage(stage).location(location).build(); } diff --git a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java index 85e3fd67dc3..e3460d7a703 100644 --- a/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -8,6 +8,8 @@ import static org.junit.jupiter.api.Assertions.*; import java.util.Map; +import org.hamcrest.CoreMatchers; +import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.Test; /** Unit tests for ErrorReport. */ @@ -64,7 +66,7 @@ public void testErrorReportJsonMapWithStageInContext() { @SuppressWarnings("unchecked") Map context = (Map) json.get("context"); assertEquals("analyzing", context.get("stage")); - assertEquals("Validating the query", context.get("stage_description")); + assertEquals("Parsing and validating the query", context.get("stage_description")); assertEquals("test", context.get("field_name")); } @@ -140,11 +142,11 @@ public void testToDetailedMessage() { String message = report.toDetailedMessage(); - assertTrue(message.contains("FIELD_NOT_FOUND")); - assertTrue(message.contains("Validating the query")); - assertTrue(message.contains("Field not found")); - assertTrue(message.contains("while resolving fields")); - assertTrue(message.contains("field_name")); - assertTrue(message.contains("Check field name")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("FIELD_NOT_FOUND")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("validating the query")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("Field not found")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("while resolving fields")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("field_name")); + MatcherAssert.assertThat(message, CoreMatchers.containsString("Check field name")); } } diff --git a/docs/user/ppl/cmd/mvcombine.md b/docs/user/ppl/cmd/mvcombine.md index d8fa63ada67..1dee435a203 100644 --- a/docs/user/ppl/cmd/mvcombine.md +++ b/docs/user/ppl/cmd/mvcombine.md @@ -124,6 +124,6 @@ source=mvcombine_data Expected output: ```text -{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query'}, 'details': 'Field [does_not_exist] not found.', 'location': ['while preparing and validating the query plan'], 'code': 'FIELD_NOT_FOUND', 'type': 'IllegalArgumentException'} +{'context': {'stage': 'analyzing', 'stage_description': 'Parsing and validating the query'}, 'details': 'Field [does_not_exist] not found.', 'location': ['while preparing and validating the query plan'], 'code': 'FIELD_NOT_FOUND', 'type': 'IllegalArgumentException'} Error: Query returned no data ``` \ No newline at end of file diff --git a/docs/user/ppl/cmd/mvexpand.md b/docs/user/ppl/cmd/mvexpand.md index a2c418691ba..939e6804163 100644 --- a/docs/user/ppl/cmd/mvexpand.md +++ b/docs/user/ppl/cmd/mvexpand.md @@ -132,6 +132,6 @@ source=people Expected output: ```text -{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query', 'command': 'mvexpand'}, 'details': "Field 'tags' not found in the schema", 'location': ['while preparing and validating the query plan', 'while evaluating the input field for mvexpand'], 'code': 'FIELD_NOT_FOUND', 'type': 'SemanticCheckException'} +{'context': {'stage': 'analyzing', 'stage_description': 'Parsing and validating the query', 'command': 'mvexpand'}, 'details': "Field 'tags' not found in the schema", 'location': ['while preparing and validating the query plan', 'while evaluating the input field for mvexpand'], 'code': 'FIELD_NOT_FOUND', 'type': 'SemanticCheckException'} Error: Query returned no data ``` \ No newline at end of file diff --git a/docs/user/ppl/cmd/rex.md b/docs/user/ppl/cmd/rex.md index b8a16128b00..7623ca1257c 100644 --- a/docs/user/ppl/cmd/rex.md +++ b/docs/user/ppl/cmd/rex.md @@ -228,7 +228,7 @@ source=accounts The query returns the following results: ```text -{'context': {'stage': 'analyzing', 'stage_description': 'Validating the query', 'command': 'rex'}, 'details': "Invalid capture group name 'user_name'.", 'location': ['while preparing and validating the query plan', 'while processing the rex command', 'while validating the capture groups for the pattern'], 'code': 'SYNTAX_ERROR', 'type': 'IllegalArgumentException', 'suggestion': 'Java Regex capture groups must be alphanumeric and start with a letter. Update the capture group to be alphanumeric.'} +{'context': {'stage': 'analyzing', 'stage_description': 'Parsing and validating the query', 'command': 'rex'}, 'details': "Invalid capture group name 'user_name'.", 'location': ['while preparing and validating the query plan', 'while processing the rex command', 'while validating the capture groups for the pattern'], 'code': 'SYNTAX_ERROR', 'type': 'IllegalArgumentException', 'suggestion': 'Java Regex capture groups must be alphanumeric and start with a letter. Update the capture group to be alphanumeric.'} Error: Query returned no data ``` diff --git a/docs/user/ppl/interfaces/protocol.md b/docs/user/ppl/interfaces/protocol.md index f7c57171031..3503c094ef3 100644 --- a/docs/user/ppl/interfaces/protocol.md +++ b/docs/user/ppl/interfaces/protocol.md @@ -123,7 +123,7 @@ Expected output: "context": { "stage": "analyzing", "index_name": "unknown", - "stage_description": "Validating the query" + "stage_description": "Parsing and validating the query" }, "details": "no such index [unknown]", "location": [ diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java index 2b907a03561..f51ffabdc35 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java @@ -108,7 +108,7 @@ public void testMultipleFieldErrorsIncludeStage() throws IOException { } @Test - public void testErrorReportTypeIsCorrect() throws IOException { + public void testErrorReportTypeMatchesExceptionType() throws IOException { ResponseException exception = assertThrows( ResponseException.class, @@ -126,7 +126,7 @@ public void testErrorReportTypeIsCorrect() throws IOException { } @Test - public void testErrorCodePresent() throws IOException { + public void testFieldNotFoundIncludesErrorCode() throws IOException { ResponseException exception = assertThrows( ResponseException.class, @@ -136,12 +136,9 @@ public void testErrorCodePresent() throws IOException { JSONObject response = new JSONObject(responseBody); JSONObject error = response.getJSONObject("error"); - // Verify error may have code field (optional, but if present should be valid) - if (error.has("code")) { - String code = error.getString("code"); - assertFalse("Error code should not be empty", code.isEmpty()); - assertFalse("Error code should not be UNKNOWN", code.equals("UNKNOWN")); - } + String code = error.getString("code"); + assertFalse("Error code should not be empty", code.isEmpty()); + assertFalse("Error code should not be UNKNOWN", code.equals("UNKNOWN")); } @Test diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java index b9f8c264a87..22e12e71556 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLIntegTestCase.java @@ -200,8 +200,7 @@ public void onFailure(Exception e) { case IllegalArgumentException illegalArgumentException -> // most exceptions thrown by Calcite when resolve a plan. throw illegalArgumentException; - case null, default -> - throw new IllegalStateException("Exception happened during execution", e); + default -> throw new IllegalStateException("Exception happened during execution", e); } } }, diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java index ae8267c8c38..8adffea526e 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java @@ -68,7 +68,7 @@ public void execute(Client client, Map params, RestChannel chann "Malformed cursor: unable to extract cursor information"), RestStatus.BAD_REQUEST.getStatus()) .toString())); - } catch (OpenSearchException e) { // RESUME: need to leverage this logic with error reports + } catch (OpenSearchException e) { int status = (e.status().getStatus()); if (status > 399 && status < 500) { Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java index 289e09645c8..ad979578b76 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessage.java @@ -34,14 +34,6 @@ public ErrorMessage(Throwable exception, int status) { } private String fetchType() { - // For ErrorReport, use the ErrorCode as the type (if present and not UNKNOWN) - if (exception instanceof ErrorReport) { - ErrorReport report = (ErrorReport) exception; - if (report.getCode() != null - && report.getCode() != org.opensearch.sql.common.error.ErrorCode.UNKNOWN) { - return report.getCode().name(); - } - } return exception.getClass().getSimpleName(); } 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 ee85772e30f..5c6266beee1 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 @@ -47,6 +47,9 @@ public RestPPLQueryAction() { } private static boolean isClientError(Exception ex) { + // (Tombstone) NullPointerException has historically been treated as a client error, but + // nowadays they're rare and should be treated as system errors, since it represents a broken + // data model in our logic. return ex instanceof IllegalArgumentException || ex instanceof IndexNotFoundException || ex instanceof QueryEngineException @@ -62,6 +65,9 @@ private static int getRawErrorCode(Exception ex) { 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; }