Skip to content

[Analytics Backend / DataFusion] Onboard PPL regex command + regexp_match() to DataFusion analytics-engine route - #21524

Merged
mch2 merged 1 commit into
opensearch-project:mainfrom
RyanL1997:mustang-regex
May 7, 2026
Merged

[Analytics Backend / DataFusion] Onboard PPL regex command + regexp_match() to DataFusion analytics-engine route#21524
mch2 merged 1 commit into
opensearch-project:mainfrom
RyanL1997:mustang-regex

Conversation

@RyanL1997

@RyanL1997 RyanL1997 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Description

Wires Calcite REGEXP_CONTAINS — the lowering target for both PPL regex command and regexp_match() function — through the analytics-engine path, so PPL queries like ... | regex firstname='Amber' and ... | eval m = regexp_match(field, pattern) route through DataFusion natively when the indices are parquet-backed.

This follows the same templated shape as #21472 (PPL fillnull via COALESCE) — onboarding a Calcite scalar that DataFusion already knows how to execute, with no Rust-side or DataFusion-extension changes.

How it failed before

CalciteRelNodeVisitor.visitRegex already lowers the PPL regex command into LogicalFilter(REGEXP_CONTAINS(field, pattern)) (and regexp_match() into a project-side REGEXP_CONTAINS) — verified by CalcitePPLRegexTest. But running CalciteRegexCommandIT against an analytics-engine-routed cluster yielded:

java.lang.IllegalArgumentException: Unable to convert call REGEXP_CONTAINS(string?, string).

Isthmus had no Calcite→Substrait mapping for SqlLibraryOperators.REGEXP_CONTAINS, so every regex query failed at substrait serialization before reaching DataFusion.

Changes

  1. ScalarFunction.REGEXP_CONTAINS — new enum constant in Category.FULL_TEXT, SqlKind.OTHER_FUNCTION. Resolves through fromSqlFunction(name)valueOf(name.toUpperCase()) since REGEXP_CONTAINS is the operator's name in Calcite.
  2. DataFusionAnalyticsBackendPlugin.STANDARD_FILTER_OPS + STANDARD_PROJECT_OPS — declare REGEXP_CONTAINS. Filter side covers the regex command (lowered to a Filter); project side covers regexp_match() in eval projections (returns BOOLEAN).
  3. opensearch_scalar_functions.yaml — add the regex_match Substrait extension declaration (varchar, varchar) → boolean, mirroring the existing ilike entry.
  4. DataFusionFragmentConvertor.ADDITIONAL_SCALAR_SIGS — add FunctionMappings.s(SqlLibraryOperators.REGEXP_CONTAINS, "regex_match"). This is the Calcite-op→Substrait-name bridge: Isthmus serializes the call as the regex_match extension, and datafusion-substrait 52's consumer maps that name to Operator::RegexMatch (the binary regex operator backing PostgreSQL's ~), which executes via arrow-string's regex kernel and returns BOOLEAN.

No Rust-side, convertor, or DataFusion-extension changes — DataFusion's substrait crate already resolves regex_match to a native operator.

Test results

RegexCommandIT (new, self-contained, 13 tests on the parquet-backed calcs dataset):

Phase Result
Before All 13 fail with Unable to convert call REGEXP_CONTAINS(string?, string)
After this PR 13/13 pass

Coverage: exact match, contains-substring, ^anchor / $anchor, character classes, negated !=, full-row content check, regexp_match() in eval (BOOLEAN column), and the SQL-plugin preflight type-check error path (regex on numeric field).

CalciteRegexCommandIT from the SQL plugin, force-routed through analytics-engine via tests.analytics.{parquet_indices,force_routing}=true:

Phase Result
Before 0/5 pass (all fail with Unable to convert call)
After this PR 5/5 pass (and 5/5 pass in the CalciteNoPushdownIT suite variant)

./gradlew check -p sandbox -Dsandbox.enabled=true: BUILD SUCCESSFUL (compile, unit tests, spotless, forbiddenApis, dependencyLicenses, licenseHeaders all green).

Routing evidence (cluster log under force_routing=true)

LogicalFilter(condition=[REGEXP_CONTAINS($7, 'Amber':VARCHAR)])
OpenSearchFilter(condition=[ANNOTATED_PREDICATE(id=0, backends=[datafusion],
                  REGEXP_CONTAINS($7, 'Amber':VARCHAR))], viableBackends=[[datafusion]])

The backends=[datafusion] annotation confirms the analytics-engine planner selected DataFusion to execute the regex filter. Negated form (regex field!='X') round-trips correctly: the planner wraps the annotation in NOT(...) without losing backend viability.

POC framing for future bucket-1 functions

Three knobs apply to any Calcite scalar that DataFusion executes natively:

  • ScalarFunction enum entry (only if SqlKind is OTHER_FUNCTION and the name doesn't already map)
  • Membership in STANDARD_FILTER_OPS / STANDARD_PROJECT_OPS
  • ADDITIONAL_SCALAR_SIGS + opensearch_scalar_functions.yaml if Isthmus's built-in mapping doesn't cover the operator

For regex, all three were needed because REGEXP_CONTAINS is BigQuery-named and not in Isthmus's default Calcite→Substrait function table. Once the Substrait extension is named regex_match (the Substrait standard name DataFusion resolves), the rest is wiring.

Check List

  • New functionality includes testing.
  • All tests pass.
  • New functionality has been documented (PR description; user-facing PPL regex / regexp_match syntax unchanged).
  • API changes companion pull request created — N/A.
  • Public documentation issue/PR created — N/A.

…gine route

Signed-off-by: Jialiang Liang <jiallian@amazon.com>
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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: Register REGEXP_CONTAINS scalar function in DataFusion analytics backend

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 integration tests for PPL regex command and regexp_match() function

Relevant files:

  • sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/RegexCommandIT.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 duplication was present before but remains unaddressed in this PR. It may cause unexpected behavior or silent overrides in function mapping resolution.

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")
Test Comment Mismatch

The comment for testRegexCharacterClass says "matches BINDING (BINDING MACHINES, BINDING SUPPLIES) but not BUSINESS", but the pattern used is 'BINDING' — not a character class pattern like [BC]INDING. The test name and comment suggest a character class test, but the actual pattern doesn't exercise that behavior.

public void testRegexCharacterClass() throws IOException {
    // [BC]INDING matches BINDING (BINDING MACHINES, BINDING SUPPLIES) but not BUSINESS.
    assertRowCount("source=" + DATASET.indexName + " | regex str1='BINDING' | fields str1", 2);
}
Static Mutable State

The dataProvisioned flag is a static mutable field used to guard lazy provisioning. In a parallel or multi-JVM test execution environment, this could lead to race conditions where provisioning is skipped or executed multiple times. Consider using a thread-safe mechanism or a @BeforeClass-equivalent approach.

private static boolean dataProvisioned = false;

/**
 * Lazily provision the calcs dataset on first invocation. Mirrors the
 * {@code FillNullCommandIT} pattern — {@code client()} is unavailable at static init.
 */
private void ensureDataProvisioned() throws IOException {
    if (dataProvisioned == false) {
        DatasetProvisioner.provision(client(), DATASET);
        dataProvisioned = true;
    }
}
JSON Injection Risk

The executePpl method constructs the JSON body via string concatenation using escapeJson(ppl). If escapeJson does not properly handle all edge cases (e.g., Unicode escapes, nested quotes), this could result in malformed JSON or potential injection in test queries. Verify that escapeJson is robust.

private Map<String, Object> executePpl(String ppl) throws IOException {
    ensureDataProvisioned();
    Request request = new Request("POST", "/_analytics/ppl");
    request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
    Response response = client().performRequest(request);
    return assertOkAndParse(response, "PPL: " + ppl);
}

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove duplicate function mapping entry

DelegatedPredicateFunction.FUNCTION is registered twice in ADDITIONAL_SCALAR_SIGS.
The duplicate entry is redundant and could cause unexpected behavior or conflicts
during function resolution. Remove the duplicate mapping.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [94-97]

 FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME),
     FunctionMappings.s(SqlLibraryOperators.ILIKE, "ilike"),
-    FunctionMappings.s(DelegatedPredicateFunction.FUNCTION, DelegatedPredicateFunction.NAME),
Suggestion importance[1-10]: 6

__

Why: DelegatedPredicateFunction.FUNCTION is indeed registered twice in ADDITIONAL_SCALAR_SIGS (lines 95 and 97), which is a pre-existing issue visible in the diff. While this duplicate existed before the PR, it's a valid concern that could cause unexpected behavior during function resolution.

Low
General
Fix thread-safety of static provisioning flag

The dataProvisioned flag is a non-volatile static field accessed and mutated without
synchronization. If tests run in parallel, multiple threads could concurrently enter
the provisioning block, causing duplicate provisioning or race conditions. Use a
volatile modifier or a synchronized block to ensure safe lazy initialization.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/RegexCommandIT.java [46-57]

-private static boolean dataProvisioned = false;
+private static volatile boolean dataProvisioned = false;
 
-/**
- * Lazily provision the calcs dataset on first invocation. Mirrors the
- * {@code FillNullCommandIT} pattern — {@code client()} is unavailable at static init.
- */
 private void ensureDataProvisioned() throws IOException {
     if (dataProvisioned == false) {
-        DatasetProvisioner.provision(client(), DATASET);
-        dataProvisioned = true;
+        synchronized (RegexCommandIT.class) {
+            if (dataProvisioned == false) {
+                DatasetProvisioner.provision(client(), DATASET);
+                dataProvisioned = true;
+            }
+        }
     }
 }
Suggestion importance[1-10]: 4

__

Why: The thread-safety concern for dataProvisioned is valid in theory, but integration tests typically run sequentially rather than in parallel, making this a low-priority issue. The double-checked locking pattern in the improved code is correct and would prevent race conditions if parallel execution were used.

Low
Ensure safe JSON escaping of query strings

The escapeJson method is called but never defined in this class or shown to be
inherited. If it is not available in the parent class, special characters in PPL
queries (e.g., quotes, backslashes) will cause malformed JSON and hard-to-diagnose
test failures. Ensure escapeJson is properly defined or use a JSON library to safely
serialize the query string.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/RegexCommandIT.java [229-235]

 private Map<String, Object> executePpl(String ppl) throws IOException {
     ensureDataProvisioned();
     Request request = new Request("POST", "/_analytics/ppl");
-    request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
+    // Use a JSON library or a robust escape to avoid malformed JSON for special characters
+    String escaped = ppl.replace("\\", "\\\\").replace("\"", "\\\"");
+    request.setJsonEntity("{\"query\": \"" + escaped + "\"}");
     Response response = client().performRequest(request);
     return assertOkAndParse(response, "PPL: " + ppl);
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a valid concern about escapeJson being undefined in this class, but since this is a test class extending AnalyticsRestTestCase, escapeJson is likely inherited from the parent class. The suggestion score is low because it questions a method that is probably defined in the outer codebase.

Low

@RyanL1997
RyanL1997 marked this pull request as ready for review May 6, 2026 22:31
@RyanL1997
RyanL1997 requested a review from a team as a code owner May 6, 2026 22:31
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for a968223: 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

Copy link
Copy Markdown
Contributor Author

Transferring internal conversation with @penghuo to here:

DataFusion side was already done — DF has a native Operator::RegexMatch (the binary regex operator that backs PostgreSQL's ~), plus arrow-string's regex kernel underneath. Returns BOOLEAN. Same semantics as Calcite REGEXP_CONTAINS.

Thats why this change is just a direct projection of mappings

@RyanL1997 RyanL1997 changed the title Onboard PPL regex command + regexp_match() to DataFusion analytics-engine route [Analytics Backend / DataFusion] Onboard PPL regex command + regexp_match() to DataFusion analytics-engine route May 7, 2026
@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for a968223: SUCCESS

@codecov

codecov Bot commented May 7, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21524      +/-   ##
============================================
- Coverage     73.44%   73.38%   -0.07%     
+ Complexity    74426    74404      -22     
============================================
  Files          5970     5970              
  Lines        338267   338267              
  Branches      48753    48753              
============================================
- Hits         248451   248226     -225     
- Misses        70042    70209     +167     
- Partials      19774    19832      +58     

☔ 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
mch2 merged commit 8d64f1d into opensearch-project:main May 7, 2026
23 of 25 checks passed
imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request May 8, 2026
…gine route (opensearch-project#21524)

Signed-off-by: Jialiang Liang <jiallian@amazon.com>
vishwasgarg18 pushed a commit to vishwasgarg18/OpenSearch that referenced this pull request May 8, 2026
…gine route (opensearch-project#21524)

Signed-off-by: Jialiang Liang <jiallian@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants