Wire PPL string scalar functions with analytics-backend-datafusion by tanik98 · Pull Request #21543 · opensearch-project/OpenSearch

tanik98 · 2026-05-07T17:56:49Z

Description

Wires PPL string scalar functions through the analytics-backend-datafusion plugin, enabling native DataFusion execution for string operations.

Functions added

PPL Function	Strategy	Return Type
`ascii(str)`	Direct name match → DataFusion built-in	INTEGER
`concat(str1, ..., strN)`	Direct name match → DataFusion built-in	KEYWORD
`concat_ws(sep, str1, str2)`	Direct name match → DataFusion built-in	KEYWORD
`left(str, n)`	Direct name match → DataFusion built-in	KEYWORD
`length(str)`	Name alias → `char_length`	INTEGER
`locate(substr, str)`	Adapter (PositionAdapter)	INTEGER
`lower(str)`	Direct name match → DataFusion built-in	KEYWORD
`ltrim(str)`	Direct name match → DataFusion built-in	KEYWORD
`position(substr, str)`	PositionAdapter	INTEGER
`reverse(str)`	Direct name match → DataFusion built-in	KEYWORD
`right(str, n)`	Direct name match → DataFusion built-in	KEYWORD
`rtrim(str)`	Direct name match → DataFusion built-in	KEYWORD
`strcmp(str1, str2)`	Adapter (StrcmpFunctionAdapter)	INTEGER
`substr(str, start[, len])`	Name alias → `substring`	KEYWORD
`tostring(value[, format])`	Adapter (ToStringFunctionAdapter) + Rust UDF	KEYWORD
`tonumber(string[, base])`	Adapter (ToNumberFunctionAdapter) + Rust UDF	DOUBLE
`trim(str)`	Direct name match → DataFusion built-in (`btrim`)	KEYWORD
`upper(str)`	Direct name match → DataFusion built-in	KEYWORD

Key changes

ToStringFunctionAdapter — Routes 1-arg calls through CAST(x AS VARCHAR), boolean values through an explicit CASE WHEN ... THEN 'TRUE' ... THEN 'FALSE' END (DataFusion's CAST produces lowercase), and 2-arg format calls (hex/binary/commas/duration/duration_millis) through the tostring Rust UDF.
ToNumberFunctionAdapter — 1-arg calls use SAFE_CAST to DOUBLE (substrait's null-on-failure cast, which DataFusion maps to try_cast) so unparseable values like tonumber('abc') return NULL instead of hard-erroring in the simplify_expressions optimizer. Handles both fractional ('4598.678') and integer input. 2-arg calls route through the tonumber Rust UDF for base-N integer parsing (i64::from_str_radix); the UDF returns NULL on parse failure.
StrcmpFunctionAdapter — The PPL frontend reverses strcmp arguments; the adapter swaps them back. The comparison itself is decomposed into a SIMD-vectorized CASE WHEN lhs IS NULL OR rhs IS NULL THEN NULL WHEN lhs < rhs THEN -1 WHEN lhs = rhs THEN 0 ELSE 1 END so DataFusion evaluates it through its native arrow-rs compare kernels — no Rust UDF needed.
PositionAdapter — PPL's POSITION(substr IN str) and locate(substr, str[, start]) has reversed argument order vs DataFusion's strpos(str, substr) and supports an optional start position that strpos doesn't. The adapter swaps arguments for the 2-arg form; for the 3-arg form it decomposes to substring(str, start) + strpos(substring(str, start), substr) - 1, preserving 1-based semantics.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-05-07T17:59:01Z

PR Reviewer Guide 🔍

(Review updated until commit `a07f9b5`)

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 Duplicate Entry Line 117 duplicates the `SqlLibraryOperators.REGEXP_CONTAINS` mapping that already appears at line 114. This creates two identical entries in the `ADDITIONAL_SCALAR_SIGS` list, which is redundant and may cause confusion or unexpected behavior if the list is later processed assuming unique entries. FunctionMappings.s(SqlLibraryOperators.REGEXP_CONTAINS, "regex_match"), Possible Panic The `from_str_radix` call at line 173 can panic if the radix is outside the valid range [2, 36], even though `validate_base` is supposed to guard this. However, if `BaseMode::Valid(radix)` is constructed with an invalid radix due to a logic error or future code change, the panic will occur. Consider adding a debug assertion or comment clarifying the invariant that `radix` is always valid when `BaseMode::Valid` is constructed. match values.at(i).and_then(\|s\| i64::from_str_radix(s, radix).ok()) { Possible Panic Line 213 uses `.expect()` on a `split_once` result, assuming the `{:.2}` format always produces a decimal point. While this is true for finite numbers, if `v.is_finite()` check at line 307 fails to catch an edge case (e.g., due to a future Rust version change or unexpected input), the panic will occur. Consider using `unwrap_or` or explicitly handling the case where no decimal point is found. /// Format modes, case-sensitive to match the SQL plugin's Java reference /// (`ToStringFunction.DURATION_FORMAT`, etc.).

github-actions · 2026-05-07T17:59:52Z

PR Code Suggestions ✨

Latest suggestions up to a07f9b5

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent undefined behavior in cast Casting non-finite or out-of-range `f64` values to `i64` is undefined behavior in Rust. The `is_finite` check prevents NaN/Infinity, but values outside `i64::MIN..=i64::MAX` still cause UB. Use `value.clamp(i64::MIN as f64, i64::MAX as f64)` before casting. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tostring.rs [259-264] fn truncate_to_i64(value: f64) -> i64 { if !value.is_finite() { return 0; } - value as i64 + value.clamp(i64::MIN as f64, i64::MAX as f64) as i64 } Suggestion importance[1-10]: 9 __ Why: Casting `f64` values outside `i64::MIN..=i64::MAX` to `i64` causes undefined behavior in Rust. The `is_finite` check prevents NaN/Infinity but doesn't handle out-of-range values. Using `clamp` before casting is critical for correctness and safety.	High
General	Remove duplicate REGEXP_CONTAINS mapping The `REGEXP_CONTAINS` operator is mapped twice to `"regex_match"` in consecutive lines. Remove the duplicate entry to avoid redundant configuration and potential confusion during maintenance. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [117] FunctionMappings.s(SqlLibraryOperators.REGEXP_CONTAINS, "regex_match"), FunctionMappings.s(SqlStdOperatorTable.REPLACE, "replace"), FunctionMappings.s(SqlLibraryOperators.REGEXP_REPLACE_3, "regexp_replace"), -FunctionMappings.s(SqlLibraryOperators.REGEXP_CONTAINS, "regex_match"), Suggestion importance[1-10]: 7 __ Why: The duplicate mapping of `REGEXP_CONTAINS` to `"regex_match"` at line 117 is redundant since the same mapping already exists at line 114. This creates unnecessary configuration duplication and could cause maintenance confusion.	Medium
	Document start parameter assumption The `adjusted` calculation evaluates `posInTail + start - 1` without verifying that `start` is non-negative. If `start` is negative or zero, the result may be incorrect. Add validation or document the assumption that `start` is always positive. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/PositionAdapter.java [92-99] RexNode zero = rexBuilder.makeExactLiteral(BigDecimal.ZERO, intType); RexNode one = rexBuilder.makeExactLiteral(BigDecimal.ONE, intType); RexNode isZero = rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, posInTail, zero); +// Assumes start >= 1 per PPL semantics; negative/zero start handled upstream RexNode adjusted = rexBuilder.makeCall( SqlStdOperatorTable.MINUS, rexBuilder.makeCall(SqlStdOperatorTable.PLUS, posInTail, start), one ); Suggestion importance[1-10]: 5 __ Why: Adding a comment to document the assumption that `start >= 1` improves code clarity. However, the suggestion doesn't address actual validation logic, and PPL semantics likely ensure valid `start` values upstream, making this a minor documentation improvement.	Low
	Document precision loss in cast Casting `i64` to `f64` can lose precision for values beyond 2^53. Consider documenting this limitation or adding a check to warn when precision loss occurs for large integers. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [173-176] for i in 0..n { match values.at(i).and_then(\|s\| i64::from_str_radix(s, radix).ok()) { - Some(v) => builder.append_value(v as f64), + Some(v) => { + // Note: i64 → f64 cast may lose precision for \|v\| > 2^53 + builder.append_value(v as f64) + } None => builder.append_null(), } } Suggestion importance[1-10]: 4 __ Why: The comment about precision loss when casting `i64` to `f64` is informative but addresses a known limitation of the data type choice (return type is `Float64` per spec). This is a minor documentation enhancement rather than a functional issue.	Low

Previous suggestions

Suggestions up to commit 3a69307

Category	Suggestion	Impact
Possible issue	Handle leading `+` sign in base-N parsing The `i64::from_str_radix` function does not accept a leading `+` sign, so inputs like `"+FA"` will silently return NULL instead of parsing correctly. The test `signed_inputs_parse` asserts `invoke_scalar(Some("+FA"), Some(16))` returns `Some(250.0)`, but this will fail because `i64::from_str_radix` rejects the `+` prefix. Strip a leading `+` before parsing to match the documented semantics. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [173-176] -match values.at(i).and_then(\|s\| i64::from_str_radix(s, radix).ok()) { +match values.at(i).and_then(\|s\| { + let s = s.strip_prefix('+').unwrap_or(s); + i64::from_str_radix(s, radix).ok() +}) { Some(v) => builder.append_value(v as f64), None => builder.append_null(), } Suggestion importance[1-10]: 8 __ Why: The test `signed_inputs_parse` asserts that `"+FA"` with base 16 returns `Some(250.0)`, but `i64::from_str_radix` does not accept a leading `+` sign, so this test would fail. The fix correctly strips the `+` prefix before parsing, aligning the array path with the documented semantics.	Medium
	Handle leading `+` sign in scalar fast path The `parse_with_base` scalar fast path also uses `i64::from_str_radix` internally and will fail to parse strings with a leading `+` sign (e.g., `"+FA"`). The same fix of stripping a leading `+` before calling `from_str_radix` should be applied inside `parse_with_base` for consistency with the array path. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [135-137] -return Ok(ColumnarValue::Scalar(ScalarValue::Float64( - parse_with_base(s.as_deref(), *b), -))); +fn parse_with_base(s: Option<&str>, base: Option<i32>) -> Option<f64> { + let s = s?; + let radix = validate_base(base?)?; + let s = s.strip_prefix('+').unwrap_or(s); + i64::from_str_radix(s, radix).ok().map(\|v\| v as f64) +} Suggestion importance[1-10]: 8 __ Why: The scalar fast path via `parse_with_base` also uses `i64::from_str_radix` and would fail to parse `"+FA"` for the same reason. The improved `parse_with_base` function correctly strips the leading `+` before parsing, ensuring consistency between the scalar and array code paths.	Medium
	Guard first-match semantics for OpenSearch fixed variants When an OpenSearch fixed-arity variant is found, it is stored but the loop only breaks after the `if/else` block. However, if a second OpenSearch fixed-arity variant is encountered later, it silently overwrites the first one. The assignment `openSearchFixed = variant` should be guarded with a null check (like the other slots) to preserve first-match semantics consistently. sandbox/plugins/analytics-backend-datafusion/src/main/java/io/substrait/isthmus/expression/NameBasedScalarFunctionConverter.java [165-171] if (isOpenSearch) { - if (!isVariadic) { + if (!isVariadic && openSearchFixed == null) { openSearchFixed = variant; - } else if (openSearchVariadic == null) { + } else if (isVariadic && openSearchVariadic == null) { openSearchVariadic = variant; } } else { Suggestion importance[1-10]: 7 __ Why: The current code overwrites `openSearchFixed` if multiple OpenSearch fixed-arity variants match, breaking first-match semantics. The fix adds a null check consistent with how `openSearchVariadic`, `upstreamFixed`, and `upstreamVariadic` are guarded. The `improved_code` also correctly fixes the `else if` condition for `openSearchVariadic`.	Medium
General	Avoid scientific notation in default float formatting The comment says this matches Java's `NumberFormat` with no decimals printing `"42"`, but `format!("{value}")` for `f64` uses Rust's default float formatting which may produce scientific notation (e.g., `"1e10"`) for large or small values, diverging from the Java reference. Use `format!("{:.}", value)` or an explicit decimal-aware formatter to avoid scientific notation. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tostring.rs [253-254] -let rendered = format!("{value}"); +// Use explicit decimal notation to avoid scientific notation for large/small values. +let rendered = format!("{}", value); +if rendered.contains('e') \|\| rendered.contains('E') { + return format!("{:.0}", value); +} rendered.strip_suffix(".0").map_or(rendered.clone(), \|s\| s.to_string()) Suggestion importance[1-10]: 4 __ Why: The concern about scientific notation is valid in theory, but Rust's `{}` formatter for `f64` does use scientific notation for very large/small values. However, the `improved_code` proposed is incomplete and potentially incorrect (e.g., `{:.0}` would drop fractional digits). The suggestion identifies a real edge case but the fix is not well-formed.	Low
General	Fix strcmp test to handle all comparison outcomes The test assumes all non-"Amber" firstnames ("Hattie", "Nanette") are lexicographically greater than "Amber" and thus always return `1`. However, `strcmp` returns `-1` when `lhs < rhs`, so if any fixture name were alphabetically before "Amber" the test would fail silently. The expected value should account for both `< 0` and `> 0` cases to make the test robust and correct. sandbox/plugins/analytics-backend-datafusion/src/internalClusterTest/java/org/opensearch/be/datafusion/StringScalarIT.java [221-230] public void testStrcmpColumn() { PPLResponse r = run("source = " + INDEX + " \| eval out = strcmp(firstname, 'Amber') \| fields firstname, out"); assertEquals(3, r.getRows().size()); for (Object[] row : r.getRows()) { String firstname = (String) row[0]; int out = ((Number) row[1]).intValue(); - int expected = "Amber".equals(firstname) ? 0 : 1; + int cmp = firstname.compareTo("Amber"); + int expected = cmp == 0 ? 0 : (cmp < 0 ? -1 : 1); assertEquals("strcmp('" + firstname + "', 'Amber')", expected, out); } } Suggestion importance[1-10]: 3 __ Why: The suggestion is technically correct that the test hardcodes `1` for all non-"Amber" names, but the fixture data ("Hattie", "Nanette") are both lexicographically after "Amber", so the test is actually correct for the given data. The improvement adds robustness but is not fixing a real bug in the current fixture.	Low

Suggestions up to commit f0c0566

Category	Suggestion	Impact
Possible issue	Handle leading `+` sign in base-N parsing The `i64::from_str_radix` function does not handle leading `+` signs (e.g., `"+FA"`), yet the test `signed_inputs_parse` asserts `invoke_scalar(Some("+FA"), Some(16))` returns `Some(250.0)`. This will silently return NULL instead of the expected value because `i64::from_str_radix` rejects `+` prefixes. Strip a leading `+` before parsing to match the documented behavior. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [173-176] -match values.at(i).and_then(\|s\| i64::from_str_radix(s, radix).ok()) { +match values.at(i).and_then(\|s\| { + let s = s.strip_prefix('+').unwrap_or(s); + i64::from_str_radix(s, radix).ok() +}) { Some(v) => builder.append_value(v as f64), None => builder.append_null(), } Suggestion importance[1-10]: 8 __ Why: The test `signed_inputs_parse` asserts `invoke_scalar(Some("+FA"), Some(16))` returns `Some(250.0)`, but `i64::from_str_radix` rejects leading `+` signs, so the array path would silently return NULL. This is a real correctness bug that would cause test failures.	Medium
	Fix scalar fast-path to handle leading `+` sign The `parse_with_base` scalar fast-path calls `i64::from_str_radix` directly (via `validate_base`), which also does not handle a leading `+` sign. The test `signed_inputs_parse` exercises this path with `"+FA"` and expects `Some(250.0)`, so the fast path will return `None` (NULL) instead. Apply the same `+`-stripping fix here for consistency. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [135-138] return Ok(ColumnarValue::Scalar(ScalarValue::Float64( - parse_with_base(s.as_deref(), b), + parse_with_base(s.as_deref().map(\|s\| s.strip_prefix('+').unwrap_or(s)), b), ))); Suggestion importance[1-10]: 8 __ Why: The scalar fast-path also uses `i64::from_str_radix` via `parse_with_base`, which rejects `+` prefixes. The test `signed_inputs_parse` exercises this path and expects `Some(250.0)` for `"+FA"`, so this is a real correctness bug affecting the scalar code path as well.	Medium
	Fix incorrect alias mapping replace to regexp_replace The alias maps `"replace"` → `"regexp_replace"`, but PPL's `replace` is a plain string replacement (not regex-based). This will cause `replace(str, 'a', 'b')` to be dispatched as a regex function, producing incorrect results or errors when the search string contains regex metacharacters. The alias for `"replace"` should be removed or corrected to map to the actual plain-replace UDF name. sandbox/plugins/analytics-backend-datafusion/src/main/java/io/substrait/isthmus/expression/NameBasedScalarFunctionConverter.java [57-68] private static final Map<String, String> NAME_ALIASES = Map.of( "length", "char_length", "locate", "strpos", "position", "strpos", "substr", - "substring", - "replace", - "regexp_replace" + "substring" ); Suggestion importance[1-10]: 7 __ Why: The alias `"replace"` → `"regexp_replace"` is semantically incorrect — PPL's `replace` is a plain string substitution, not regex-based. This could cause incorrect behavior when the search string contains regex metacharacters. The fix is valid and directly addresses a functional bug in the alias map.	Medium
General	Handle scientific notation in float fallback rendering The fallback rendering for `f64` strips a trailing `.0`, but `format!("{value}")` for values like `1e20` produces scientific notation (e.g. `"100000000000000000000"`), and for `NaN`/`Infinity` the `is_finite()` guard above already returns early. However, for very large or very small floats, Rust's `Display` uses scientific notation (e.g. `1e-5`), which may not match the expected decimal output. Consider using a fixed-point formatter or explicitly handling these edge cases. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tostring.rs [253-254] let rendered = format!("{value}"); +if rendered.contains('e') \|\| rendered.contains('E') { + // Scientific notation: render as full decimal to match Java's BigDecimal behavior + return format!("{:.}", value); +} rendered.strip_suffix(".0").map_or(rendered.clone(), \|s\| s.to_string()) Suggestion importance[1-10]: 4 __ Why: The concern about scientific notation in Rust's `Display` for `f64` is valid for edge cases with very large or very small values, but the `improved_code` uses `format!("{:.}", value)` which is not valid Rust syntax. The suggestion identifies a real edge case but the proposed fix is incorrect.	Low
	Fix test to correctly compute expected strcmp sign The test assumes all non-"Amber" firstnames are lexicographically greater than "Amber" (expected = 1), but "Hattie" > "Amber" and "Nanette" > "Amber" happen to be true for this fixture. However, the test would silently pass even if the adapter swapped arguments incorrectly for the equal case. More importantly, the test does not verify the case where `firstname < 'Amber'` (expected = -1), leaving the negative result path untested. Consider adding a fixture row with a name lexicographically less than "Amber" or asserting the sign explicitly. sandbox/plugins/analytics-backend-datafusion/src/internalClusterTest/java/org/opensearch/be/datafusion/StringScalarIT.java [235-244] public void testStrcmpColumn() { PPLResponse r = run("source = " + INDEX + " \| eval out = strcmp(firstname, 'Amber') \| fields firstname, out"); assertEquals(3, r.getRows().size()); for (Object[] row : r.getRows()) { String firstname = (String) row[0]; int out = ((Number) row[1]).intValue(); - int expected = "Amber".equals(firstname) ? 0 : 1; + int cmp = firstname.compareTo("Amber"); + int expected = cmp < 0 ? -1 : (cmp == 0 ? 0 : 1); assertEquals("strcmp('" + firstname + "', 'Amber')", expected, out); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the test hardcodes `expected = 1` for all non-"Amber" names without verifying the negative case. The improved code uses `compareTo` to derive the expected sign dynamically, which is more robust. However, with the current fixture data ("Hattie" and "Nanette" are both > "Amber"), the test still doesn't cover the `-1` case regardless of this fix.	Low
	Fix nullable VARCHAR type derivation for boolean branch The nullability check for the BOOLEAN branch uses `original.getType().isNullable()` to set the VARCHAR type, but `original` is the `tostring` call whose return type may not correctly reflect the nullability of the boolean input. If `value` is nullable, the CASE expression should produce a nullable VARCHAR. The nullability should be derived from `value.getType().isNullable()` instead. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ToStringFunctionAdapter.java [121-123] if (value.getType().getSqlTypeName() == SqlTypeName.BOOLEAN) { - return makeBooleanToString(original, value, cluster); + return makeBooleanToString(value, cluster); } Suggestion importance[1-10]: 3 __ Why: The suggestion points to a potential nullability mismatch, but the `improved_code` changes the method signature of `makeBooleanToString` without showing the updated method body, making it incomplete and potentially misleading. The actual impact is minor since `original.getType().isNullable()` is typically consistent with the input nullability in practice.	Low
	Clarify lifetime of materialized array borrows `string_source` borrows from `str1_arr` (an `Option`) but `str1_arr` is a local that is dropped at the end of the block. The `StringSource::Array` variant holds a `&'a` `StringArray` tied to the lifetime of `materialized: Option<&'a ArrayRef>`, so the borrow of the temporary `Option` must outlive the loop. Passing `str1_arr.as_ref()` (a reference to the local) is fine as long as both locals are kept alive for the duration of the loop, which they are here — but the lifetime annotation `'a` on `string_source` ties the output lifetime to the `materialized` argument, not to the local. Ensure the materialized arrays are bound before calling `string_source` so the compiler can verify the lifetimes correctly, or restructure to make the dependency explicit. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/strcmp.rs [126-129] let str1_arr = materialize(&args.args[0], n, "str1")?; let str2_arr = materialize(&args.args[1], n, "str2")?; -let str1 = string_source(&args.args[0], str1_arr.as_ref())?; -let str2 = string_source(&args.args[1], str2_arr.as_ref())?; +// Keep refs in named bindings so the borrow checker can track lifetimes. +let str1_ref = str1_arr.as_ref(); +let str2_ref = str2_arr.as_ref(); +let str1 = string_source(&args.args[0], str1_ref)?; +let str2 = string_source(&args.args[1], str2_ref)?; Suggestion importance[1-10]: 2 __ Why: The existing code already correctly keeps `str1_arr` and `str2_arr` alive for the duration of the loop, so the Rust borrow checker already verifies this. The suggestion adds intermediate bindings but provides no functional or safety improvement — the code compiles and works correctly as-is.	Low

Suggestions up to commit 88ca200

Category	Suggestion	Impact
Possible issue	Handle leading '+' sign in base-N parsing The `i64::from_str_radix` function does not accept a leading `+` sign, so inputs like `"+FA"` will silently return NULL instead of parsing correctly. The test `signed_inputs_parse` asserts `invoke_scalar(Some("+FA"), Some(16))` returns `Some(250.0)`, but this will fail at runtime. You should strip a leading `+` before calling `from_str_radix`. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [173-176] -match values.at(i).and_then(\|s\| i64::from_str_radix(s, radix).ok()) { +match values.at(i).and_then(\|s\| { + let s = s.strip_prefix('+').unwrap_or(s); + i64::from_str_radix(s, radix).ok() +}) { Some(v) => builder.append_value(v as f64), None => builder.append_null(), } Suggestion importance[1-10]: 8 __ Why: `i64::from_str_radix` does not accept a leading `+` sign, so the test `signed_inputs_parse` asserting `invoke_scalar(Some("+FA"), Some(16))` returns `Some(250.0)` would fail. This is a real correctness bug in the array path.	Medium
	Handle leading '+' sign in scalar fast path The scalar fast-path `parse_with_base` also calls `i64::from_str_radix` directly without stripping a leading `+`, causing the same issue as in the array path. The test `signed_inputs_parse` for `"+FA"` with base 16 will fail here too. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tonumber.rs [213-217] fn parse_with_base(s: Option<&str>, base: Option<i32>) -> Option<f64> { let s = s?; let radix = validate_base(base?)?; + let s = s.strip_prefix('+').unwrap_or(s); i64::from_str_radix(s, radix).ok().map(\|v\| v as f64) } Suggestion importance[1-10]: 8 __ Why: Same bug as in the array path — `parse_with_base` also calls `i64::from_str_radix` without stripping a leading `+`, causing the scalar fast-path test for `"+FA"` with base 16 to fail.	Medium
	Remove incorrect alias mapping replace to regexp_replace The alias `"replace" → "regexp_replace"` is incorrect. PPL's `replace(str, from, to)` is a plain string replacement, not a regex operation. Mapping it to `regexp_replace` will cause literal strings to be interpreted as regex patterns, producing wrong results or errors for inputs containing regex metacharacters (e.g., `replace(str, '.', 'x')` would replace every character). This alias should be removed or mapped to the correct DataFusion built-in (e.g., `translate` or a dedicated plain-replace UDF). sandbox/plugins/analytics-backend-datafusion/src/main/java/io/substrait/isthmus/expression/NameBasedScalarFunctionConverter.java [57-68] private static final Map<String, String> NAME_ALIASES = Map.of( "length", "char_length", "locate", "strpos", "position", "strpos", "substr", - "substring", - "replace", - "regexp_replace" + "substring" ); Suggestion importance[1-10]: 7 __ Why: The alias `"replace" → "regexp_replace"` is semantically incorrect — PPL's `replace` is a plain string substitution, not a regex operation. Mapping it to `regexp_replace` would cause metacharacters in the search string to be interpreted as regex patterns, producing wrong results. This is a real correctness issue, though the `RegexpReplaceFunctionAdapter` is separately registered for `ScalarFunction.REGEXP_REPLACE`, so the actual runtime behavior depends on how the routing works end-to-end.	Medium
General	Fix strcmp test to handle less-than case correctly The test assumes that all non-"Amber" firstnames (`"Hattie"` and `"Nanette"`) are lexicographically greater than `"Amber"`, so `expected` is always `1` for them. However, `strcmp` should return `-1` when `lhs < rhs`. If any fixture name were alphabetically before `"Amber"`, this test would incorrectly expect `1` instead of `-1`. The expected value should be computed using `String.compareTo` to correctly handle both less-than and greater-than cases. sandbox/plugins/analytics-backend-datafusion/src/internalClusterTest/java/org/opensearch/be/datafusion/StringScalarIT.java [235-244] public void testStrcmpColumn() { PPLResponse r = run("source = " + INDEX + " \| eval out = strcmp(firstname, 'Amber') \| fields firstname, out"); assertEquals(3, r.getRows().size()); for (Object[] row : r.getRows()) { String firstname = (String) row[0]; int out = ((Number) row[1]).intValue(); - int expected = "Amber".equals(firstname) ? 0 : 1; + int cmp = firstname.compareTo("Amber"); + int expected = cmp < 0 ? -1 : cmp > 0 ? 1 : 0; assertEquals("strcmp('" + firstname + "', 'Amber')", expected, out); } } Suggestion importance[1-10]: 5 __ Why: The test hardcodes `expected = 1` for all non-"Amber" names, which only works because the fixture names "Hattie" and "Nanette" happen to be lexicographically greater than "Amber". Using `String.compareTo` makes the test robust against future fixture changes and correctly documents the expected behavior for all comparison outcomes.	Low
	Guard against unmaterialized array in string source `string_source` borrows from `str1_arr` (an `Option`), but `str1_arr` is a local variable that holds the `ArrayRef`. The lifetime of the `StringSource::Array` reference is tied to `str1_arr`, which is dropped at the end of the function scope — this is fine in Rust, but the `as_ref()` call passes `Option<&ArrayRef>` while `string_source` expects `Option<&'a` `ArrayRef>` tied to the same lifetime as `cv`. If `str1_arr` is `None` and `cv` is an `Array` variant (which shouldn't happen given `materialize`'s logic), the fallback in `string_source` would incorrectly try to treat the array `ColumnarValue` as a scalar. Consider asserting or returning an error if `materialized` is `None` but `cv` is `ColumnarValue::Array`. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/strcmp.rs [126-129] -let str1_arr = materialize(&args.args[0], n, "str1")?; -let str2_arr = materialize(&args.args[1], n, "str2")?; -let str1 = string_source(&args.args[0], str1_arr.as_ref())?; -let str2 = string_source(&args.args[1], str2_arr.as_ref())?; +fn string_source<'a>( + cv: &'a ColumnarValue, + materialized: Option<&'a ArrayRef>, +) -> Result<StringSource<'a>> { + if let Some(arr) = materialized { + let sa = arr.as_string::<i32>(); + return Ok(StringSource::Array(sa)); + } + match cv { + ColumnarValue::Scalar(ScalarValue::Utf8(opt)) => Ok(StringSource::Scalar(opt.as_deref())), + ColumnarValue::Array(_) => exec_err!("strcmp: internal error: array not materialized"), + other => exec_err!("strcmp: expected Utf8 scalar, got {other:?}"), + } +} Suggestion importance[1-10]: 4 __ Why: The suggestion adds a defensive check in `string_source` for the case where `materialized` is `None` but `cv` is `ColumnarValue::Array`, which is a logical inconsistency. However, `materialize` already ensures this invariant holds, so this is a minor defensive improvement rather than a critical bug fix.	Low
	Simplify fractional zero trimming to handle all trailing zeros The fractional trimming logic only removes one trailing zero (e.g., `.50 → .5`), but since `{:.2}` always produces exactly 2 decimal digits, `frac` can only be `"00"`, `"X0"`, or `"XY"`. The case `frac == "10"` would correctly trim to `".1"`, but `frac == "00"` is already handled. This logic is correct for 2 decimal places, but the comment "Trim a trailing '0' when exactly one digit would be meaningful" is misleading — it always trims exactly one trailing zero from a 2-char string. This is fine functionally, but worth clarifying to avoid future bugs if precision changes. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/tostring.rs [321-329] -fn format_commas_f64(v: f64) -> String { - // Non-finite fall through to native rendering (matches Double.toString for Infinity/NaN). - if !v.is_finite() { - return v.to_string(); - } - let is_negative = v.is_sign_negative(); - // rounds the number to the nearest two decimal places. - let rounded = format!("{:.2}", v.abs()); - let (whole, frac) = rounded - .split_once('.') - .expect("{:.2} always produces a decimal point"); - ... - if frac != "00" { - out.push('.'); - // Trim a trailing '0' when exactly one digit would be meaningful, e.g. `.50 → .5`. - if frac.ends_with('0') { - out.push_str(&frac[..frac.len() - 1]); - } else { - out.push_str(frac); - } - } - out +if frac != "00" { + out.push('.'); + // frac is always 2 chars from {:.2}; strip one trailing '0' so ".50" → ".5". + let trimmed = frac.trim_end_matches('0'); + out.push_str(trimmed); } Suggestion importance[1-10]: 3 __ Why: The suggestion to use `trim_end_matches('0')` is a minor readability improvement. Since `{:.2}` always produces exactly 2 decimal digits, the current logic is functionally correct, but `trim_end_matches` is more idiomatic and self-documenting. The impact is low as there's no functional bug.	Low
	Handle nullable boolean three-valued logic in CASE rewrite The boolean check uses `==` for `SqlTypeName` enum comparison, which is correct, but the nullability of the boolean type is not considered. If `value` is a nullable boolean column, the generated `CASE WHEN value THEN 'TRUE' WHEN NOT value THEN 'FALSE' END` will return `NULL` for null inputs via the ELSE branch, which is the correct behavior. However, `NOT value` on a nullable boolean may not behave as expected in all SQL dialects — consider using `IS NOT TRUE` or an explicit null check to ensure correctness for three-valued logic. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/ToStringFunctionAdapter.java [121-123] +if (value.getType().getSqlTypeName() == SqlTypeName.BOOLEAN) { + return makeBooleanToString(original, value, cluster); +} - Suggestion importance[1-10]: 2 __ Why: The `existing_code` and `improved_code` are identical, so no actual change is proposed. The concern about nullable boolean three-valued logic is valid in theory, but the ELSE branch already handles the null case correctly, and the suggestion doesn't provide a concrete fix.	Low

github-actions · 2026-05-07T18:22:07Z

❌ Gradle check result for 88ca200: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-07T18:58:38Z

Persistent review updated to latest commit f0c0566

github-actions · 2026-05-07T19:54:19Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7abb1d1.

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/src/main/java/io/substrait/isthmus/expression/NameBasedScalarFunctionConverter.java	76	medium	NAME_ALIASES maps the PPL `replace` function to `regexp_replace` at the Substrait layer. A user calling replace(str, literal_from, literal_to) expecting literal-string substitution will silently get regex-pattern semantics instead. If from_str is user-controlled, special regex metacharacters (e.g., `.`, `*`, `(`) are interpreted as patterns rather than literals, creating an unexpected semantic gap that could be exploited for regex injection or unintended data manipulation. This may be intentional for PPL's semantics but warrants explicit verification.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-05-07T20:18:00Z

Persistent review updated to latest commit 3a69307

github-actions · 2026-05-07T21:16:55Z

✅ Gradle check result for 3a69307: SUCCESS

codecov · 2026-05-07T21:17:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.53%. Comparing base (d09b185) to head (a07f9b5).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21543      +/-   ##
============================================
+ Coverage     73.45%   73.53%   +0.08%     
- Complexity    74591    74626      +35     
============================================
  Files          5980     5980              
  Lines        338779   338779              
  Branches      48848    48848              
============================================
+ Hits         248849   249124     +275     
+ Misses        70079    69818     -261     
+ Partials      19851    19837      -14

☔ View full report in Codecov by Sentry.
📢 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.

mch2 · 2026-05-07T21:36:05Z

pls add a test in our qa pkg - similar tests https://github.com/opensearch-project/OpenSearch/tree/main/sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa

Signed-off-by: Tanik Pansuriya <panbhai@amazon.com>

github-actions · 2026-05-08T20:39:12Z

Persistent review updated to latest commit a07f9b5

github-actions · 2026-05-08T21:36:29Z

✅ Gradle check result for a07f9b5: SUCCESS

tanik98 force-pushed the string-scalar-ppl branch from 88ca200 to f0c0566 Compare May 7, 2026 18:56

tanik98 force-pushed the string-scalar-ppl branch from f0c0566 to 7abb1d1 Compare May 7, 2026 19:52

tanik98 marked this pull request as ready for review May 7, 2026 20:10

tanik98 requested a review from a team as a code owner May 7, 2026 20:10

tanik98 force-pushed the string-scalar-ppl branch from 7abb1d1 to 3a69307 Compare May 7, 2026 20:16

mch2 reviewed May 7, 2026

View reviewed changes

Comment thread ...tafusion/src/main/java/io/substrait/isthmus/expression/NameBasedScalarFunctionConverter.java Outdated

mch2 reviewed May 7, 2026

View reviewed changes

Comment thread sandbox/plugins/analytics-backend-datafusion/rust/src/udf/strcmp.rs Outdated

Wire PPL string scalar functions with analytics-backend-datafusion

a07f9b5

Signed-off-by: Tanik Pansuriya <panbhai@amazon.com>

tanik98 force-pushed the string-scalar-ppl branch from 3a69307 to a07f9b5 Compare May 8, 2026 20:37

tanik98 requested a review from mch2 May 8, 2026 21:14

mch2 approved these changes May 8, 2026

View reviewed changes

mch2 merged commit 239f4d9 into opensearch-project:main May 8, 2026
18 checks passed

opensearch-ci-bot mentioned this pull request May 27, 2026

[AUTO] Add release notes for 3.7.0 #21843

Merged

opensearch-ci-bot mentioned this pull request Jun 17, 2026

[AUTOCUT] Gradle Check Flaky Test Report for IngestFromKafkaIT #17215

Open

Navigation Menu

Uh oh!

Wire PPL string scalar functions with analytics-backend-datafusion - #21543

Wire PPL string scalar functions with analytics-backend-datafusion#21543
mch2 merged 1 commit into
opensearch-project:mainfrom
tanik98:string-scalar-ppl

tanik98 commented May 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

mch2 commented May 7, 2026

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tanik98 commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Functions added

Key changes

Related Issues

Check List

Uh oh!

github-actions Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit a07f9b5)

Uh oh!

github-actions Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mch2 commented May 7, 2026

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

github-actions Bot commented May 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tanik98 commented May 7, 2026 •

edited

Loading

github-actions Bot commented May 7, 2026 •

edited

Loading

(Review updated until commit `a07f9b5`)

github-actions Bot commented May 7, 2026 •

edited

Loading

codecov Bot commented May 7, 2026 •

edited

Loading