Sandbox/qa - Add multi shard qa test suite

[mch2]

Description

Adds a test suite to sandbox/qa for 2 shard setups. Also unmutes single shard tests that are now passing.
see sandbox/qa/analytics-engine-rest/src/test/resources/datasets/merge_coverage/README.md.
for current list of skipped/failing tests.

How

merge_coverage is provisioned at 1 shard (oracle) and 2 shards; each %INDEX% query runs against both. Exact tier asserts the two results are equal (unordered, numeric-tolerant, multiset-normalized for
values/list); curated shapes also pin a golden. Approximate tier (distinct_count/percentile) asserts the 2-shard result within tolerance of a required golden, since sketches drift across a merge. head/limit
carry a unique id tie-breaker for determinism. Indices are composite parquet + lucene, so the planner's per-arm backend selection is exercised.

Structure: one abstract TwoShardReduceTestCase (all machinery) + thin per-category ITs that only declare their dataset tier and known-issue set — TwoShardAggregationIT, TwoShardScalarIT, TwoShardShapeIT,
TwoShardJoinIT, TwoShardCommandIT, plus IpFieldMultiShardIT.

Currently muted tests:

Here's the full list of currently-muted tests (@AwaitsFix) in analytics-engine-rest

ere's the complete failure inventory across the analytics-engine-rest module.

  1-shard suites

  Per-query failures (suite runs, these qN skip)

  AggregationsPplIT       q7, q8, q9, q10                              (4 of 10)
  AppLogsPplIT            q5, q9                                       (2 of 10)
  ComplexJoinsPplIT       q1,q2,q3,q4,q7,q8,q9,q10                     (8 of 10)
  FulltextWindowPplIT     q1,q6,q8,q12,q13,q14,q15,q17,q19            (9 of 20)
  FunctionsPplIT          q13                                          (1 of 18)
  KubernetesLogsPplIT     q9                                           (1 of 10)
  MultiIndexQueriesPplIT  q2, q7, q10                                  (3 of 14)
  MultiSourceJoinsPplIT   q2, q4                                       (2 of 5)
  RexCommandPplIT         q1,q5,q7,q8,q13,q18                         (6 of 20)
  SecurityLogsPplIT       q1,q2,q3,q4,q5,q7,q8                        (7 of 10)
  ExtensiveCoveragePplIT  96 of 197 — q8,9,10,13,19,20,22,24,25,28,29,30,37,39,40,41,42,43,44,52,54,55,56,57,58,59,60,61,62,70,77,81,85,86,88,89,93,94,95,97,98,99,100,101,102,103,104,105,106,108,110,111,112,114,115,116,117,119,120,125,126,128,129,130,131,132,136,137,138,139,143,144,145,147,148,149,150,151,152,153,154,155,156,157,158,160,162,163,177,188,189,190,191,193,195,196

  Whole suite fails (100% of queries → class still @AwaitsFix)

  ComplexRegexPplIT       (5 of 5)   — grok/regex unsupported
  LookupJoinQueriesPplIT  (6 of 6)   — lookup-join unsupported
  LookupTableQueriesPplIT (2 of 2)   — lookup-table unsupported

  Individual method failures (method-level @AwaitsFix)

  ConditionalFunctionsIT.testEarliestAbsoluteLiteralSelectsSpecificRows
  DynamicMappingSearchIT.testSearchOnDynamicallyAddedFields
  MultisearchCommandIT.testMultisearchThreeBranchesByStr0
  PatternsCommandIT.testSimplePatternAggregationModeMultiShard

  2-shard suite (knownIssues)

  distinct_count_label / distinct_count_by_cat / dc_label   — HLL cross-shard merge over-counts
  streamstats_sum                                            — cumulative window over arrival-ordered gather
  join_inner_id_count / join_inner_category_count /
  join_left_count / join_semi_count / join_anti_count /
  join_inner_by_category                                     — #21867 mixed lucene/datafusion backend reject
  cmd_appendpipe                                             — same #21867 (union)
  cmd_search / cmd_append / cmd_regex / cmd_multisearch      — indexed-executor SIGSEGV (node crash)
  cmd_appendcols                                             — not in PPL grammar
  cmd_timechart                                              — needs @timestamp field

  Totals

  - 1-shard: ~138 failing queries across 10 partial suites + 3 fully-failing suites (13 queries) + 8 standalone methods.
  - 2-shard: 17 muted queries.

Related Issues

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

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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 a577ad1)

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 Resource Leak The FileSystem fs opened at line 363 may not be closed if an exception occurs between lines 364-377 before reaching the finally block. If PathUtils.get(uri) or Files.list(dir) throws, the FileSystem remains open. Move the FileSystem creation inside the try block or ensure it is closed in all exception paths. FileSystem fs = null; try { URI uri = url.toURI(); Path dir; if ("jar".equals(uri.getScheme())) { fs = FileSystems.newFileSystem(uri, Collections.emptyMap()); dir = fs.getPath(resourceDir); } else { dir = PathUtils.get(uri); } try (Stream<Path> stream = Files.list(dir)) { stream.forEach(p -> { String fileName = p.getFileName().toString(); if (fileName.endsWith(".ppl")) { names.add(fileName.substring(0, fileName.length() - ".ppl".length())); } }); } } catch (Exception e) { throw new IOException("Failed to discover queries in [" + resourceDir + "]", e); } finally { if (fs != null) { fs.close(); } } Possible Issue Lines 120-131 have early returns inside conditions that check for null or size mismatches, but these return statements are missing. The code falls through to line 134 where it attempts to copy and sort lists that may be null or mismatched in size, causing a NullPointerException or incorrect behavior. The removed code at lines 106-112 and 123-126 in the old hunk had explicit return statements that are now absent. if (expectedRows == null && actualRows == null) { return null; // both empty } if (expectedRows == null) { return String.format(java.util.Locale.ROOT, "%s: Expected empty response but got %d rows", label, actualRows.size()); } if (actualRows == null) { return String.format(java.util.Locale.ROOT, "%s: Expected %d rows but got empty response", label, expectedRows.size()); } if (expectedRows.size() != actualRows.size()) { return String.format(java.util.Locale.ROOT, "%s: Row count mismatch - expected %d, got %d", label, expectedRows.size(), actualRows.size()); } // Copy before sorting so we never reorder the caller's lists. expectedRows = new java.util.ArrayList<>(expectedRows); actualRows = new java.util.ArrayList<>(actualRows); expectedRows.sort(new RowComparator());

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a577ad1
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix null-equality check logic The null-equality check expected == actual is always false when one is null and the other isn't (the condition already ensures at least one is null). The intended logic should check if both are null. Use Objects.equals(expected, actual) or explicitly check both are null. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TwoShardReduceTestCase.java [199-201] private static String compareWithinTolerance(List<List<Object>> expected, List<List<Object>> actual, String label) { + if (expected == null && actual == null) { + return null; + } if (expected == null || actual == null) { - return expected == actual ? null : label + ": one side empty"; + return label + ": one side empty"; } Suggestion importance[1-10]: 8 __ Why: This is a genuine logic bug. The condition expected == actual when one is null will always be false, making the null-equality check ineffective. The suggested fix correctly separates the "both null" case from the "one null" case, preventing incorrect error messages.	Medium
General	Use try-with-resources for FileSystem The FileSystem resource is manually managed with a finally block, but if an exception occurs before fs is assigned (e.g., in url.toURI()), the finally block still executes safely. However, using try-with-resources would be more idiomatic and safer for resource management. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TwoShardReduceTestCase.java [360-384] -FileSystem fs = null; -try { - URI uri = url.toURI(); - Path dir; - if ("jar".equals(uri.getScheme())) { - fs = FileSystems.newFileSystem(uri, Collections.emptyMap()); +URI uri = url.toURI(); +Path dir; +if ("jar".equals(uri.getScheme())) { + try (FileSystem fs = FileSystems.newFileSystem(uri, Collections.emptyMap())) { dir = fs.getPath(resourceDir); - } else { - dir = PathUtils.get(uri); + try (Stream<Path> stream = Files.list(dir)) { + stream.forEach(p -> { + String fileName = p.getFileName().toString(); + if (fileName.endsWith(".ppl")) { + names.add(fileName.substring(0, fileName.length() - ".ppl".length())); + } + }); + } } - ... -} finally { - if (fs != null) { - fs.close(); +} else { + dir = PathUtils.get(uri); + try (Stream<Path> stream = Files.list(dir)) { + stream.forEach(p -> { + String fileName = p.getFileName().toString(); + if (fileName.endsWith(".ppl")) { + names.add(fileName.substring(0, fileName.length() - ".ppl".length())); + } + }); } } Suggestion importance[1-10]: 6 __ Why: Valid suggestion to use try-with-resources for better resource management. The current manual approach is correct but less idiomatic. This improves code maintainability and reduces the risk of resource leaks, though the existing code is functionally correct.	Low
	Deep copy rows before sorting The shallow copy of row lists doesn't protect against mutation of the nested row objects themselves. If RowComparator or downstream code modifies individual rows (e.g., sorting multi-value cells), the caller's data is still affected. Consider deep-copying the rows or documenting this limitation. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/ResponseValidator.java [133-137] -// Copy before sorting so we never reorder the caller's lists. -expectedRows = new java.util.ArrayList<>(expectedRows); -actualRows = new java.util.ArrayList<>(actualRows); +// Deep copy before sorting so we never reorder or mutate the caller's data. +expectedRows = deepCopyRows(expectedRows); +actualRows = deepCopyRows(actualRows); expectedRows.sort(new RowComparator()); actualRows.sort(new RowComparator()); +// Helper method to deep copy rows +private static List<List<Object>> deepCopyRows(List<List<Object>> rows) { + List<List<Object>> copy = new ArrayList<>(rows.size()); + for (List<Object> row : rows) { + copy.add(new ArrayList<>(row)); + } + return copy; +} + Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that shallow copying doesn't protect nested row objects. However, the PR already includes normalizeArrayCells which sorts multi-value cells, so this is a real concern. The impact is moderate since it affects test data integrity but doesn't cause incorrect test results.	Low

---

Previous suggestions

Suggestions up to commit 83c90c7

Category	Suggestion	Impact
Possible issue	Handle null exception messages safely The rootMessage method may return null when r.getMessage() is null, causing potential NullPointerException in string concatenation contexts. Add null-safety by using String.valueOf() or providing a default message when the root cause message is null. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TwoShardReduceTestCase.java [320-326] private static String rootMessage(Throwable t) { Throwable r = t; while (r.getCause() != null && r.getCause() != r) { r = r.getCause(); } - return r.getClass().getSimpleName() + ": " + r.getMessage(); + String message = r.getMessage(); + return r.getClass().getSimpleName() + ": " + (message != null ? message : "<no message>"); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that r.getMessage() can return null, which would result in the string "null" being concatenated. Adding explicit null-handling improves robustness and provides clearer error messages.	Low
General	Handle exceptions in stream operations The forEach operation on the stream may throw unchecked exceptions (e.g., from toString() or add()), which won't be properly wrapped by the outer IOException. Consider using a traditional loop or explicitly handling exceptions within the lambda to ensure proper error propagation. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TwoShardReduceTestCase.java [370-377] try (Stream<Path> stream = Files.list(dir)) { stream.forEach(p -> { - String fileName = p.getFileName().toString(); - if (fileName.endsWith(".ppl")) { - names.add(fileName.substring(0, fileName.length() - ".ppl".length())); + try { + String fileName = p.getFileName().toString(); + if (fileName.endsWith(".ppl")) { + names.add(fileName.substring(0, fileName.length() - ".ppl".length())); + } + } catch (Exception e) { + throw new RuntimeException("Failed to process file: " + p, e); } }); } Suggestion importance[1-10]: 4 __ Why: The suggestion addresses a potential issue where exceptions in the lambda might not be properly propagated. However, the operations (toString(), add()) are unlikely to throw exceptions in normal circumstances, making this a minor defensive improvement rather than a critical fix.	Low

[github-actions]

Persistent review updated to latest commit a577ad1

[github-actions]

✅ Gradle check result for a577ad1: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.53%. Comparing base (4cafb38) to head (a577ad1).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21951      +/-   ##
============================================
+ Coverage     73.43%   73.53%   +0.10%     
- Complexity    75527    75643     +116     
============================================
  Files          6035     6035              
  Lines        342721   342720       -1     
  Branches      49301    49301              
============================================
+ Hits         251676   252020     +344     
+ Misses        71018    70718     -300     
+ Partials      20027    19982      -45     

☔ View full report in Codecov by Sentry.
📢 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.

[vinaykpud]

Why IpFieldMultiShardIT is different from other two shards tests which are extending TwoShardReduceTestCase.java?

[vinaykpud]

I found ip_multishard (this PR) and clickbench (plus big5 from #21953) are pinned to number_of_shards: 2 in their mapping files and never get a 1-shard run. Everything else including merge_coverage, says number_of_shards: 1. But merge_coverage also runs with 2 shard.

I think mapping file ends up being a misleading source of truth, and I think it's worth deciding whether we want to align on a single convention before more reduce-style suites get added on top. Curious what you think.

[sandeshkr419]

Merging this change to get the sandbox healthy.

@mch2 Can you please address the comments in a quick follow-up.