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 42b7e37)

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 b558d05

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate row size before access The code assumes row always has at least 2 elements when accessing row.get(0) and row.get(1). If the data structure changes or returns fewer columns, this will throw an IndexOutOfBoundsException. Add a size check before accessing elements. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLDedupIT.java [210-219] for (List<Object> row : rows) { + if (row.size() < 2) { + throw new AssertionError("Expected at least 2 columns, got " + row.size()); + } 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); } } Suggestion importance[1-10]: 6 __ Why: Adding a size check before accessing row.get(0) and row.get(1) is a reasonable defensive programming practice. The suggestion correctly identifies a potential IndexOutOfBoundsException if the data structure changes. However, in a test context where the schema is controlled, this is less critical than in production code.	Low
	Add bounds check before access Accessing row.get(2) without verifying that the row has at least 3 elements can cause an IndexOutOfBoundsException. Add a bounds check before accessing the third element to prevent runtime failures. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLPatternsIT.java [149-153] +assertTrue("Expected at least 3 columns in row", row.size() >= 3); List<String> samples = asStringList(row.get(2)); assertEquals(3, samples.size()); for (String s : samples) { assertTrue("not an email: " + s, EMAIL.matcher(s).matches()); } Suggestion importance[1-10]: 6 __ Why: The suggestion to add a bounds check before accessing row.get(2) is valid and prevents potential IndexOutOfBoundsException. This is a reasonable defensive measure, though in a controlled test environment with a fixed schema, the risk is lower. The suggestion improves robustness.	Low
	Validate array length before indexing Directly accessing array indices 0 and 1 without checking the array length can throw JSONException if a row has fewer than 2 elements. Validate each row's length before accessing its elements to prevent runtime errors. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteStreamstatsCommandIT.java [1659-1662] for (int i = 0; i < datarows.length(); i++) { - ages.add(datarows.getJSONArray(i).getInt(0)); - cnts.add(datarows.getJSONArray(i).getInt(1)); + JSONArray row = datarows.getJSONArray(i); + if (row.length() < 2) { + throw new AssertionError("Expected at least 2 columns in row " + i + ", got " + row.length()); + } + ages.add(row.getInt(0)); + cnts.add(row.getInt(1)); } Suggestion importance[1-10]: 6 __ Why: Adding a length check before accessing row.getInt(0) and row.getInt(1) is a valid defensive practice to prevent JSONException. The suggestion correctly identifies a potential runtime error. However, in a test with a controlled schema, the risk is lower, making this a moderate improvement.	Low
General	Clarify max() usage for sampling Using max(content) to select a representative log sample is semantically misleading. While it provides determinism, max() suggests a meaningful maximum value, but here it's used as an arbitrary selection mechanism. Consider using a more explicit approach or documenting this pattern selection strategy. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLDashboardPatternsIT.java [37] -+ " | stats count() as pattern_count, max(content) as sample_logs" ++ " | stats count() as pattern_count, max(content) as sample_logs /* deterministic sample */" Suggestion importance[1-10]: 4 __ Why: The suggestion to add a comment clarifying that max(content) is used for deterministic sampling is valid and improves code readability. However, the impact is minor since the surrounding comment already explains the determinism rationale. The suggestion is correct but offers only marginal improvement.	Low

---

Previous suggestions

Suggestions up to commit bbe59ba

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

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

[github-actions]

Persistent review updated to latest commit bbe59ba

[mengweieric]

@penghuo The two review points are addressed at the current head: the Dashboard Patterns sample now uses deterministic max(content), and the dedup case sorts before asserting exact rows. Could you take another look when you have a chance?

[github-actions]

Persistent review updated to latest commit b558d05

[github-actions]

Persistent review updated to latest commit 42b7e37