[analytics-engine] Add json_valid Rust UDF to the DataFusion backend

[mengweieric]

Description

Adds the PPL json_valid scalar function to the analytics-engine (DataFusion) route, so json_valid(...) runs on parquet-backed indices instead of failing with No backend supports scalar function [JSON_VALID].

Wires it through the full chain: ScalarFunction.JSON_VALID enum, JsonValidAdapter (rewrites Calcite's IS_JSON_VALUE postfix operator → a local json_valid op), the project- and filter-op registries, the substrait FunctionMapping, the opensearch_scalar_functions.yaml signature, and the Rust UDF (rust/src/udf/json_valid.rs).

Complements #22130 (which adds json / json_object / json_array); json_valid is independent and not covered there.

Semantics

Match the legacy SQL-plugin JsonUtils.isValidJson (Jackson ObjectMapper.readTree):

• valid JSON → true
• malformed input → false
• NULL / missing input → false (not NULL) — legacy returns LITERAL_FALSE for null/missing, so where not json_valid(col) includes NULL rows (matches JsonFunctionsIT.test_not_json_valid).
• empty / whitespace-only input → true — Jackson readTree("") returns a MissingNode without throwing, so the legacy function and the JsonFunctionsIT "json empty string" fixture row treat it as valid. serde_json::from_str rejects empty input, so is_valid_json special-cases empty/whitespace to preserve parity.

Filter support

json_valid returns BOOLEAN, so it is registered as a filter op (not only project), letting it work as a WHERE predicate — where json_valid(col) / where not json_valid(col) — same shape as cidrmatch.

Testing

Unit

• Rust UDF tests: 10/10 — valid/malformed/NULL/empty-whitespace, scalar + columnar paths, Utf8/LargeUtf8/Utf8View coercion, arity/non-string rejection.
• Java adapter tests (JsonFunctionAdaptersTests): 9/9 — rewrite-to-local-op + original-return-type preservation.
• ScalarFunctionTests: 19/19 — incl. a new test pinning the production resolver path SqlStdOperatorTable.IS_JSON_VALUE (SqlKind.OTHER) → reference-operator identity → ScalarFunction.JSON_VALID.

End-to-end (force-routed analytics-engine cluster, -Dtests.analytics.parquet_indices=true, AE route confirmed via _explain) — all green:

• JsonFunctionsIT.test_json_valid ✅ — where json_valid(json_string) over the full json_test dataset (incl. empty-string → valid).
• JsonFunctionsIT.test_not_json_valid ✅ — where not json_valid(...) (incl. NULL → false, so NULL rows are returned).
• CalcitePPLJsonBuiltinFunctionIT.testJsonValid ✅ — valid/malformed projection.

The empty-string and NULL behaviors were also confirmed live on the rebuilt cluster (where not json_valid returns the NULL and malformed rows; empty-string excluded as valid), proving the Calcite → Substrait → DataFusion chain preserves the legacy Jackson contract.

Check List

• New functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cf0a4de)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review NULL Handling Mismatch The code comment and description claim NULL input returns FALSE to match legacy behavior, but the function signature declares ReturnTypes.BOOLEAN_NULLABLE (line 203 in JsonFunctionAdapters.java), indicating NULL can be returned. The Rust implementation at lines 88-94 and 104-107 explicitly converts NULL to FALSE, which contradicts the nullable return type declaration. If the intent is truly to return FALSE for NULL (non-null-propagating), the return type should be ReturnTypes.BOOLEAN (non-nullable). This inconsistency may cause type-checking issues or unexpected behavior in query planning. fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { if arg_types.len() != 1 { return plan_err!("json_valid expects 1 argument, got {}", arg_types.len()); } Ok(DataType::Boolean)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to cf0a4de

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix NULL handling documentation mismatch The description states "NULL on NULL input" but the implementation returns FALSE for NULL inputs (see json_valid.rs line 90: unwrap_or(false)). This documentation mismatch could mislead users about the function's actual null-handling behavior, especially for filter predicates like where not json_valid(col). sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml [836-840] - name: "json_valid" - description: "TRUE if the input parses as JSON, FALSE on malformed input, NULL on NULL input (parity with legacy JsonUtils.isValidJson / Jackson readTree — empty/whitespace input is valid)." + description: "TRUE if the input parses as JSON, FALSE on malformed input or NULL input (parity with legacy JsonUtils.isValidJson / Jackson readTree — empty/whitespace input is valid, NULL returns FALSE not NULL)." impls: - args: [{ value: string, name: "value" }] return: boolean Suggestion importance[1-10]: 8 __ Why: The description incorrectly states "NULL on NULL input" when the implementation actually returns FALSE for NULL inputs (as documented in json_valid.rs lines 16-17 and implemented at line 91). This is a critical documentation bug that could mislead users about the function's behavior, especially for filter predicates like where not json_valid(col).	Medium
	Clarify empty string vs NULL handling The empty/whitespace special-case logic contradicts the documented NULL-handling contract. The function doc states "NULL / missing input → FALSE" and "NULL input returns NULL" (from yaml), but is_valid_json("") returns TRUE. This creates inconsistent behavior where empty strings are treated as valid JSON while NULL produces FALSE, potentially confusing users expecting uniform null-like handling. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_valid.rs [119-125] fn is_valid_json(s: &str) -> bool { // Jackson readTree("") / readTree(" ") → MissingNode (valid); serde would reject. Match legacy. + // Note: Empty/whitespace strings return TRUE (valid JSON per Jackson), distinct from + // NULL input which returns FALSE (handled by caller before reaching this function). if s.trim().is_empty() { return true; } serde_json::from_str::<Value>(s).is_ok() } Suggestion importance[1-10]: 3 __ Why: The suggestion asks to add a clarifying comment about empty string vs NULL handling. While the distinction is already documented in the function's doc comment (lines 16-20), adding an inline note could improve readability. However, this is a minor documentation enhancement that doesn't fix any bug or significantly improve code quality.	Low

---

Previous suggestions

Suggestions up to commit 01134b0

Category	Suggestion	Impact
General	Avoid unnecessary string allocation The trim() operation allocates a new string when whitespace is present, which is unnecessary for validation. Use trim_start() and trim_end() with length comparison instead, or check chars().all(|c| c.is_whitespace()) to avoid allocation overhead in the hot path. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_valid.rs [119-125] fn is_valid_json(s: &str) -> bool { // Jackson readTree("") / readTree(" ") → MissingNode (valid); serde would reject. Match legacy. - if s.trim().is_empty() { + if s.chars().all(|c| c.is_whitespace()) { return true; } serde_json::from_str::<Value>(s).is_ok() } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a potential optimization by avoiding trim() allocation. However, chars().all(|c| c.is_whitespace()) may be less efficient for long strings with early non-whitespace characters compared to trim().is_empty(). The performance impact is minor in typical use cases.	Low
	Extract validation result before appending The loop iterates through indices and calls strings.cell(i) for each row. Consider using an iterator pattern like strings.iter() if available, which would be more idiomatic and potentially more efficient by avoiding repeated index bounds checks. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_valid.rs [102-107] let mut builder = BooleanBuilder::with_capacity(n); for i in 0..n { // NULL row → FALSE (legacy JsonUtils.isValidJson returns FALSE for null/missing), // so `where not json_valid(col)` includes NULL rows as the IT expects. - builder.append_value(strings.cell(i).map(is_valid_json).unwrap_or(false)); + let is_valid = strings.cell(i).map(is_valid_json).unwrap_or(false); + builder.append_value(is_valid); } Suggestion importance[1-10]: 2 __ Why: The improved_code is functionally identical to the existing_code - it merely extracts the result into a variable before appending, which provides no meaningful benefit. The suggestion about using an iterator pattern is not reflected in the improved code, making this a minimal-impact style change.	Low

Suggestions up to commit d834702

Category	Suggestion	Impact
General	Document legacy compatibility deviation The empty/whitespace-as-valid behavior deviates from RFC 8259 and most modern JSON validators. Consider documenting this as a legacy compatibility quirk and evaluate whether future versions should align with standard JSON validation semantics where empty strings are invalid. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_valid.rs [116-122] fn is_valid_json(s: &str) -> bool { - // Jackson readTree("") / readTree(" ") → MissingNode (valid); serde would reject. Match legacy. + // LEGACY COMPATIBILITY: Jackson readTree("") / readTree(" ") → MissingNode (valid). + // This deviates from RFC 8259 where empty input is invalid JSON. + // TODO: Consider aligning with standard JSON validation in future versions. if s.trim().is_empty() { return true; } serde_json::from_str::<Value>(s).is_ok() } Suggestion importance[1-10]: 4 __ Why: The suggestion adds a TODO comment about future alignment with standard JSON validation. While the existing comment already explains the Jackson parity rationale, the TODO provides marginal value by flagging this as a potential future improvement. However, the current documentation is already comprehensive, and the behavior is intentional for backward compatibility, making this a minor enhancement.	Low

[github-actions]

Persistent review updated to latest commit 01134b0

[github-actions]

Persistent review updated to latest commit cf0a4de

[github-actions]

✅ Gradle check result for cf0a4de: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (dd57991) to head (cf0a4de).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22139      +/-   ##
============================================
- Coverage     73.37%   73.31%   -0.06%     
+ Complexity    75836    75816      -20     
============================================
  Files          6064     6064              
  Lines        344498   344498              
  Branches      49575    49575              
============================================
- Hits         252773   252585     -188     
- Misses        71561    71762     +201     
+ Partials      20164    20151      -13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.