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/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..3a4ac7b287d --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorCode.java @@ -0,0 +1,54 @@ +/* + * 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.
+ *
+ * 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 */ + 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 -- don't set this manually, it's filled in as the default if no + * other code applies. + */ + 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..1430af5ed16 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java @@ -0,0 +1,282 @@ +/* + * 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; +import lombok.Getter; + +/** + * 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 { + + @Getter private final Exception cause; + @Getter private final ErrorCode code; + @Getter private final QueryProcessingStage stage; + private final List locationChain; + private final Map context; + @Getter private final String suggestion; + @Getter 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(Exception cause) { + if (cause instanceof ErrorReport existing) { + 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 List getLocationChain() { + return new ArrayList<>(locationChain); + } + + public Map getContext() { + return new LinkedHashMap<>(context); + } + + /** 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++) { + // 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"); + } + } + + 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) { + json.put("code", code.name()); + } + + if (details != null) { + json.put("details", details); + } + + if (!locationChain.isEmpty()) { + // 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 + 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); + } + + if (suggestion != null) { + json.put("suggestion", suggestion); + } + + return json; + } + + /** Builder for constructing error reports with contextual information. */ + public static class Builder { + private final Exception 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(Exception cause) { + this.cause = cause; + // Default details to the original exception message + this.details = + cause.getLocalizedMessage() != null ? cause.getLocalizedMessage() : cause.getMessage(); + } + + /** 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) { + // Don't overwrite more-specific stages with less-specific ones + if (this.stage == null) { + 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..98da1db5880 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/QueryProcessingStage.java @@ -0,0 +1,46 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +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. May not be a complete list, add + * stages if needed. + */ +@Getter +public enum QueryProcessingStage { + /** + * ANALYZING stage: Semantic validation and type checking. Errors: Field not found, type + * mismatches, semantic violations. + */ + ANALYZING("Parsing and validating the query"), + + /** + * PLAN_CONVERSION stage: Conversion to Calcite execution plan with system limits. Errors: + * Unsupported operations, plan conversion failures. + */ + 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 the query"); + + /** -- GETTER -- Get human-readable display name for this stage. */ + private final String displayName; + + QueryProcessingStage(String displayName) { + this.displayName = 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..2827293a9e2 --- /dev/null +++ b/common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java @@ -0,0 +1,103 @@ +/* + * 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 (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 (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, Exception 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..e3460d7a703 --- /dev/null +++ b/common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java @@ -0,0 +1,152 @@ +/* + * 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.hamcrest.CoreMatchers; +import org.hamcrest.MatcherAssert; +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("Parsing and validating the query", context.get("stage_description")); + 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(); + + 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/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index e4e036da3a6..fb7fa3e7fab 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; @@ -3743,8 +3756,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/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/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 320325c8438..b294a168cd8 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,9 @@ 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; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.common.utils.QueryContext; @@ -142,11 +145,30 @@ public void executeWithCalcite( CalcitePlanContext context = CalcitePlanContext.create( buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); + context.setHighlightConfig(highlightConfig); - 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), + "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), + "while converting the query to an executable plan"); + 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), + "while running the query"); } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); @@ -291,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..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 -{'reason': 'Invalid Query', 'details': 'Field [does_not_exist] 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 6fdd9bca365..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 -{'reason': 'Invalid Query', 'details': "Field 'tags' not found in the schema", '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 b4fe706f489..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 -{'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': '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 680f01fd379..3503c094ef3 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": "Parsing and 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/calcite/remote/CalciteErrorReportStageIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java new file mode 100644 index 00000000000..f51ffabdc35 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java @@ -0,0 +1,217 @@ +/* + * 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.toLowerCase().contains("checking") + || stageDescription.toLowerCase().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 testErrorReportTypeMatchesExceptionType() 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 testFieldNotFoundIncludesErrorCode() 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"); + + 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")); + } +} 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/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..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 @@ -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,19 @@ 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; + default -> throw new IllegalStateException("Exception happened during execution", e); } } }, 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(); } } 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/"} 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/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/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 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..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 @@ -8,6 +8,7 @@ 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 { @@ -62,12 +63,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); - 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; 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/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..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 @@ -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,10 +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.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; @@ -49,17 +46,47 @@ public RestPPLQueryAction() { super(); } - 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; + 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 + || 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(); + } + // Possible future work: We currently do this on exception types, when we have more robust + // ErrorCodes in more locations it may be worth switching this to be based on those instead. + // That lets us identify specific error cases at a granularity higher than exception types. + if (isClientError(ex)) { + return 400; + } + return 500; + } + + 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(); + } else { + LOG.warn("Got an exception returning non-error status {}", RestStatus.fromCode(code), ex); + } + return RestStatus.fromCode(code); } @Override @@ -98,33 +125,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); } }); } 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); });