Update invalid date error to return 400

[ritvibhatt]

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1cb8b71)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix exception handling to return 400 for invalid date errors Relevant files: core/src/main/java/org/opensearch/sql/exception/NonFallbackCalciteException.java opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java Sub-PR theme: Add integration tests for invalid date 400 response Relevant files: integ-test/src/test/java/org/opensearch/sql/ppl/QueryAnalysisIT.java

⚡ Recommended focus areas for review Inheritance Change NonFallbackCalciteException was changed from extending QueryEngineException to extending RuntimeException. This may affect exception handling logic elsewhere in the codebase that catches QueryEngineException specifically, potentially causing different error handling behavior or breaking existing catch blocks that relied on the previous hierarchy. public class NonFallbackCalciteException extends RuntimeException { public NonFallbackCalciteException(String message) { super(message); } Unsafe Cast After checking for Error, the code unconditionally casts Throwable to Exception. However, Throwable subtypes include both Error and Exception, but there could be edge cases. More importantly, if a non-Exception, non-Error Throwable were thrown (which is theoretically possible in Java), this cast would throw a ClassCastException. Consider using instanceof Exception check before casting, or catching Exception and Error separately. } catch (Throwable t) { if (t instanceof Error) { throw (Error) t; } Exception e = (Exception) t; listener.onFailure(e); Missing Error Message Check The invalidDateShouldReturn400 test only validates the HTTP status code (400) but does not verify the error message content, unlike invalidDateInEvalShouldReturn400. Consider adding message validation (e.g., checking for ExpressionEvaluationException or similar) to make the test more robust and meaningful. public void invalidDateShouldReturn400() { String query = String.format("search source=%s | where age > date('2023-99-99')", TEST_INDEX_ACCOUNT); try { executeQuery(query); fail("Expected to return 400, but none was thrown for query: " + query); } catch (ResponseException e) { assertEquals( "HTTP status should be 400", 400, e.getResponse().getStatusLine().getStatusCode()); } catch (IOException e) { throw new IllegalStateException("Unexpected exception raised for query: " + query); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1cb8b71

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid unsafe cast from Throwable to Exception The cast (Exception) t is unsafe because Throwable can also be a non-Error, non-Exception type (though rare), but more critically, after filtering out Error, the remaining Throwable could still be a plain Throwable that is not an Exception, causing a ClassCastException. Use a safer approach by wrapping non-Exception throwables or checking the type before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [225-231] } catch (Throwable t) { if (t instanceof Error) { throw (Error) t; } - Exception e = (Exception) t; + Exception e = (t instanceof Exception) ? (Exception) t : new RuntimeException(t); listener.onFailure(e); } Suggestion importance[1-10]: 6 __ Why: While in Java the only subclasses of Throwable are Error and Exception, so the cast is technically safe after filtering Error, the suggested defensive pattern using instanceof check before casting is a good practice that avoids potential ClassCastException in edge cases and improves code robustness.	Low
General	Verify exception hierarchy change doesn't break handlers Changing NonFallbackCalciteException from extending QueryEngineException to RuntimeException may break existing catch blocks or exception-handling logic elsewhere in the codebase that relies on catching QueryEngineException to handle this exception type. Verify that all callers and exception handlers that previously caught QueryEngineException still correctly handle NonFallbackCalciteException after this change. core/src/main/java/org/opensearch/sql/exception/NonFallbackCalciteException.java [9] +public class NonFallbackCalciteException extends RuntimeException { - Suggestion importance[1-10]: 4 __ Why: The existing_code and improved_code are identical, meaning no actual code change is proposed. This is a verification suggestion asking the developer to check that changing from QueryEngineException to RuntimeException doesn't break existing catch blocks, which is a valid concern but doesn't warrant a high score.	Low

---

Previous suggestions

Suggestions up to commit 8567827

Category	Suggestion	Impact
Possible issue	Preserve original status code for OpenSearch exceptions The original code used exception.status().getStatus() to get the HTTP status from the OpenSearchException itself, which is semantically correct since OpenSearch exceptions carry their own status codes. Replacing this with the passed-in status parameter may cause incorrect HTTP status codes to be returned for OpenSearch-specific exceptions (e.g., a 404 Not Found being reported as 400 or 500). Consider preserving the original behavior for OpenSearchException unless there's a specific reason to override it. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java [31] -return new OpenSearchErrorMessage((OpenSearchException) cause, status); +OpenSearchException exception = (OpenSearchException) cause; +return new OpenSearchErrorMessage(exception, exception.status().getStatus()); Suggestion importance[1-10]: 7 __ Why: The PR changes the behavior for OpenSearchException by using the passed-in status parameter instead of the exception's own status code (exception.status().getStatus()). This could cause incorrect HTTP status codes for OpenSearch-specific exceptions (e.g., 404 being reported as 400). The suggestion correctly identifies this semantic change and proposes restoring the original behavior.	Medium
	Avoid unsafe cast from Throwable to Exception The cast (Exception) t is unsafe — if t is a Throwable that is neither an Error nor an Exception (which is theoretically possible), this will throw a ClassCastException. Additionally, if t is a checked exception wrapped in a RuntimeException, it may be better to unwrap it. Use t instanceof Exception check before casting to be safe. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [225-231] } catch (Throwable t) { if (t instanceof Error) { throw (Error) t; } - Exception e = (Exception) t; - listener.onFailure(e); + if (t instanceof Exception) { + listener.onFailure((Exception) t); + } else { + listener.onFailure(new RuntimeException(t)); + } } Suggestion importance[1-10]: 5 __ Why: In practice, all Throwable subclasses are either Error or Exception, so the unsafe cast is extremely unlikely to fail. However, the suggestion is technically correct and adds a defensive check. The improvement is minor since the theoretical case of a Throwable that is neither Error nor Exception essentially never occurs in Java.	Low

Suggestions up to commit 23216ec

Category	Suggestion	Impact
Possible issue	Guard against null cause in recursive call If report.getCause() returns null, calling getRawErrorCode(null) will cause a NullPointerException since the method likely does instanceof checks on the exception. You should guard against a null cause before recursing. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [63-92] if (ex instanceof ErrorReport) { ErrorReport report = (ErrorReport) ex; // Map ErrorCode to appropriate HTTP status if (report.getCode() != null) { switch (report.getCode()) { - ... + case PERMISSION_DENIED: + return 403; + case INDEX_NOT_FOUND: + return 404; + case FIELD_NOT_FOUND: + case SYNTAX_ERROR: + case SEMANTIC_ERROR: + case TYPE_ERROR: + case AMBIGUOUS_FIELD: + return 400; + case RESOURCE_LIMIT_EXCEEDED: + return 429; + case UNSUPPORTED_OPERATION: + return 501; + case EVALUATION_ERROR: + case PLANNING_ERROR: + case EXECUTION_ERROR: + case UNKNOWN: default: break; // Fall through to check underlying cause } } // If no specific mapping, check the underlying cause - return getRawErrorCode(((ErrorReport) ex).getCause()); + Throwable cause = report.getCause(); + if (cause instanceof Exception) { + return getRawErrorCode((Exception) cause); + } + return 500; } Suggestion importance[1-10]: 7 __ Why: If report.getCause() returns null, the recursive call to getRawErrorCode(null) would cause a NullPointerException since the method performs instanceof checks without null guards. This is a valid defensive fix.	Medium
	Guard against unsafe Throwable cast The cast (Exception) t will throw a ClassCastException if t is a Throwable that is neither an Error nor an Exception (e.g., a direct Throwable subclass). You should handle this case by wrapping the Throwable in a RuntimeException when it cannot be cast to Exception. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [225-231] } catch (Throwable t) { if (t instanceof Error) { throw (Error) t; } - Exception e = (Exception) t; + Exception e = (t instanceof Exception) ? (Exception) t : new RuntimeException(t); listener.onFailure(e); } Suggestion importance[1-10]: 6 __ Why: The cast (Exception) t could fail if t is a direct Throwable subclass that is neither Error nor Exception. Wrapping it in a RuntimeException is a safer approach, though direct Throwable subclasses are extremely rare in practice.	Low
	Changing base class breaks client error detection Changing NonFallbackCalciteException from QueryEngineException to RuntimeException removes it from the isClientError check in RestPPLQueryAction, which checks ex instanceof QueryEngineException. This means exceptions of this type will no longer automatically return a 400 status, potentially causing previously-client-error responses to return 500. Verify that all callers of this exception are handled correctly with the new error routing logic. core/src/main/java/org/opensearch/sql/exception/NonFallbackCalciteException.java [9] -public class NonFallbackCalciteException extends RuntimeException { +public class NonFallbackCalciteException extends QueryEngineException { Suggestion importance[1-10]: 5 __ Why: The change from QueryEngineException to RuntimeException removes NonFallbackCalciteException from the isClientError check, but the suggestion to revert may conflict with the PR's intent. The PR likely intentionally changed this, and the new error routing logic in getRawErrorCode may handle this case differently now.	Low

Suggestions up to commit c5034b8

Category	Suggestion	Impact
Possible issue	Preserve OpenSearchException's own HTTP status code The original code used exception.status().getStatus() to get the HTTP status from the OpenSearchException itself, which is semantically correct for OpenSearch-specific errors. By replacing it with the passed-in status, you may return an incorrect HTTP status code (e.g., 500 instead of 404) when the OpenSearchException carries its own meaningful status. Consider preserving the original behavior by using the exception's own status when available. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java [31] -return new OpenSearchErrorMessage((OpenSearchException) cause, status); +OpenSearchException exception = (OpenSearchException) cause; +int resolvedStatus = exception.status() != null ? exception.status().getStatus() : status; +return new OpenSearchErrorMessage(exception, resolvedStatus); Suggestion importance[1-10]: 7 __ Why: The original code used exception.status().getStatus() to derive the HTTP status from the OpenSearchException itself, which is semantically correct. The PR replaces this with the passed-in status parameter, which may not reflect the exception's own status code. However, the status handling was also moved to OpenSearchErrorMessage constructor in this PR, so the full impact needs to be assessed across both files.	Medium
	Changing base class may break client error classification Changing NonFallbackCalciteException from extending QueryEngineException to RuntimeException removes it from the isClientError check in RestPPLQueryAction, which checks for QueryEngineException. This means exceptions of this type will no longer be treated as client errors (400) and may instead return 500. Verify that this is the intended behavior, or add NonFallbackCalciteException explicitly to the isClientError method if it should still return 400. core/src/main/java/org/opensearch/sql/exception/NonFallbackCalciteException.java [9] -public class NonFallbackCalciteException extends RuntimeException { +public class NonFallbackCalciteException extends QueryEngineException { Suggestion importance[1-10]: 6 __ Why: Changing NonFallbackCalciteException from QueryEngineException to RuntimeException removes it from the isClientError check in RestPPLQueryAction, potentially causing these exceptions to return 500 instead of 400. This is a valid concern about behavioral change, though it may be intentional if the PR wants non-fallback exceptions to be treated differently.	Low
General	Properly propagate error codes from wrapped exceptions The SQLException check only returns 400 if the cause is a client error, but falls through to the OpenSearchException check and then returns 500 for all other SQLException cases (including those wrapping server errors). Consider also recursively calling getRawErrorCode on the cause for non-client-error SQLException cases to properly propagate the error code. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [94-99] if (ex instanceof java.sql.SQLException) { Throwable cause = ex.getCause(); - if (cause instanceof Exception && isClientError((Exception) cause)) { - return 400; + if (cause instanceof Exception) { + return getRawErrorCode((Exception) cause); } } Suggestion importance[1-10]: 5 __ Why: The current SQLException handling only returns 400 for client errors but falls through to return 500 for all other cases. Recursively calling getRawErrorCode on the cause would properly propagate error codes for non-client-error SQLException cases, improving correctness.	Low

Suggestions up to commit 7d58ef1

Category	Suggestion	Impact
Possible issue	Preserve OpenSearch exception's own status code The original code used exception.status().getStatus() for OpenSearchException, which preserves the semantic HTTP status from OpenSearch (e.g., 404 for index not found). By passing the outer status parameter instead, you lose this information when the cause is an OpenSearchException. Consider using the exception's own status code as the default, falling back to the passed status only if the exception's status is null. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java [30-32] if (cause instanceof OpenSearchException) { - return new OpenSearchErrorMessage((OpenSearchException) cause, status); + OpenSearchException exception = (OpenSearchException) cause; + int resolvedStatus = (exception.status() != null) ? exception.status().getStatus() : status; + return new OpenSearchErrorMessage(exception, resolvedStatus); } Suggestion importance[1-10]: 7 __ Why: The PR changed the behavior by passing the outer status parameter instead of using the OpenSearchException's own status code. The original code used exception.status().getStatus() which preserves semantic HTTP status from OpenSearch. This change could cause incorrect HTTP status codes to be returned for OpenSearch exceptions. The suggested fix correctly restores this behavior with a null-safe fallback.	Medium
	Guard against null cause in recursive call When getRawErrorCode is called recursively with ((ErrorReport) ex).getCause(), the cause may be null if no underlying cause was set, which would cause a NullPointerException. Add a null check before the recursive call. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [101] -return getRawErrorCode(((ErrorReport) ex).getCause()); +Throwable cause = ((ErrorReport) ex).getCause(); +if (cause == null) { + return 500; +} +if (cause instanceof Exception) { + return getRawErrorCode((Exception) cause); +} +return 500; Suggestion importance[1-10]: 6 __ Why: If getCause() returns null, the recursive call to getRawErrorCode would throw a NullPointerException since the method signature takes a non-null Exception. This is a valid defensive programming concern, though the improved_code changes the method signature assumption by handling Throwable vs Exception.	Low
General	Handle multiple shard failure statuses correctly The SearchPhaseExecutionException check is placed after the OpenSearchException check, but SearchPhaseExecutionException extends RuntimeException, not OpenSearchException. However, the isClientError traversal now walks the cause chain, so a SearchPhaseExecutionException wrapping a client error could return 400 via isClientError. The dedicated handler is fine, but it only inspects shardFailures()[0] — if there are multiple shard failures with different statuses, only the first is considered. Consider iterating all shard failures to find the most appropriate status code. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [106-108] +if (ex instanceof SearchPhaseExecutionException) { + return getSearchPhaseExecutionExceptionStatus((SearchPhaseExecutionException) ex); +} - Suggestion importance[1-10]: 3 __ Why: The suggestion points out that only the first shard failure is inspected in getSearchPhaseExecutionExceptionStatus, but the improved_code is identical to the existing_code, making this more of a review comment than an actionable fix. The concern about multiple shard failures is valid but minor.	Low

Suggestions up to commit f764bed

Category	Suggestion	Impact
General	Simplify redundant status determination logic The logic return status != originalStatus ? status : originalStatus always returns status when they differ and originalStatus when they're the same, but since originalStatus == status in the second branch, this simplifies to always returning status. The method is therefore equivalent to just returning status, making the originalStatus computation unnecessary. Simplify the method to directly return status. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java [20-25] private static int determineStatus(OpenSearchException exception, int status) { - int originalStatus = exception.status() != null ? exception.status().getStatus() : 500; - // If the passed status differs from original, use the passed status (it was corrected) - // If they're the same, use the original (no correction needed) - return status != originalStatus ? status : originalStatus; + return status; } Suggestion importance[1-10]: 6 __ Why: The logic status != originalStatus ? status : originalStatus always evaluates to status regardless of the branch taken, making the originalStatus computation and the conditional unnecessary. Simplifying to return status is correct and cleaner.	Low
	Limit cause chain traversal depth to avoid false positives Traversing the full cause chain can lead to false positives: a server-side error that wraps a client-side exception deep in its cause chain would incorrectly be classified as a client error and return a 400. Consider limiting the traversal to only the direct cause (one level deep) to avoid misclassifying server errors. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [53-65] Throwable current = ex; -while (current != null) { +int depth = 0; +while (current != null && depth <= 1) { if (current instanceof IllegalArgumentException || current instanceof IndexNotFoundException || current instanceof QueryEngineException || current instanceof SyntaxCheckException || current instanceof DataSourceClientException || current instanceof IllegalAccessException) { return true; } current = current.getCause(); + depth++; } return false; Suggestion importance[1-10]: 4 __ Why: The concern about false positives from deep cause chain traversal is valid in theory, but the PR's intent is specifically to unwrap wrapped exceptions (e.g., RuntimeException wrapping a QueryEngineException) to correctly classify them as client errors. Limiting to depth 1 may be too restrictive and could miss legitimate cases.	Low
Possible issue	Preserve OpenSearch exception's original status code Previously, OpenSearchErrorMessage used the exception's own status code, which is semantically correct for OpenSearch exceptions (e.g., a 404 from OpenSearch should remain 404). By always passing the outer status (which may be a corrected/overridden value like 400), you may incorrectly override meaningful OpenSearch-specific HTTP status codes. Consider preserving the original exception status for OpenSearchException unless there's a specific reason to override it. opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java [30-32] if (cause instanceof OpenSearchException) { - return new OpenSearchErrorMessage((OpenSearchException) cause, status); + OpenSearchException exception = (OpenSearchException) cause; + int exceptionStatus = exception.status() != null ? exception.status().getStatus() : status; + return new OpenSearchErrorMessage(exception, exceptionStatus); } Suggestion importance[1-10]: 5 __ Why: The PR intentionally passes the outer status to OpenSearchErrorMessage, and the determineStatus method in OpenSearchErrorMessage already handles the logic of choosing between the passed status and the exception's own status. This suggestion partially duplicates that logic and may conflict with the PR's design intent.	Low

[github-actions]

Persistent review updated to latest commit f764bed

[github-actions]

Persistent review updated to latest commit 7d58ef1

[github-actions]

Persistent review updated to latest commit c5034b8

[github-actions]

Persistent review updated to latest commit 23216ec

[github-actions]

Persistent review updated to latest commit 8567827

[github-actions]

Persistent review updated to latest commit 1cb8b71