Improve exception handling in UnifiedQueryPlanner

[dai-chen]

Description

Tighten exception handling, improve error classification, and add structured logging in UnifiedQueryPlanner.plan().

Related Issues

Part of #5246

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 Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 67ffe78.

Path	Line	Severity	Description
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java	80	low	LOG.error logs e.getMessage() which may include user-supplied query fragments in plaintext log output. Not malicious, but could inadvertently expose sensitive query content in logs.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9417b02
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Include exception in log statement Include the exception object in the log statement to capture the full stack trace. This provides better debugging information when investigating failures in production environments. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [74-79] } catch (SyntaxCheckException | QueryEngineException | UnsupportedOperationException | IllegalArgumentException e) { - LOG.error("Failed to plan query: {}", e.getMessage()); + LOG.error("Failed to plan query: {}", e.getMessage(), e); throw e; Suggestion importance[1-10]: 7 __ Why: Adding the exception object to the log statement provides full stack trace information, which is valuable for debugging. However, this is a logging improvement rather than a functional fix.	Medium
	Avoid catching AssertionError broadly Catching AssertionError is risky as it can mask programming errors and JVM-level issues. Consider catching a more specific Calcite exception type or documenting why this broad catch is necessary for this specific Calcite behavior. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [80-84] } catch (AssertionError e) { // Calcite throws assertion error directly when building bad RelNode + // TODO: Replace with specific Calcite exception when available String message = "Failed to plan query: invalid plan structure"; LOG.error(message, e); throw new SemanticCheckException(message, e); Suggestion importance[1-10]: 5 __ Why: While catching AssertionError is generally discouraged, the code already includes a comment explaining this is a known Calcite behavior. The suggestion to add a TODO is reasonable but offers marginal improvement since the issue is already documented.	Low

---

Previous suggestions

Suggestions up to commit 67ffe78

Category	Suggestion	Impact
General	Include query in error logs The error message logged does not include the actual query that failed, making debugging difficult. Include the query string in the log message to provide better context for troubleshooting. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [74-79] } catch (SyntaxCheckException | QueryEngineException | UnsupportedOperationException | IllegalArgumentException e) { - LOG.error("Failed to plan query: {}", e.getMessage()); + LOG.error("Failed to plan query '{}': {}", query, e.getMessage()); throw e; Suggestion importance[1-10]: 7 __ Why: Including the query string in error logs significantly improves debugging capabilities by providing essential context. This is a valuable enhancement for troubleshooting production issues.	Medium
	Document AssertionError catch rationale Catching AssertionError is generally discouraged as it indicates programming errors or violated invariants. Consider documenting why this specific case is safe and necessary, or work with the Calcite team to replace assertions with proper exceptions. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [80-84] } catch (AssertionError e) { - // Calcite throws assertion error directly when building bad RelNode + // WORKAROUND: Calcite throws AssertionError for certain invalid plan structures + // instead of proper exceptions. This should be addressed upstream in Calcite. + // See: [link to relevant Calcite issue if available] String message = "Failed to plan query: invalid plan structure"; LOG.error(message, e); throw new SemanticCheckException(message, e); Suggestion importance[1-10]: 5 __ Why: While the suggestion to enhance documentation is valid, the existing comment already explains why AssertionError is caught. The improved version adds marginal value by suggesting upstream tracking, but doesn't fundamentally change the code behavior.	Low

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The catch block for QueryEngineException at line 75 only logs and rethrows, but QueryEngineException is not thrown by the code in the try block. If this exception type is expected from strategy.plan() or plan.accept(), it should be documented. Otherwise, this catch clause is unreachable and misleading. } catch (SyntaxCheckException | QueryEngineException | UnsupportedOperationException | IllegalArgumentException e) { LOG.error("Failed to plan query: {}", e.getMessage()); throw e; Possible Issue Catching IllegalArgumentException at line 77 may inadvertently suppress legitimate programming errors (e.g., null checks, invalid method arguments) that should propagate as bugs rather than user-facing query errors. This is too broad unless there is a specific known scenario where IllegalArgumentException indicates a query planning issue. } catch (SyntaxCheckException | QueryEngineException | UnsupportedOperationException | IllegalArgumentException e) { LOG.error("Failed to plan query: {}", e.getMessage()); throw e;