[Analytics Backend / DataFusion] Wire PPL replace command + replace()/regexp_replace() functions

[RyanL1997]

Description

Onboards the PPL replace command and the replace() / regexp_replace() eval functions to the analytics-engine route by mapping their two Calcite lowering targets — SqlStdOperatorTable.REPLACE and SqlLibraryOperators.REGEXP_REPLACE_3 — through Substrait to DataFusion's native replace and regexp_replace UDFs.

Same templated shape as the fillnull POC (#21472) and the ABS/SUBSTRING follow-up (#21521):

ScalarFunction enum constant
  + STANDARD_PROJECT_OPS membership
  + opensearch_scalar_functions.yaml extension entry
  + ADDITIONAL_SCALAR_SIGS Calcite-op→Substrait-name bridge
  = onboarded to the analytics route.

Two scalar functions added: REPLACE (literal substring replace) and REGEXP_REPLACE (regex replace). Both project-side only — the comparison result of a replaced field is filtered via the existing EQUALS capability, so no STANDARD_FILTER_OPS additions are needed.

Why an adapter is necessary for REGEXP_REPLACE

PPL's wildcard replace form lowers via the SQL plugin's WildcardUtils.convertWildcardPatternToRegex() to a Java-Pattern-compatible regex. Two flavors of Java syntax need translation before substrait serialization, because DataFusion uses Rust's regex crate which has different parsing rules:

1. \Q…\E quoted-literal blocks. Rust rejects \Q as an unrecognized escape sequence:

   External error: regex parse error:
       ^\QBUSINESS\E(.*?)\Q\E$
        ^^
   error: unrecognized escape sequence
   

   The adapter expands each \Q…\E block to per-character escaped literals — semantics-preserving, no behavior change.

2. $N numeric backreferences in the replacement. Rust's replacement parser is identifier-greedy: $1_$2 is parsed as a reference to the (non-existent) group named 1_ followed by group $2, yielding empty + group-2's value. Java's Matcher.replaceAll stops at the first non-digit, so $1_$2 means group-1 + literal underscore + group-2. The adapter wraps every numeric backreference in braces (${N}) for unambiguous Rust parsing.

Both transforms live in RegexpReplaceAdapter and are registered against ScalarFunction.REGEXP_REPLACE in scalarFunctionAdapters(). Calls without \Q in the pattern AND without bare $N in the replacement pass through unchanged.

Test results

Suite	Result
RegexpReplaceAdapterTests (unit)	19/19 — unquote: 9, brace: 7, dual-rewrite integration: 3
ReplaceCommandIT (new self-contained QA IT, calcs dataset)	10/10 — literal command (single + multi-pair = nested REPLACE), wildcard prefix/suffix, replace() / regexp_replace() in eval, full-row content checks, no-match passthrough, multi-field IN clause
SQL plugin's CalciteReplaceCommandIT force-routed via -Dtests.analytics.{force_routing,parquet_indices}=true	21/21 in both the direct suite and the CalciteNoPushdownIT re-run

./gradlew check -p sandbox -Dsandbox.enabled=true is green except for the unrelated upstream ScalarDateTimeFunctionIT > testConvertTz flake (added by bd8e81027a6 from #21476).

Companion PR

opensearch-project/sql#5415 — makes CalciteReplaceCommandIT column-order-agnostic for the analytics-engine route, mirroring the rename precedent from #5413. Required for the SQL-plugin-side 21/21 result above.

Check List

• New functionality includes testing.
• All tests pass (excluding the unrelated upstream testConvertTz flake).
• New functionality has been documented (this PR description; class javadoc on RegexpReplaceAdapter documents the Java↔Rust regex divergence).
• API changes companion pull request created — N/A.
• Public documentation issue/PR created — N/A.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2f880e7)

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: Wire REPLACE and REGEXP_REPLACE scalar functions through Substrait Relevant files: sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java sandbox/plugins/analytics-backend-datafusion/src/main/resources/opensearch_scalar_functions.yaml Sub-PR theme: Add RegexpReplaceAdapter for Java-to-Rust regex syntax normalization Relevant files: sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/RegexpReplaceAdapterTests.java Sub-PR theme: Add integration tests for PPL replace command and replace/regexp_replace functions Relevant files: sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/ReplaceCommandIT.java

⚡ Recommended focus areas for review Duplicate Entry The ADDITIONAL_SCALAR_SIGS list contains a duplicate entry for DelegatedPredicateFunction.FUNCTION mapped to DelegatedPredicateFunction.NAME. This was present before the PR and remains in the new code. While not introduced by this PR, the new entries were added alongside it and the list should be reviewed for correctness. private static final List<FunctionMappings.Sig> ADDITIONAL_SCALAR_SIGS = List.of( FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), FunctionMappings.s(SqlLibraryOperators.ILIKE, "ilike"), FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), FunctionMappings.s(SqlLibraryOperators.DATE_PART, "date_part"), FunctionMappings.s(ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, "convert_tz"), FunctionMappings.s(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, "to_unixtime"), FunctionMappings.s(SqlLibraryOperators.REGEXP_CONTAINS, "regex_match"), FunctionMappings.s(SqlStdOperatorTable.REPLACE, "replace"), FunctionMappings.s(SqlLibraryOperators.REGEXP_REPLACE_3, "regexp_replace") ); Non-Literal Replacement When the replacement operand is a non-literal (e.g., a column reference), the $N backreference brace-wrapping is silently skipped. This is documented as a pass-through, but there is no warning or comment explaining that runtime failures may occur if a column value contains bare $N patterns in a Rust regex context. Consider at least logging a warning. String rewrittenReplacement = null; if (replacementOperand instanceof RexLiteral replacementLiteral) { String replacement = replacementLiteral.getValueAs(String.class); if (replacement != null && replacement.indexOf('$') >= 0) { String rewritten = braceBackreferences(replacement); if (!replacement.equals(rewritten)) { rewrittenReplacement = rewritten; } } } Static State Risk The dataProvisioned static boolean field is not thread-safe and could cause issues if tests are run in parallel. Additionally, using a static flag for test setup is fragile — if a previous test class leaves the index in a bad state, dataProvisioned will remain true and the provisioning step will be skipped. Consider using @BeforeClass or the framework's built-in provisioning mechanism instead. private static boolean dataProvisioned = false; private void ensureDataProvisioned() throws IOException { if (dataProvisioned == false) { DatasetProvisioner.provision(client(), DATASET); dataProvisioned = true; } } Unused Method The assertErrorContains helper method is defined but never called in any test. This dead code should either be used in a negative test case (e.g., testing an invalid regex pattern) or removed. private void assertErrorContains(String ppl, String expectedSubstring) { try { Map<String, Object> response = executePpl(ppl); fail("Expected query to fail with [" + expectedSubstring + "] but got response: " + response); } catch (ResponseException e) { String body; try { body = org.opensearch.test.rest.OpenSearchRestTestCase.entityAsMap(e.getResponse()).toString(); } catch (IOException ioe) { body = e.getMessage(); } assertTrue( "Expected response body to contain [" + expectedSubstring + "] but was: " + body, body.contains(expectedSubstring) ); } catch (IOException e) { fail("Unexpected IOException: " + e); } } Metachar Escaping The REGEX_METACHARS string includes / as a metacharacter to escape inside \Q…\E blocks. However, / is not a regex metacharacter in Java's Pattern or Rust's regex crate — it has no special meaning and does not need escaping. This could produce unnecessarily escaped output like \/ which, while harmless in most engines, may be unexpected. private static final String REGEX_METACHARS = ".\\+*?^$()[]{}|/";

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 2f880e7
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove duplicate function mapping entry There is a duplicate entry for DelegatedPredicateFunction.FUNCTION in ADDITIONAL_SCALAR_SIGS. The same function/name pair appears twice, which is redundant and may cause unexpected behavior in function resolution. Remove the duplicate entry. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [101-103] FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), FunctionMappings.s(SqlLibraryOperators.ILIKE, "ilike"), - FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), Suggestion importance[1-10]: 7 __ Why: The duplicate DelegatedPredicateFunction.FUNCTION entry at line 103 is a real bug that existed before this PR (visible in both old and new hunks). While this PR didn't introduce it, removing it would prevent potential issues with function resolution. The suggestion correctly identifies the redundancy and the improved_code accurately reflects the fix.	Medium
General	Fix thread-safety of static provisioning flag The dataProvisioned static flag is not thread-safe. If tests run in parallel, multiple threads could simultaneously see dataProvisioned == false and call DatasetProvisioner.provision() concurrently, potentially causing race conditions or duplicate provisioning. Use a @BeforeClass-equivalent mechanism or synchronize access to this flag. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/ReplaceCommandIT.java [52-59] -private static boolean dataProvisioned = false; +private static volatile boolean dataProvisioned = false; -private void ensureDataProvisioned() throws IOException { +private synchronized void ensureDataProvisioned() throws IOException { if (dataProvisioned == false) { DatasetProvisioner.provision(client(), DATASET); dataProvisioned = true; } } Suggestion importance[1-10]: 4 __ Why: Adding volatile and synchronized to the provisioning mechanism is a valid thread-safety improvement. However, in typical OpenSearch test frameworks, tests usually run sequentially within a class, making this a low-priority concern in practice.	Low
	Document greedy identifier matching boundary behavior The braceBackreferences method only wraps numeric backreferences ($1, $12), but Rust's regex crate also supports named backreferences like $name. However, the more critical issue is that the digit-scanning loop starts at i + 1 (the position of next, which is already confirmed to be a digit), but then the loop condition re-checks charAt(j) starting at i + 1 — this is correct. However, the loop should also stop at non-digit characters that are valid Rust identifier chars (letters, underscores) to ensure the brace boundary is placed correctly. Currently, $1abc would produce ${1}abc which is correct, but $1_ would produce ${1}_ which is also correct. The logic is fine for digits-only group names, but if the intent is to handle all cases where Rust would greedily extend the group name (including letters and underscores after digits), the loop should consume all identifier characters, not just digits. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java [140-146] if (Character.isDigit(next)) { // Bare $N — wrap in braces so Rust doesn't consume following identifier // characters (letters, digits, underscores) as part of the group name. int j = i + 1; while (j < replacement.length() && Character.isDigit(replacement.charAt(j))) { j++; } + // After digits, Rust also greedily consumes letters and underscores as part + // of the group name. Wrap only the digit portion so the boundary is explicit. Suggestion importance[1-10]: 2 __ Why: The suggestion acknowledges the existing logic is correct for the intended use case (numeric group names only), and the improved_code only adds a comment without changing behavior. This is essentially a documentation-only suggestion with minimal impact.	Low

---

Previous suggestions

Suggestions up to commit 4083b7e

Category	Suggestion	Impact
Possible issue	Remove duplicate function mapping entry DelegatedPredicateFunction.FUNCTION is registered twice in ADDITIONAL_SCALAR_SIGS. The duplicate entry (line 4 of the list) is likely a copy-paste error and may cause unexpected behavior or conflicts during Substrait serialization. Remove the duplicate entry. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [96-105] private static final List<FunctionMappings.Sig> ADDITIONAL_SCALAR_SIGS = List.of( FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), FunctionMappings.s(SqlLibraryOperators.ILIKE, "ilike"), - FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME), FunctionMappings.s(SqlLibraryOperators.DATE_PART, "date_part"), FunctionMappings.s(ConvertTzAdapter.LOCAL_CONVERT_TZ_OP, "convert_tz"), FunctionMappings.s(UnixTimestampAdapter.LOCAL_TO_UNIXTIME_OP, "to_unixtime"), FunctionMappings.s(SqlStdOperatorTable.REPLACE, "replace"), FunctionMappings.s(SqlLibraryOperators.REGEXP_REPLACE_3, "regexp_replace") ); Suggestion importance[1-10]: 7 __ Why: The duplicate DelegatedPredicateFunction.FUNCTION entry at line 99 is a pre-existing issue (visible in both old and new hunks), but it's a real bug that could cause conflicts during Substrait serialization. The improved code correctly removes the duplicate while preserving all other entries.	Medium
General	Fix thread-safety of static provisioning flag The dataProvisioned static flag is not thread-safe. If tests run in parallel, multiple threads could simultaneously pass the if check and call DatasetProvisioner.provision() more than once, potentially causing race conditions or duplicate provisioning errors. Use a @BeforeClass / @AfterClass pattern or synchronize access to the flag. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/ReplaceCommandIT.java [52-59] -private static boolean dataProvisioned = false; +private static volatile boolean dataProvisioned = false; -private void ensureDataProvisioned() throws IOException { +private synchronized void ensureDataProvisioned() throws IOException { if (dataProvisioned == false) { DatasetProvisioner.provision(client(), DATASET); dataProvisioned = true; } } Suggestion importance[1-10]: 4 __ Why: The thread-safety concern is valid for parallel test execution, but integration tests in OpenSearch typically run sequentially. The volatile + synchronized approach in the improved code is a reasonable fix, though a @BeforeClass pattern would be cleaner.	Low
	Fail fast on unexpected operand count The adapter silently passes through calls with a wrong operand count instead of signaling an error. Since REGEXP_REPLACE_3 is defined to always have exactly 3 operands, a mismatch indicates a programming error rather than a valid runtime case. Consider throwing an IllegalArgumentException to surface misuse early. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/RegexpReplaceAdapter.java [60-62] if (original.getOperands().size() != 3) { - return original; + throw new IllegalArgumentException( + "RegexpReplaceAdapter expects exactly 3 operands but got " + original.getOperands().size() + ); } Suggestion importance[1-10]: 3 __ Why: While failing fast is generally good practice, the silent pass-through is a reasonable defensive approach for an adapter that may encounter unexpected call shapes. The suggestion is valid but has limited impact since REGEXP_REPLACE_3 is always 3-arity by definition.	Low

[github-actions]

❌ Gradle check result for 4083b7e: 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?

[RyanL1997]

The conflicts are caused by the merge of #21524, will resolve it soon.

[github-actions]

Persistent review updated to latest commit 2f880e7

[github-actions]

✅ Gradle check result for 2f880e7: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.49%. Comparing base (6888345) to head (2f880e7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21527      +/-   ##
============================================
+ Coverage     73.42%   73.49%   +0.07%     
- Complexity    74547    74628      +81     
============================================
  Files          5978     5978              
  Lines        338743   338743              
  Branches      48843    48843              
============================================
+ Hits         248707   248947     +240     
+ Misses        70229    69925     -304     
- Partials      19807    19871      +64     

☔ 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.