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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a

[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 31f8a2a)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 60e3690

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31f8a2a

Explore these optional code suggestions:

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

---

Previous suggestions

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

[dai-chen]

I see most are caused by head and we fix by adding sort or where. Just thinking any way to enforce this in our IT?

[mengweieric]

Good question. OpenSearch core handles this behaviorally rather than with a static rule. Its integration-test framework randomizes index shard counts from 1 to 10, and tests choose ordered or order-insensitive assertions based on the contract. A static head rule would flag valid deterministic cases and miss non-head sources of shard-order dependence, so I opened #5738 to track a route-independent multi-shard SQL CI lane: #5738

[github-actions]

Persistent review updated to latest commit 31f8a2a