Fix boolean coercion from numeric values for wildcard index queries (#5269)

[qianheng-aws]

Description

[BugFix] When querying across indices with conflicting field mappings (boolean vs text) using wildcard patterns like source = repro*, numeric values such as 0 stored in the text-typed index were not coerced to boolean, causing OpenSearchParseException: node must be a boolean, found NUMBER.

Root cause: OpenSearchJsonContent.parseBooleanValue() only handled isBoolean() and isTextual() nodes but not numeric nodes. Added isNumber() handling consistent with ObjectContent.booleanValue() which already does ((Number) value).intValue() != 0.

Related Issues

Resolves #5269

Check List

• New functionality includes testing
• Javadoc added for new public API
• Commits signed per DCO (-s)
• spotlessCheck passed
• Unit tests passed
• Integration tests passed
• Grammar changes: all three .g4 files synced (if applicable)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5a78b78)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Core fix for boolean coercion from numeric values in OpenSearchJsonContent Relevant files: opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java Sub-PR theme: Integration tests for wildcard index boolean/text mapping conflict Relevant files: integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5269.yml

⚡ Recommended focus areas for review Floating Point Handling The fix uses node.intValue() for numeric nodes, which truncates floating-point values (e.g., 0.5 would become 0 and thus false). Consider whether node.doubleValue() != 0 or node.numberValue().doubleValue() != 0 would be more appropriate to handle all numeric types consistently, similar to how other languages/frameworks handle truthy numeric values. } else if (node.isNumber()) { return node.intValue() != 0; Test Assertion The integration test only checks that 2 rows are returned but does not validate the actual boolean values of the flag field. It would be more robust to also assert that the coerced boolean values are correct (e.g., false for numeric 0 and false for the boolean false). JSONObject result = executeQuery("source=repro_bool_test_* | fields flag"); assertEquals(2, result.getJSONArray("datarows").length());

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5a78b78

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle floating-point numeric boolean coercion correctly Using node.intValue() on a floating-point number (e.g., 0.5) will truncate to 0, incorrectly returning false. Consider using node.doubleValue() != 0 or node.numberValue().doubleValue() != 0 to correctly handle all numeric types including floats and doubles. opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java [215-216] } else if (node.isNumber()) { - return node.intValue() != 0; + return node.doubleValue() != 0; } Suggestion importance[1-10]: 6 __ Why: Using node.intValue() on floating-point numbers truncates to 0, causing incorrect false results for values like 0.5. Switching to node.doubleValue() != 0 would correctly handle all numeric types, though the primary use case in the PR is integer values (0/1).	Low
General	Validate coerced boolean values in integration test The test only verifies the count of rows but does not validate the actual boolean values returned. Adding assertions on the content of the datarows would make the test more robust and ensure the boolean coercion is producing correct values (e.g., false for 0 and false for the boolean false). integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java [180-182] // Query across both indices with wildcard — should not throw an error JSONObject result = executeQuery("source=repro_bool_test_* | fields flag"); -assertEquals(2, result.getJSONArray("datarows").length()); +JSONArray datarows = result.getJSONArray("datarows"); +assertEquals(2, datarows.length()); +// Verify both rows contain boolean false values (flag=false and flag=0) +for (int i = 0; i < datarows.length(); i++) { + assertEquals(false, datarows.getJSONArray(i).getBoolean(0)); +} Suggestion importance[1-10]: 4 __ Why: The suggestion improves test coverage by validating actual boolean values rather than just the row count, but the order of results from a wildcard query may not be deterministic, making the assertion on specific values potentially fragile.	Low

---

Previous suggestions

Suggestions up to commit 614bd7d

Category	Suggestion	Impact
Possible issue	Handle floating-point numeric boolean coercion correctly Using node.intValue() on a floating-point number (e.g., 0.5) will truncate to 0, incorrectly returning false. Consider using node.doubleValue() != 0 or node.numberValue().doubleValue() != 0 to correctly handle all numeric types including floats and doubles. opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java [215-216] } else if (node.isNumber()) { - return node.intValue() != 0; + return node.doubleValue() != 0; } Suggestion importance[1-10]: 6 __ Why: Using node.intValue() truncates floating-point numbers (e.g., 0.5 becomes 0, returning false), while node.doubleValue() != 0 would correctly handle all numeric types. This is a valid edge case, though the primary use case in the PR is integer values like 0 and 1.	Low
General	Assert actual coerced boolean values in test The test only verifies the count of returned rows but does not validate the actual boolean values returned. Adding assertions on the actual flag values would make the test more meaningful and ensure the coercion produces correct results (e.g., false for numeric 0). integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java [181-182] JSONObject result = executeQuery("source=repro_bool_test_* | fields flag"); -assertEquals(2, result.getJSONArray("datarows").length()); +JSONArray datarows = result.getJSONArray("datarows"); +assertEquals(2, datarows.length()); +// Verify that both rows contain valid boolean values (false) +for (int i = 0; i < datarows.length(); i++) { + assertFalse(datarows.getJSONArray(i).getBoolean(0)); +} Suggestion importance[1-10]: 4 __ Why: The suggestion to validate actual flag values improves test coverage, but the improved code asserts false for both rows, which is incorrect since the indexBool document has flag: false and indexText has flag: 0 (both false), but the ordering isn't guaranteed. The suggestion has merit but the implementation may be fragile.	Low

Suggestions up to commit d237222

Category	Suggestion	Impact
Possible issue	Fix numeric boolean coercion for floating-point values Using intValue() on a floating-point number (e.g., 0.5) will truncate to 0, incorrectly returning false. Use doubleValue() or node.numberValue().doubleValue() to correctly handle all numeric types, or check node.asDouble() != 0 to avoid truncation issues. opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java [215-216] } else if (node.isNumber()) { - return node.intValue() != 0; + return node.doubleValue() != 0; } Suggestion importance[1-10]: 6 __ Why: Using intValue() on floating-point numbers truncates to integer, causing 0.5 to incorrectly return false. Using doubleValue() would handle all numeric types correctly, though the current PR's use case (integer 0 or 1) may not be affected in practice.	Low
General	Ensure both indices are cleaned up in teardown If the first DELETE request throws an exception, the second index will not be cleaned up, leaving test state behind. Add ignore_unavailable or wrap each delete in its own try-catch to ensure both indices are always deleted. integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java [184-185] -client().performRequest(new Request("DELETE", "/" + indexBool)); -client().performRequest(new Request("DELETE", "/" + indexText)); +try { + client().performRequest(new Request("DELETE", "/" + indexBool + "?ignore_unavailable=true")); +} finally { + client().performRequest(new Request("DELETE", "/" + indexText + "?ignore_unavailable=true")); +} Suggestion importance[1-10]: 4 __ Why: If the first DELETE request fails, the second index won't be cleaned up. Wrapping each delete in a nested try-finally ensures both indices are always removed, improving test reliability.	Low

[github-actions]

Persistent review updated to latest commit 614bd7d

[penghuo]

claude is co-author. Not sure the guideline. Remove it if not intention.

[github-actions]

Persistent review updated to latest commit 5a78b78

[qianheng-aws]

Removed the Co-Authored-By line — it was unintentional. Updated in the latest force-push.

[yuancu]

LGTM