[Backport 3.9] Stop Janino compile failures from returning the query plan

[opensearch-ci-bot]

Backport 07f079d from #5784.

[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 method hideCodegenInternals catches all exceptions during chain analysis but does not rethrow them. If ExceptionUtils.getThrowableList(e) or subsequent operations throw an exception, the original user error is silently replaced with nothing, causing the query to fail without a meaningful error message. This occurs when the exception chain contains unexpected structures that cause the analysis to fail. private static ErrorReport.Builder hideCodegenInternals( QueryProcessingStage stage, ErrorReport.Builder builder, Throwable e) { // Code generation only runs under EXECUTING. Gating on the stage stops a user-supplied // identifier that happens to carry a marker from reading as an engine defect, since a field // name reaches the analysis stages inside the message. if (stage != QueryProcessingStage.EXECUTING || builder.hasReason()) { return builder; } // getThrowableList halts if the cause chain loops back on itself. Do not swap in Guava's // getCausalChain, which throws on that, and this runs inside a catch block where a new throw // would replace the user's error. Taken once, since both checks read the same chain. List<Throwable> chain = ExceptionUtils.getThrowableList(e); if (!isCodegenFailure(chain)) { return builder; } builder.reason(CODEGEN_FAILURE_REASON); Throwable compileException = findCompileException(chain); String serialized = serializedCompilerMessage(chain); if (compileException != null && compileException.getMessage() != null) { builder.details(compileException.getMessage()); } else if (serialized != null) { builder.details(serialized); } else { // Detected by a marker whose own message embeds the generated source, so nothing in the // chain is known to be free of engine internals. builder.details(CODEGEN_FAILURE_DETAILS); } return builder; } Logic Issue The method withoutPlan uses contains to detect the plan marker, which can match user-supplied strings that happen to contain "Error while preparing plan [". For example, a field named "Error while preparing plan [field]" would trigger plan stripping even though it is not actually a plan. This causes legitimate error messages to be incorrectly replaced with the generic fallback message. private static String withoutPlan(String message) { if (message == null) { return null; } return message.contains(PLAN_PREPARATION_PREFIX) ? PLAN_PREPARATION_MESSAGE : message; }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for throwable parameter The method marksCodegen should handle potential NullPointerException when t is null. Add a null check at the beginning of the method to prevent crashes if a null Throwable is passed in the exception chain. common/src/main/java/org/opensearch/sql/common/error/StageErrorHandler.java [73-79] private static boolean marksCodegen(Throwable t) { + if (t == null) { + return false; + } if (t.getClass().getSimpleName().equals(COMPILE_EXCEPTION_CLASS)) { return true; } String message = t.getMessage(); return message != null && Arrays.stream(CODEGEN_MESSAGE_MARKERS).anyMatch(message::contains); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that marksCodegen could receive null from the stream, though getThrowableList typically doesn't include nulls. Adding the null check is a defensive programming practice that prevents potential NullPointerException.	Medium
Security	Prevent plan exposure via root cause The method enrichErrorsForSpecialCases should handle the case where rootCauseMessage(e) returns the same message as e.getMessage(). This can create a situation where the plan is still exposed if the root cause fallback returns the original plan-bearing message. core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java [493-508] private static void enrichErrorsForSpecialCases(ErrorReport.Builder report, SQLException e) { String message = e.getMessage(); if (message != null && message.contains(PLAN_PREPARATION_PREFIX) && e.getCause() != null) { - ... String rootCause = rootCauseMessage(e); - if (rootCause != null && !rootCause.contains(PLAN_PREPARATION_PREFIX)) { + if (rootCause != null && !rootCause.equals(message) && !rootCause.contains(PLAN_PREPARATION_PREFIX)) { report.details(rootCause); } else { report.details( "The engine could not prepare this query plan for execution. The plan is in the" + " OpenSearch node log."); } } Suggestion importance[1-10]: 6 __ Why: Valid concern that rootCauseMessage could return the same plan-bearing message. Adding !rootCause.equals(message) prevents this edge case where the fallback logic would expose the plan through the details field.	Low
General	Improve plan detection accuracy The withoutPlan method performs a simple substring check that may incorrectly match legitimate error messages containing the prefix. Consider using startsWith or a more precise pattern match to avoid false positives where the prefix appears mid-string in unrelated contexts. common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java [108-113] private static String withoutPlan(String message) { if (message == null) { return null; } - return message.contains(PLAN_PREPARATION_PREFIX) ? PLAN_PREPARATION_MESSAGE : message; + int index = message.indexOf(PLAN_PREPARATION_PREFIX); + return index >= 0 ? PLAN_PREPARATION_MESSAGE : message; } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the intent. The code comment explicitly states the prefix is matched with contains (not startsWith) because exception wrapping can bury the marker mid-string. The suggested change using indexOf is functionally equivalent to contains and doesn't improve accuracy.	Low

[codecov]

Codecov Report

❌ Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.11%. Comparing base (b60f045) to head (defeefa).

Files with missing lines	Patch %	Lines
...ensearch/sql/calcite/utils/CalciteToolsHelper.java	0.00%	7 Missing ⚠️
...ch/sql/opensearch/response/error/ErrorMessage.java	85.71%	0 Missing and 1 partial ⚠️

❌ Your project check has failed because the head coverage (63.11%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##                3.9    #5881      +/-   ##
============================================
+ Coverage     63.10%   63.11%   +0.01%     
- Complexity     8865     8868       +3     
============================================
  Files           942      942              
  Lines         40294    40300       +6     
  Branches       4532     4534       +2     
============================================
+ Hits          25427    25437      +10     
+ Misses        14056    14050       -6     
- Partials        811      813       +2     

Flag	Coverage Δ
sql-engine	63.11% <42.85%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.