Skip to content

fix(integ): repair two pre-existing IT failures on main (QueryValidationIT error type, NoMv explain flake)#5545

Merged
mengweieric merged 1 commit into
opensearch-project:mainfrom
mengweieric:ae-ci-flaky-fixes
Jun 12, 2026
Merged

fix(integ): repair two pre-existing IT failures on main (QueryValidationIT error type, NoMv explain flake)#5545
mengweieric merged 1 commit into
opensearch-project:mainfrom
mengweieric:ae-ci-flaky-fixes

Conversation

@mengweieric

Copy link
Copy Markdown
Collaborator

Description

Repairs two pre-existing IT failures on main (both in integ-test, test-only, no production change).

1. QueryValidationIT.testAliasToKeywordMultiFieldFailsWithBadRequest#5532 changed the SQL error formatter to unwrap ErrorReport to its cause, so the reported type is now the underlying SemanticCheckException, not the ErrorReport wrapper. The query is still correctly rejected with 400; only the error-envelope type changed. Updated the expected type to match.

2. CalciteExplainIT.testNoMvBasic / testNoMvWithEval — the assertion scanned the full logical+physical explain YAML for ARRAY_JOIN, but the physical section renders differently when pushdown is disabled (CalciteNoPushdownIT), making the test flaky on some runners. nomv lowers to MVJOIN → ARRAY_JOIN, which is stable in the logical plan regardless of pushdown (verified with pushdown on and off). Assert on the logical section only.

Testing

Ran against an OpenSearch cluster:

  • QueryValidationIT — full class passes (target test + siblings).
  • CalciteExplainIT.testNoMvBasic / testNoMvWithEval — pass; logical plan confirmed to contain ARRAY_JOIN with pushdown both enabled and disabled.

Check List

  • New functionality includes testing.
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 5b621c9)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 5b621c9
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add null check for parameter

The method assumes explainYaml is never null, which could cause a
NullPointerException. Add a null check at the beginning of the method to handle this
edge case gracefully.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java [2922-2925]

 private static String logicalPlan(String explainYaml) {
+  if (explainYaml == null) {
+    return "";
+  }
   int physicalIdx = explainYaml.indexOf("\nphysical:");
   return physicalIdx >= 0 ? explainYaml.substring(0, physicalIdx) : explainYaml;
 }
Suggestion importance[1-10]: 3

__

Why: While adding a null check is generally good practice, the method logicalPlan is private and only called with the result of explainQueryYaml(query) in the same class. The likelihood of receiving null is low in this controlled context, making this a minor defensive programming improvement rather than a critical fix.

Low

Previous suggestions

Suggestions up to commit 1e52ff2
CategorySuggestion                                                                                                                                    Impact
General
Fix assertion message case mismatch

The assertion message doesn't match the actual check being performed. The code
converts to lowercase before checking, but the message refers to uppercase function
names. Update the message to use lowercase names for consistency.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java [2908-2910]

 String logical = logicalPlan(explainQueryYaml(query)).toLowerCase();
 Assert.assertTrue(
-    "Expected logical plan to contain both CONCAT and ARRAY_JOIN",
+    "Expected logical plan to contain both concat and array_join",
     logical.contains("concat") && logical.contains("array_join"));
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a minor inconsistency where the assertion message uses uppercase function names (CONCAT and ARRAY_JOIN) while the actual check uses lowercase (concat and array_join). While valid, this is a cosmetic improvement that doesn't affect functionality.

Low

@mengweieric mengweieric added PPL Piped processing language bugFix labels Jun 11, 2026
QueryValidationIT.testAliasToKeywordMultiFieldFailsWithBadRequest: opensearch-project#5532 changed
the SQL error formatter to unwrap ErrorReport to its cause, so the reported type
is now the underlying SemanticCheckException, not the ErrorReport wrapper. Update
the expected error type to match. Query is still correctly rejected (400); only
the envelope type changed.

CalciteExplainIT.testNoMvBasic / testNoMvWithEval: the assertion scanned the full
logical+physical explain YAML for ARRAY_JOIN, but the physical section renders
differently with pushdown disabled (CalciteNoPushdownIT), making the test flaky
on some runners. nomv lowers to MVJOIN -> ARRAY_JOIN, which is stable in the
LOGICAL plan regardless of pushdown (verified with pushdown on and off). Assert
on the logical section only.

Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5b621c9

@dai-chen dai-chen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@mengweieric mengweieric merged commit 7f31510 into opensearch-project:main Jun 12, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFix PPL Piped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants