test(integ-test): stabilize deterministic fixtures across shards

[mengweieric]

Summary

A set of integration tests selected rows through head, limit, shard-local representative choice, or unordered collection output, then asserted exact single-shard results.

This change selects intended fixture rows with unique keys, compares unordered collections as multisets, and uses numeric tolerances only for distributed floating-point or geo-point encoding differences. Schema, cardinality, grouping, source-value, and command-specific assertions remain exact.

No production behavior is modified.

Validation

• Verified on an external cluster forced to five primary shards
• Exercised direct, paginating, and independently checked no-pushdown paths as applicable
• All modified tests passed targeted five-shard validation
• spotlessCheck, compileTestJava, and git diff --check pass

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 91bfdd2)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The test testJoinWithFieldListMaxEqualsOne now asserts only 4 deterministic rows and checks total count is 5, but does not verify David's country. If David's country is genuinely undefined due to max=1 selecting an arbitrary match, the test should explicitly document that both USA and Canada are valid outcomes. Without this, a future reader may incorrectly assume the test is incomplete or that David's country should be deterministic. The same issue appears in testJoinWhenLegacyNotPreferred. schema("month", "int"), schema("occupation", "string"), schema("salary", "int")); JSONObject actual2 = executeQuery( String.format( "source=%s | join type=inner max=1 name,year,month %s | fields name, country", TEST_INDEX_STATE_COUNTRY, TEST_INDEX_OCCUPATION)); // max=1 keeps a single OCCUPATION match per left row, and the co-named country column resolves // to the OCCUPATION (right) side (see Jake -> England). David has two OCCUPATION rows that are // identical on the join field list (name=David, year=2023, month=4) and differ only in the // projected country (USA from the Doctor row, Canada from the Unemployed row), so which one // survives max=1 is undefined and cannot be disambiguated by any ordering. Assert the four // deterministic rows and that max=1 collapses David's two matches to a single row (total of 5, // not 6); do not pin David's undefined country. verifyNumOfRows(actual2, 5); verifyDataRowsSome( actual2, rows("Jake", "England"), rows("Jane", "Canada"), rows("John", "Canada"), rows("Hello", "USA")); } Possible Issue The new rowsUnordered matcher recursively compares JSON values treating all arrays as unordered multisets. If a test legitimately requires a specific array order (e.g., a sorted result or a sequence where order matters), this matcher will incorrectly pass even when the order is wrong. The matcher is scoped to this test class, but if copied elsewhere or if a future test in this class needs ordered array comparison, it will silently accept incorrect results. private static TypeSafeMatcher<JSONArray> rowsUnordered(Object... expectedObjects) { return new TypeSafeMatcher<>() { @Override protected boolean matchesSafely(JSONArray array) { JSONArray expected = new JSONArray(expectedObjects); if (array.length() != expected.length()) { return false; } // Compare top-level cells positionally so column identity is preserved; only descend into // nested arrays/objects with order-insensitive comparison. for (int i = 0; i < expected.length(); i++) { if (!jsonEqualsIgnoringOrder(array.get(i), expected.get(i))) { return false; } } return true; } @Override public void describeTo(Description description) { description.appendText(new JSONArray(expectedObjects).toString()); } }; } Possible Issue The verifyDataRows method now compares rows as multisets by sorting their string representations. If two rows have identical string serializations but differ in actual structure (e.g., nested JSON objects with different key orders that serialize identically), this comparison will falsely report them as equal. This could mask genuine differences in row content. public void verifyDataRows(JSONArray dataRowsOne, JSONArray dataRowsTwo) { // The paginated and non-paginated APIs must return the same complete set of rows, but none of // these queries impose a total ordering: three have no ORDER BY at all, and the ORDER BY // variants sort on a non-unique column (balance), so the order among equal-key rows is not // contractual. On a multi-shard index the scroll (paginated) and search (non-paginated) paths // materialize the same rows in different orders, so JSONArray.similar()'s positional comparison // fails even though the row sets are equivalent. Compare as multisets to assert complete-row // equivalence (full coverage and totals) without depending on an order that is not contractual. List<String> rowsOne = new ArrayList<>(); dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add(o.toString())); List<String> rowsTwo = new ArrayList<>(); dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add(o.toString())); rowsOne.sort(null); rowsTwo.sort(null); assertEquals(rowsOne, rowsTwo); }

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 91bfdd2

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use tolerance for floating-point comparison Comparing floating-point numbers with == can produce false negatives due to precision errors. Use a tolerance-based comparison (e.g., Math.abs(a - b) < EPSILON) to handle rounding differences that arise from different accumulation orders across shards. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] +private static final double EPSILON = 1e-9; + private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null || a == JSONObject.NULL; boolean bNull = b == null || b == JSONObject.NULL; if (aNull || bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double aVal = ((Number) a).doubleValue(); + double bVal = ((Number) b).doubleValue(); + return Math.abs(aVal - bVal) < EPSILON; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue with floating-point comparison using ==. However, the scalarEquals method is used within a custom matcher for comparing JSON values in tests, where exact equality is often acceptable for most numeric values. The tolerance-based approach would be more robust for floating-point comparisons, but the current implementation may be intentional for test assertions. The score reflects that this is a valid improvement for numerical stability, though not critical given the test context.	Medium

---

Previous suggestions

Suggestions up to commit 6ba546d

Category	Suggestion	Impact
General	Use tolerance-based floating-point comparison Floating-point equality comparison using == is unreliable due to precision issues. Replace the direct equality check with a tolerance-based comparison (e.g., Math.abs(diff) < EPSILON) to avoid false negatives when comparing numeric values that should be considered equal. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null || a == JSONObject.NULL; boolean bNull = b == null || b == JSONObject.NULL; if (aNull || bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double diff = ((Number) a).doubleValue() - ((Number) b).doubleValue(); + return Math.abs(diff) < 1E-9; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue with floating-point equality comparison using ==. However, the context shows this is a test utility for comparing JSON values where exact equality may be acceptable for most test cases. The suggested tolerance-based comparison would improve robustness, but the impact is moderate since this is test code and the current implementation may be sufficient for the test scenarios.	Medium

Suggestions up to commit 31f8a2a

Category	Suggestion	Impact
General	Use tolerance for floating-point comparison Comparing floating-point numbers with == can produce incorrect results due to precision issues. Use a tolerance-based comparison (e.g., Math.abs(diff) < EPSILON) to handle rounding errors properly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null || a == JSONObject.NULL; boolean bNull = b == null || b == JSONObject.NULL; if (aNull || bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double aVal = ((Number) a).doubleValue(); + double bVal = ((Number) b).doubleValue(); + return Math.abs(aVal - bVal) < 1e-9; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: Valid concern about floating-point comparison precision. The suggestion to use tolerance-based comparison (Math.abs(aVal - bVal) < 1e-9) is appropriate for test assertions where rounding errors can occur. This improves robustness without changing the test's intent.	Medium
	Use structural JSON comparison Converting JSON objects to strings for comparison can produce false positives when different JSON structures serialize to identical strings. Use a structural comparison that respects JSON semantics instead of string equality. integ-test/src/test/java/org/opensearch/sql/legacy/CursorIT.java [501-516] public void verifyDataRows(JSONArray dataRowsOne, JSONArray dataRowsTwo) { ... - List<String> rowsOne = new ArrayList<>(); - dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add(o.toString())); - List<String> rowsTwo = new ArrayList<>(); - dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add(o.toString())); - rowsOne.sort(null); - rowsTwo.sort(null); - assertEquals(rowsOne, rowsTwo); + List<JSONArray> rowsOne = new ArrayList<>(); + dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add((JSONArray) o)); + List<JSONArray> rowsTwo = new ArrayList<>(); + dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add((JSONArray) o)); + rowsOne.sort(Comparator.comparing(JSONArray::toString)); + rowsTwo.sort(Comparator.comparing(JSONArray::toString)); + assertEquals(rowsOne.size(), rowsTwo.size()); + for (int i = 0; i < rowsOne.size(); i++) { + assertTrue(rowsOne.get(i).similar(rowsTwo.get(i))); + } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that string comparison of JSON can be fragile. Using JSONArray.similar() for structural comparison is more robust. However, the implementation still sorts by toString() which could be problematic. A better approach would use a custom comparator that handles JSON structure, but the suggested improvement is still an upgrade over pure string equality.	Low
	Optimize array comparison algorithm The nested loop with iterator removal has O(n²) time complexity for array comparison. For large arrays with many elements, this could cause performance issues. Consider using a frequency map approach to achieve O(n) complexity while still handling duplicates correctly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [146-169] private static boolean jsonArrayEqualsIgnoringNestedOrder(JSONArray actual, JSONArray expected) { if (actual.length() != expected.length()) { return false; } - List<Object> remaining = new ArrayList<>(); + Map<String, Integer> expectedCounts = new HashMap<>(); for (int i = 0; i < expected.length(); i++) { - remaining.add(expected.get(i)); + String key = expected.get(i).toString(); + expectedCounts.merge(key, 1, Integer::sum); } for (int i = 0; i < actual.length(); i++) { - Object actualElement = actual.get(i); - boolean matched = false; - for (Iterator<Object> it = remaining.iterator(); it.hasNext(); ) { - if (jsonEqualsIgnoringOrder(actualElement, it.next())) { - it.remove(); - matched = true; - break; - } - } - if (!matched) { + String key = actual.get(i).toString(); + Integer count = expectedCounts.get(key); + if (count == null || count == 0) { return false; } + expectedCounts.put(key, count - 1); } return true; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies a performance concern with the O(n²) nested loop. However, the proposed frequency map approach using toString() as keys would break the deep structural comparison that jsonEqualsIgnoringOrder provides. The current implementation correctly handles nested arrays/objects recursively, while the suggested approach would only compare serialized strings, losing semantic correctness.	Low

Suggestions up to commit b9c4c8a

Category	Suggestion	Impact
Possible issue	Handle type mismatches in recursive comparison The recursive comparison does not handle the case where a and b are of different types (e.g., one is a JSONArray and the other is a JSONObject). This will fall through to scalarEquals, which may produce incorrect results. Add an explicit type mismatch check before the recursive calls. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [116-134] private static boolean jsonEqualsIgnoringOrder(Object a, Object b) { if (a instanceof JSONArray && b instanceof JSONArray) { return jsonArrayEqualsIgnoringNestedOrder((JSONArray) a, (JSONArray) b); + } + if (a instanceof JSONArray || b instanceof JSONArray) { + return false; } if (a instanceof JSONObject && b instanceof JSONObject) { JSONObject ao = (JSONObject) a; JSONObject bo = (JSONObject) b; if (ao.keySet().size() != bo.keySet().size()) { return false; } for (String key : ao.keySet()) { if (!bo.has(key) || !jsonEqualsIgnoringOrder(ao.get(key), bo.get(key))) { return false; } } return true; } + if (a instanceof JSONObject || b instanceof JSONObject) { + return false; + } return scalarEquals(a, b); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the method doesn't explicitly handle type mismatches between JSONArray and JSONObject before falling through to scalarEquals. Adding explicit checks prevents incorrect comparisons when a and b are of different JSON container types, improving correctness and clarity.	Medium

Suggestions up to commit 60e3690

Category	Suggestion	Impact
General	Handle type mismatch in comparison The method does not handle the case where a and b are of different types (e.g., one is a JSONArray and the other is a JSONObject). This will cause the method to fall through to scalarEquals, which may produce incorrect results. Add an explicit type mismatch check before the scalar comparison. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [116-134] private static boolean jsonEqualsIgnoringOrder(Object a, Object b) { if (a instanceof JSONArray && b instanceof JSONArray) { return jsonArrayEqualsIgnoringNestedOrder((JSONArray) a, (JSONArray) b); } if (a instanceof JSONObject && b instanceof JSONObject) { JSONObject ao = (JSONObject) a; JSONObject bo = (JSONObject) b; if (ao.keySet().size() != bo.keySet().size()) { return false; } for (String key : ao.keySet()) { if (!bo.has(key) || !jsonEqualsIgnoringOrder(ao.get(key), bo.get(key))) { return false; } } return true; } + if ((a instanceof JSONArray || a instanceof JSONObject) != (b instanceof JSONArray || b instanceof JSONObject)) { + return false; + } return scalarEquals(a, b); } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that type mismatches between JSONArray and JSONObject are not explicitly handled before falling through to scalarEquals. However, the current implementation already handles this implicitly: if a and b are different types (one is JSONArray, the other is JSONObject), neither of the first two conditions will match, and scalarEquals will return false via toString().equals(). The explicit check adds clarity but doesn't fix a bug.	Low

[github-actions]

Persistent review updated to latest commit b9c4c8a

[github-actions]

Persistent review updated to latest commit 31f8a2a

[github-actions]

Persistent review updated to latest commit 6ba546d

[github-actions]

Persistent review updated to latest commit 91bfdd2