test(integ-test): stabilize stream-order commands across shards

[mengweieric]

Summary

Several Streamstats, Reverse, Dedup, and Patterns tests relied on the incidental encounter order of a single-shard index. On multiple shards, the commands returned valid results for a different stream order and the tests asserted different row content.

This change uses deterministic makeresults streams where exact order is part of the test, and membership/cardinality assertions where representative selection is not defined. Real multi-shard index coverage remains through order-independent property checks. Tests that require nullable numeric streams or expose known engine gaps are intentionally unchanged.

No production behavior is modified.

Validation

• Verified on an external cluster forced to five primary shards
• Exercised direct and no-pushdown paths
• Affected suite failures reduced from 104 to 40; remaining failures are documented engine/contract gaps
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bbe59ba)

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]

PR Code Suggestions ✨

Latest suggestions up to bbe59ba
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Validate dedup count limit The test verifies dedup 2 name KEEPEMPTY=true but does not validate that non-null names appear at most twice. Add an assertion that each name count in nameCounts is at most 2 to ensure the dedup limit is respected. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLDedupIT.java [208-219] Map<Object, Integer> nameCounts = new HashMap<>(); Set<List<Object>> nullNameRows = new HashSet<>(); for (List<Object> row : rows) { Object name = row.get(0); Object category = row.get(1); if (name == null) { nullNameRows.add(Arrays.asList(name, category)); } else { nameCounts.merge(name, 1, Integer::sum); assertValidPair(name, category); } } +for (Map.Entry<Object, Integer> e : nameCounts.entrySet()) { + assertTrue("name " + e.getKey() + " appears more than twice", e.getValue() <= 2); +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that testDedupKeepEmpty2 verifies dedup 2 name KEEPEMPTY=true but does not explicitly assert that each non-null name appears at most twice. Adding this assertion would strengthen the test by ensuring the dedup limit is respected, making the test more robust against potential regressions.	Medium
	Validate null-bucket exclusion behavior The query chains two streamstats commands with bucket_nullable=false, but the test does not verify that null-bucket rows are excluded from aggregation. Add assertions to confirm that rows with null partition keys have null aggregate values when bucket_nullable=false. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java [262-268] JSONObject actual2 = executeQuery( String.format( "source=%s | sort seq | streamstats bucket_nullable=false avg(age) as avg_age by" + " state, country | streamstats bucket_nullable=false avg(avg_age) as" + " avg_state_age by country | fields name, country, state, month, year, age," + " avg_age, avg_state_age", TEST_INDEX_STATE_COUNTRY_WITH_NULL_ORDERED)); +// Verify that null-bucket rows have null aggregates when bucket_nullable=false +List<List<Object>> rows = dataRows(actual2); +for (List<Object> row : rows) { + Object country = row.get(1); + Object avgStateAge = row.get(7); + if (country == null) { + assertNull("null country should have null avg_state_age with bucket_nullable=false", avgStateAge); + } +} + Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that testStreamstatsByWithNullBucket uses bucket_nullable=false but does not explicitly verify that null-bucket rows have null aggregate values. Adding this assertion would improve test coverage by confirming the expected null-handling behavior, making the test more comprehensive.	Medium
	Verify sample count constraint The test expects exactly 3 sampled emails but does not verify that max_sample_count=3 is respected when fewer than 3 matching documents exist. Add a check that the sample size does not exceed the available document count for the pattern. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLPatternsIT.java [149-153] List<String> samples = asStringList(row.get(2)); -assertEquals(3, samples.size()); +assertTrue("sample size exceeds max_sample_count", samples.size() <= 3); for (String s : samples) { assertTrue("not an email: " + s, EMAIL.matcher(s).matches()); } Suggestion importance[1-10]: 6 __ Why: The suggestion proposes changing the exact size assertion (assertEquals(3, samples.size())) to a less-than-or-equal check (assertTruesamples.size() <= 3)). However, the test comment states "which max_sample_count emails land in the sample is not [deterministic]", implying the count itself (3) is stable. The suggestion addresses a valid edge case but may weaken the test by allowing fewer samples when 3 are expected.	Low

---

Previous suggestions

Suggestions up to commit edc0d81

Category	Suggestion	Impact
Possible issue	Add null check for server lookup The test assumes messagesByServer.get(row.getString(0)) always returns a non-null set, but if an unexpected server name appears in the data, this will cause a NullPointerException. Add a null check or assertion to fail gracefully with a clear error message. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java [1531-1545] -Map<String, Set<String>> messagesByServer = - Map.of( - "server1", Set.of("Database connection failed", "High memory usage"), - "server2", Set.of("Service started", "Backup completed"), - "server3", Set.of("Disk space low")); -JSONArray rows = actual.getJSONArray("datarows"); -assertEquals(5, rows.length()); for (int i = 0; i < rows.length(); i++) { JSONArray row = rows.getJSONArray(i); - Set<String> validMessages = messagesByServer.get(row.getString(0)); + String server = row.getString(0); + Set<String> validMessages = messagesByServer.get(server); + assertNotNull("unexpected server: " + server, validMessages); assertTrue(validMessages.contains(row.getString(1))); assertTrue(validMessages.contains(row.getString(2))); assertTrue(validMessages.contains(row.getString(3))); } Suggestion importance[1-10]: 7 __ Why: The code assumes messagesByServer.get(row.getString(0)) returns a non-null set, which could cause a NullPointerException if an unexpected server appears. Adding a null check would make the test fail with a clearer error message, improving debuggability.	Medium
General	Extract duplicated helper method The dataRows helper method is duplicated across multiple test files. Consider extracting this common JSON-to-list conversion logic into a shared test utility class to reduce code duplication and improve maintainability. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLDedupIT.java [582-594] -JSONArray arr = response.getJSONArray("datarows"); -for (int i = 0; i < arr.length(); i++) { - JSONArray r = arr.getJSONArray(i); - List<Object> row = new ArrayList<>(); - for (int j = 0; j < r.length(); j++) { - row.add(r.isNull(j) ? null : r.get(j)); - } - rows.add(row); -} +// Extract to shared utility class (e.g., TestUtils.dataRows(response)) +return TestUtils.dataRows(response); Suggestion importance[1-10]: 6 __ Why: The dataRows helper is duplicated in multiple test files (CalcitePPLDedupIT and CalcitePPLPatternsIT). Extracting it to a shared utility would reduce duplication and improve maintainability, though the impact is moderate since it's test code.	Low
	Remove redundant email validation The email validation is redundant since the token reconstruction already verifies the structure. If the tokens reconstruct the sample exactly and the pattern is @., the email format is implicitly validated. Remove the redundant regex check to simplify the assertion. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLPatternsIT.java [189-192] for (int i = 0; i < samples.size(); i++) { assertEquals(samples.get(i), t1.get(i) + "@" + t2.get(i) + "." + t3.get(i)); - assertTrue("not an email: " + samples.get(i), EMAIL.matcher(samples.get(i)).matches()); } Suggestion importance[1-10]: 4 __ Why: The email regex validation is indeed redundant given the token reconstruction already verifies the exact structure. However, the redundancy provides an additional safety check and the performance impact is negligible in tests, so removing it offers only minor benefit.	Low

[penghuo]

The test set not human readable. 2 ideas

• change take(content, 1) to min/max.
• add containsInAnyOrder

[mengweieric]

Updated this to use max(content), which makes the sample deterministic across shards. verifyDataRows already performs an order insensitive comparison.

[penghuo]

does sort help?

[mengweieric]

Yes. I added sort name, category before dedup and replaced the permissive valid pair check with exact expected rows.

[github-actions]

Persistent review updated to latest commit bbe59ba