Skip to content

Wire PPL where command through the analytics-engine path - #21502

Merged
mch2 merged 2 commits into
opensearch-project:mainfrom
ahkcs:feature/mustang-where-command
May 6, 2026
Merged

Wire PPL where command through the analytics-engine path#21502
mch2 merged 2 commits into
opensearch-project:mainfrom
ahkcs:feature/mustang-where-command

Conversation

@ahkcs

@ahkcs ahkcs commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Drives the analytics-engine route to parity for the PPL where command. Same shape as the fillnull walkthrough (#21472): planner fix, backend capability adds, QA-side IT.

Failure modes addressed

Each commit corresponds to a distinct throw site observed when running CalciteWhereCommandIT against a force-routed analytics-engine cluster.

# Bucket Throw site (deepest cluster-side frame) Fix
1 S0 planner OpenSearchFilterRule.resolveViableBackends → Unrecognized filter operator [SEARCH] Expand SEARCH(col, sarg) to OR/AND/EQUALS via RexUtil.expandSearch before annotation. Triggered by every multi-value IN / chained equality once ReduceExpressionsRule (in PlannerImpl) drives RexSimplify.
2a S0 capability OpenSearchProjectRule.annotateExpr → No backend supports scalar function [CAST] Add ScalarFunction.CAST to STANDARD_PROJECT_OPS. ReduceExpressionsRule.ProjectReduceExpressionsRule constant-folds field refs through equality filters into typed literals — where str0='FURNITURE' | fields str0 becomes Project[CAST('FURNITURE' AS VARCHAR)].
2b S0 capability OpenSearchProjectRule.annotateExpr → No backend supports scalar function [EQUALS] Add the comparison set (EQUALS, NOT_EQUALS, GT/GE/LT/LE) to STANDARD_PROJECT_OPS. PPL eval x = (a == b) projects the comparison itself as a boolean column.
3a S1 adapter substrait isthmus → Unable to convert call ILIKE(string?, char<N>, char<1>) New IlikeFunctionAdapter rewriting ILIKE(field, pattern) → LIKE(LOWER(field), LOWER(pattern)). PPL emits ILIKE for contains and for LIKE when plugins.ppl.syntax.legacy.preferred=true (its default). Both LOWER and 2-arg LIKE are native to DataFusion.
3b S1 adapter substrait isthmus → Unable to convert call LIKE(string?, char<N>, char<1>) Same adapter strips the explicit '\\' escape literal (always emitted by PPLFuncImpTable, type CHAR(1), mismatches substrait's like signature). PPL never uses a non-default escape — the contains operator pre-escapes user-provided literals before composing the wildcard pattern.

Test outcome

SQL plugin — CalciteWhereCommandIT via :integ-test:analyticsCompatibilityTest (with tests.analytics.force_routing=true and tests.analytics.parquet_indices=true):

39 tests, 29 passing, 10 failing

Going from 0/39 → 29/39. All 10 remaining failures are command-orthogonal infrastructure gaps — they show up identically on any other PPL command that touches metadata fields, nested fields, full-text predicates, or date-part shorthands:

Throw site Count Tests
OpenSearchSchemaBuilder → Field [_id] not found 2 testWhereWithMetadataFields, testWhereWithMetadataFields2
OpenSearchSchemaBuilder → Field [<dotted.path>] not found 6 testFilterOnComputedNestedFields, testFilterOnNestedAndRootFields, testFilterOnNestedFields, testFilterOnMultipleCascadedNestedFields, testScriptFilterOnDifferentNestedHierarchyShouldThrow, testAggFilterOnNestedFields
OpenSearchFilterRule.resolveViableBackends → Constant predicate with no field references reached the filter rule: [query_string(...)] 1 testWhereEquivalentSortCommand
substrait isthmus → Unable to convert call MONTH(precision_timestamp<...>?) 1 testFilterScriptPushDownWithPPLBuiltInFunction

The MONTH gap follows the bucket-2 ILIKE pattern (rewrite to EXTRACT(MONTH FROM ts)) but the analytics-backend-datafusion plugin's compile classpath does not currently include org.apache.calcite.avatica.util.TimeUnitRange, which the EXTRACT operator's first operand requires. Will land separately once the classpath/dependency change is sorted.

The metadata-field, nested-field, and query_string failures are schema/planner gaps that span many PPL commands and are out of scope for a single function PR.

OpenSearch core — WhereCommandIT under sandbox/qa/analytics-engine-rest, hitting POST /_analytics/ppl against a parquet-backed calcs index:

26 tests, 26 passing

Covers comparisons (=, ==, !=, <, >, <=, >=), boolean connectives (AND, OR, NOT), IS [NOT] NULL via isnull/isnotnull, IN / NOT IN against keyword and numeric columns, LIKE function and operator with % / _ wildcards, contains (ILIKE), and inner scalar calls inside predicates (length, abs, arithmetic +).

This is the long-term verification surface: future where-command coverage lands here, and the SQL-plugin IT becomes the v2-path baseline.

Local validation

./gradlew check -p sandbox -Dsandbox.enabled=true
# BUILD SUCCESSFUL in 1m 25s

./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*WhereCommandIT"
# 26 tests, 0 failed

Pattern for future where-command additions

Bucket-2 / S1 adapter pattern: subsequent substrait-conversion gaps (e.g. MONTH/YEAR, additional library functions) follow the same ScalarFunctionAdapter shape introduced here. Bucket-1 / S0 capability adds (new functions DataFusion handles natively) are one-line additions to STANDARD_PROJECT_OPS / STANDARD_FILTER_OPS.

Check List

  • Functionality
  • Unit tests added
  • Integration tests added
  • Sandbox check runs clean
  • Commits are signed per the DCO

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

@ahkcs
ahkcs requested a review from a team as a code owner May 6, 2026 01:51
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

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

PathLineSeverityDescription
sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java316lowJSON body is constructed via string concatenation with escapeJson() rather than a structured serializer. The ppl values are all hardcoded in this file so there is no live injection risk, but the pattern is fragile if the helper is ever reused with dynamic input. Not malicious — purely a code-quality anomaly in a test file.

The table above displays the top 10 most important findings.

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

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

github-actions Bot commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 5f82f72)

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: Add CAST and comparison operators to STANDARD_PROJECT_OPS in DataFusion backend

Relevant files:

  • sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java
Sub-PR theme: Add integration tests for PPL where command on analytics-engine route

Relevant files:

  • sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java
⚡ Recommended focus areas for review

SQL Injection Risk

The executePpl method builds the JSON request body by string interpolation using escapeJson(ppl), where ppl itself is constructed by string concatenation with dataset index names and literal values. If escapeJson does not fully sanitize all special characters, a malformed PPL string could produce invalid or exploitable JSON. Verify that escapeJson is robust and consider using a proper JSON serializer instead.

 
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);
}
Shared Mutable State

dataProvisioned is a static boolean field shared across all test instances. In parallel or randomized test execution environments (common in OpenSearch's test framework), this flag may be read/written concurrently without synchronization, potentially causing the dataset to be provisioned multiple times or not at all. Consider using a thread-safe mechanism or a @BeforeClass equivalent that the framework guarantees runs once.

 
private static boolean dataProvisioned = false;

/**
 * Lazily provision the calcs dataset on first invocation. Same lazy-provision pattern
 * as {@link FillNullCommandIT} — {@code client()} is only reliably available inside a
 * test body, not in {@code @BeforeClass} / {@code setUp()}.
 */
private void ensureDataProvisioned() throws IOException {
    if (dataProvisioned == false) {
        DatasetProvisioner.provision(client(), DATASET);
        dataProvisioned = true;
    }
}
Hardcoded Row Counts

Many tests assert exact row counts (e.g., assertRowCount(..., 2), assertRowCount(..., 15)) that are tightly coupled to the specific contents of the calcs dataset. If the dataset is ever updated or regenerated, all these assertions will silently break without clear indication of what changed. Consider adding a brief comment or constant referencing the dataset version/size, or asserting relative properties where possible.

 
public void testWhereEqualOnKeyword() throws IOException {
    // 2 rows have str0='FURNITURE'.
    assertRowCount("source=" + DATASET.indexName + " | where str0 = 'FURNITURE' | fields str0", 2);
}

public void testWhereEqualOnDouble() throws IOException {
    assertRows(
        "source=" + DATASET.indexName + " | where num0 = 12.3 | fields str2, num0",
        row("one", 12.3)
    );
}

public void testWhereDoubleEqualOperator() throws IOException {
    // == is parsed as = at the AstExpressionBuilder layer; same plan, same result.
    assertRows(
        "source=" + DATASET.indexName + " | where num0 == 12.3 | fields str2, num0",
        row("one", 12.3)
    );
}

public void testWhereNotEqual() throws IOException {
    // 8 non-null distinct num0 values; != 0 keeps 7 rows (drops the single num0=0).
    assertRowCount("source=" + DATASET.indexName + " | where num0 != 0 | fields num0", 7);
}

public void testWhereGreaterThan() throws IOException {
    // num0 > 0 → {12.3, 15.7, 3.5, 10}.
    assertRowCount("source=" + DATASET.indexName + " | where num0 > 0 | fields num0", 4);
}

public void testWhereGreaterEqual() throws IOException {
    // num0 >= 0 → adds the row with num0=0 → 5 rows.
    assertRowCount("source=" + DATASET.indexName + " | where num0 >= 0 | fields num0", 5);
}

public void testWhereLessThan() throws IOException {
    // num0 < 0 → {-12.3, -15.7, -3.5}.
    assertRowCount("source=" + DATASET.indexName + " | where num0 < 0 | fields num0", 3);
}

public void testWhereLessEqual() throws IOException {
    // num0 <= 0 → adds num0=0 → 4 rows.
    assertRowCount("source=" + DATASET.indexName + " | where num0 <= 0 | fields num0", 4);
}

// ── Boolean connectives ─────────────────────────────────────────────────

public void testWhereAnd() throws IOException {
    // FURNITURE rows are key00 (num0=12.3) and key01 (num0=-12.3); AND num0>0 keeps key00.
    assertRows(
        "source=" + DATASET.indexName + " | where str0 = 'FURNITURE' and num0 > 0 | fields str2, num0",
        row("one", 12.3)
    );
}

public void testWhereOr() throws IOException {
    // num0 == 12.3 OR num0 == -12.3 → key00, key01.
    assertRowCount(
        "source=" + DATASET.indexName + " | where num0 == 12.3 OR num0 == -12.3 | fields num0",
        2
    );
}

public void testWhereNot() throws IOException {
    // NOT (str0 = 'FURNITURE') → 17 - 2 = 15 rows. (str0 has no nulls in calcs.)
    assertRowCount(
        "source=" + DATASET.indexName + " | where not str0 = 'FURNITURE' | fields str0",
        15
    );
}

public void testWhereMultipleChained() throws IOException {
    // Three filter steps: FURNITURE → num0>0 → str2='one'. Should leave one row.
    assertRows(
        "source=" + DATASET.indexName
            + " | where str0 = 'FURNITURE'"
            + " | where num0 > 0"
            + " | where str2 = 'one'"
            + " | fields str0, num0, str2",
        row("FURNITURE", 12.3, "one")
    );
}

// ── NULL handling via isnull() / isnotnull() ────────────────────────────

public void testWhereIsNull() throws IOException {
    // str2 has 4 null rows in calcs.
    assertRowCount(
        "source=" + DATASET.indexName + " | where isnull(str2) | fields str2",
        4
    );
}

public void testWhereIsNotNull() throws IOException {
    // str2 has 13 non-null rows in calcs.
    assertRowCount(
        "source=" + DATASET.indexName + " | where isnotnull(str2) | fields str2",
        13
    );
}

// ── IN / NOT IN ─────────────────────────────────────────────────────────

public void testWhereInOnKeyword() throws IOException {
    // FURNITURE (2) + OFFICE SUPPLIES (6) = 8.
    assertRowCount(
        "source=" + DATASET.indexName + " | where str0 in ('FURNITURE', 'OFFICE SUPPLIES') | fields str0",
        8
    );
}

public void testWhereInOnNumeric() throws IOException {
    // num0 IN (12.3, -12.3) → key00, key01 = 2 rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where num0 in (12.3, -12.3) | fields num0",
        2
    );
}

public void testWhereNotIn() throws IOException {
    // Complement of (FURNITURE, OFFICE SUPPLIES): 9 TECHNOLOGY rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where not str0 in ('FURNITURE', 'OFFICE SUPPLIES') | fields str0",
        9
    );
}

// ── LIKE function and operator ──────────────────────────────────────────

public void testWhereLikeFunction() throws IOException {
    // like(str0, 'FURN%') → 2 FURNITURE rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where like(str0, 'FURN%') | fields str0",
        2
    );
}

public void testWhereLikeOperator() throws IOException {
    // str0 LIKE 'OFF%' → 6 OFFICE SUPPLIES rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where str0 LIKE 'OFF%' | fields str0",
        6
    );
}

public void testWhereLikeUnderscoreWildcard() throws IOException {
    // 'on_' matches 'one' only (3 chars starting with "on").
    assertRows(
        "source=" + DATASET.indexName + " | where str2 LIKE 'on_' | fields str2",
        row("one")
    );
}

public void testWhereLikeNoMatch() throws IOException {
    assertRowCount(
        "source=" + DATASET.indexName + " | where like(str0, 'XYZ%') | fields str0",
        0
    );
}

// ── CONTAINS (lowers to ILIKE — case-insensitive) ───────────────────────

public void testWhereContains() throws IOException {
    // 'URN' inside FURNITURE → 2 rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where str0 contains 'URN' | fields str0",
        2
    );
}

public void testWhereContainsCaseInsensitive() throws IOException {
    // Lowercase pattern still hits FURNITURE because contains uses ILIKE.
    assertRowCount(
        "source=" + DATASET.indexName + " | where str0 contains 'urn' | fields str0",
        2
    );
}

// ── Sub-expression scalar calls (pass through to DataFusion) ────────────

public void testWhereInnerLength() throws IOException {
    // length('FURNITURE') = 9 → 2 rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where length(str0) = 9 | fields str0",
        2
    );
}

public void testWhereInnerAbs() throws IOException {
    // abs(num0) > 10 → {-15.7, -12.3, 12.3, 15.7} = 4 rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where abs(num0) > 10 | fields num0",
        4
    );
}

public void testWhereInnerArithmetic() throws IOException {
    // num0 + 100 > 105 ⇔ num0 > 5 → {12.3, 15.7, 10} = 3 rows.
    assertRowCount(
        "source=" + DATASET.indexName + " | where num0 + 100 > 105 | fields num0",
        3
    );
}
Unchecked Cast

Both assertRowCount and assertRows perform unchecked casts of response.get("rows") to List<List<Object>>. If the response schema changes or an error response is returned (e.g., a 200 with an error body), this cast will throw a ClassCastException with a confusing message. Consider adding a type-check guard before casting.

 
@SuppressWarnings("unchecked")
List<List<Object>> actualRows = (List<List<Object>>) response.get("rows");
assertNotNull("Response missing 'rows' field for query: " + ppl, actualRows);

@github-actions

github-actions Bot commented May 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 5f82f72

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Use safe JSON serialization for request body

The PPL query is embedded into a JSON string by manual concatenation with
escapeJson, but if escapeJson doesn't handle all JSON special characters (e.g.,
backslash, control characters), this can produce malformed JSON. Consider using a
proper JSON serialization library (e.g., Jackson's ObjectMapper) to build the
request body safely.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [323-329]

 private Map<String, Object> executePpl(String ppl) throws IOException {
     ensureDataProvisioned();
     Request request = new Request("POST", "/_analytics/ppl");
-    request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
+    String body = new com.fasterxml.jackson.databind.ObjectMapper()
+        .writeValueAsString(java.util.Collections.singletonMap("query", ppl));
+    request.setJsonEntity(body);
     Response response = client().performRequest(request);
     return assertOkAndParse(response, "PPL: " + ppl);
 }
Suggestion importance[1-10]: 4

__

Why: The concern about escapeJson potentially missing edge cases is valid, but escapeJson is likely already implemented correctly in the base class AnalyticsRestTestCase. The suggestion to use ObjectMapper is a reasonable improvement for robustness, but the risk is low given the controlled test query strings.

Low
General
Fix thread-safety of dataset provisioning flag

The dataProvisioned static flag is not thread-safe. If tests run in parallel,
multiple threads could simultaneously pass the if check and provision the dataset
multiple times, potentially causing test failures or data corruption. Consider using
a volatile keyword or an AtomicBoolean to ensure visibility and atomicity.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [49-61]

-private static boolean dataProvisioned = false;
+private static volatile boolean dataProvisioned = false;
 
 private void ensureDataProvisioned() throws IOException {
     if (dataProvisioned == false) {
-        DatasetProvisioner.provision(client(), DATASET);
-        dataProvisioned = true;
+        synchronized (WhereCommandIT.class) {
+            if (dataProvisioned == false) {
+                DatasetProvisioner.provision(client(), DATASET);
+                dataProvisioned = true;
+            }
+        }
     }
 }
Suggestion importance[1-10]: 3

__

Why: Integration tests in this framework (extending AnalyticsRestTestCase) typically run sequentially, not in parallel, making thread-safety a low-priority concern. The suggestion is technically valid but has minimal practical impact in this test context.

Low
Correct misleading comment about remaining row categories

The comment states "9 TECHNOLOGY rows" but the total dataset has 17 rows, with 2
FURNITURE and 6 OFFICE SUPPLIES rows, leaving 9 rows. However, the comment assumes
all remaining rows are TECHNOLOGY, which may not be accurate if the dataset contains
other categories. The expected count of 9 should be verified against the actual
dataset to avoid a misleading or incorrect assertion.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [184-190]

 public void testWhereNotIn() throws IOException {
-    // Complement of (FURNITURE, OFFICE SUPPLIES): 9 TECHNOLOGY rows.
+    // 17 total - 2 FURNITURE - 6 OFFICE SUPPLIES = 9 remaining rows.
     assertRowCount(
         "source=" + DATASET.indexName + " | where not str0 in ('FURNITURE', 'OFFICE SUPPLIES') | fields str0",
         9
     );
 }
Suggestion importance[1-10]: 2

__

Why: This is a minor comment improvement — the existing code and assertion are functionally correct, and the change only affects the comment text. The improved_code doesn't change any logic, only the comment wording.

Low

Previous suggestions

Suggestions up to commit b0a1006
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate operand count before accessing operands

The guard original.getOperands().size() < 2 is checked after the early-return that
requires has3Args or isIlike, but a call with fewer than 2 operands could still
reach this point if isIlike is true. More importantly, the < 2 check should come
before any operand access to avoid potential IndexOutOfBoundsException. Move the
size validation to the top of the method.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/IlikeFunctionAdapter.java [61-69]

+if (original.getOperands().size() < 2) {
+    return original;
+}
 boolean isIlike = "ILIKE".equalsIgnoreCase(original.getOperator().getName());
 boolean has3Args = original.getOperands().size() >= 3;
 if (!isIlike && !has3Args) {
     // Plain 2-arg LIKE — substrait isthmus handles it natively.
     return original;
 }
-if (original.getOperands().size() < 2) {
-    return original;
-}
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that when isIlike is true but original.getOperands().size() < 2, the code would proceed past the first guard and then be caught by the second check — but moving the < 2 guard to the top is cleaner and prevents any potential IndexOutOfBoundsException earlier in the flow.

Low
Use proper JSON serialization for request body

The PPL query is embedded into a JSON string using simple string concatenation with
escapeJson, but if escapeJson doesn't properly handle all special characters
(backslashes, quotes, newlines), this could produce malformed JSON. Consider using a
proper JSON serialization library (e.g., Jackson's ObjectMapper) to build the
request body, ensuring correctness for all possible query strings.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [323-329]

 private Map<String, Object> executePpl(String ppl) throws IOException {
     ensureDataProvisioned();
     Request request = new Request("POST", "/_analytics/ppl");
-    request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
+    ObjectMapper mapper = new ObjectMapper();
+    String body = mapper.writeValueAsString(Map.of("query", ppl));
+    request.setJsonEntity(body);
     Response response = client().performRequest(request);
     return assertOkAndParse(response, "PPL: " + ppl);
 }
Suggestion importance[1-10]: 4

__

Why: While using a proper JSON serializer like ObjectMapper is more robust than string concatenation, the escapeJson helper is presumably already handling the necessary escaping for test queries. The improvement is valid but the risk is low given the controlled test context.

Low
General
Fix non-thread-safe static provisioning flag

Using a static non-volatile boolean flag for lazy provisioning is not thread-safe if
tests run in parallel. If multiple test threads check dataProvisioned concurrently,
the dataset could be provisioned multiple times or not at all. Consider using
AtomicBoolean or synchronization to guard this flag.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [49]

-private static boolean dataProvisioned = false;
+private static final java.util.concurrent.atomic.AtomicBoolean dataProvisioned = new java.util.concurrent.atomic.AtomicBoolean(false);
Suggestion importance[1-10]: 3

__

Why: The thread-safety concern with static boolean dataProvisioned is valid in theory, but integration tests in OpenSearch typically run single-threaded per class. The AtomicBoolean suggestion is a minor improvement with limited practical impact in this test context.

Low
Suggestions up to commit c49e765
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix potential index-out-of-bounds in operand access

The guard original.getOperands().size() < 2 is checked after the early-return that
requires at least 3 args or ILIKE, but if isIlike is true with fewer than 2
operands, the code will proceed to get(0) and get(1) and throw an
IndexOutOfBoundsException. The size check should be moved before the ILIKE/3-arg
check to protect all code paths.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/IlikeFunctionAdapter.java [61-69]

 boolean isIlike = "ILIKE".equalsIgnoreCase(original.getOperator().getName());
 boolean has3Args = original.getOperands().size() >= 3;
+if (original.getOperands().size() < 2) {
+    return original;
+}
 if (!isIlike && !has3Args) {
     // Plain 2-arg LIKE — substrait isthmus handles it natively.
     return original;
 }
-if (original.getOperands().size() < 2) {
-    return original;
-}
Suggestion importance[1-10]: 7

__

Why: This is a valid bug: if isIlike is true but original.getOperands().size() < 2, the code skips the size guard and proceeds to call get(0) and get(1), causing an IndexOutOfBoundsException. Moving the size check before the isIlike/has3Args check fixes all code paths.

Medium
General
Use safe JSON serialization for request body

The PPL query string is embedded directly into a JSON string using string
concatenation, relying on escapeJson to sanitize it. If escapeJson does not properly
escape all special JSON characters (e.g., newlines, tabs, backslashes), this could
produce malformed JSON. Consider using a proper JSON serialization library (e.g.,
Jackson's ObjectMapper) to build the request body safely.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [323-329]

 private Map<String, Object> executePpl(String ppl) throws IOException {
     ensureDataProvisioned();
     Request request = new Request("POST", "/_analytics/ppl");
-    request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
+    ObjectMapper mapper = new ObjectMapper();
+    String body = mapper.writeValueAsString(Map.of("query", ppl));
+    request.setJsonEntity(body);
     Response response = client().performRequest(request);
     return assertOkAndParse(response, "PPL: " + ppl);
 }
Suggestion importance[1-10]: 4

__

Why: While using a proper JSON serializer like ObjectMapper is generally safer than string concatenation, the existing escapeJson helper is likely sufficient for the test queries used here. The improvement is valid but has low practical impact in this test context.

Low
Use thread-safe flag for dataset provisioning

Using a static non-volatile boolean for dataProvisioned is not thread-safe if tests
run in parallel. If the test framework runs tests concurrently, multiple threads
could simultaneously observe dataProvisioned == false and attempt to provision the
dataset. Consider using AtomicBoolean or a synchronized block to ensure safe lazy
initialization.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/WhereCommandIT.java [49]

-private static boolean dataProvisioned = false;
+private static final java.util.concurrent.atomic.AtomicBoolean dataProvisioned = new java.util.concurrent.atomic.AtomicBoolean(false);
Suggestion importance[1-10]: 3

__

Why: While using AtomicBoolean is more thread-safe, integration tests in this framework typically run sequentially, making this a low-priority concern. The improved_code also doesn't update the usage in ensureDataProvisioned to call compareAndSet, making the suggestion incomplete.

Low

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b0a1006

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

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

ahkcs added 2 commits May 6, 2026 10:32
@ahkcs
Wire DataFusion backend for the PPL where command
3cf4730 
Three orthogonal compatibility gaps surface when CalciteWhereCommandIT runs
through the analytics-engine route. Address them in the DataFusion backend:

1. CAST as a project capability. ReduceExpressionsRule.ProjectReduceExpressionsRule
   constant-folds field references through equality filters into typed
   literals — `where str0 = 'FURNITURE' | fields str0` becomes
   `Project[CAST('FURNITURE' AS VARCHAR)]`. Without CAST in
   STANDARD_PROJECT_OPS, OpenSearchProjectRule throws "No backend supports
   scalar function [CAST]". 5 where-command tests previously broken;
   substrait isthmus and DataFusion handle CAST natively.

2. Comparison ops as project capabilities. PPL `eval x = (a == b)`
   produces a LogicalProject whose projected expression is the comparison
   itself (returning boolean). EQUALS/NOT_EQUALS/GT/GE/LT/LE were declared
   filter-only; add them to STANDARD_PROJECT_OPS so projections of boolean
   expressions are accepted. Fixes testDoubleEqualInEvalCommand.

3. ILIKE adapter. Substrait isthmus only knows the standard SQL LIKE
   operator; the Calcite-specific ILIKE has no extension. PPL emits ILIKE
   for the `contains` operator and for `LIKE` when
   plugins.ppl.syntax.legacy.preferred is true (its default), affecting
   six where-command tests with "Unable to convert call ILIKE(...)".
   Additionally, PPLFuncImpTable always passes an explicit '\\' escape
   literal of type CHAR(1), which substrait's like signature rejects as
   "Unable to convert call LIKE(string?, char<N>, char<1>)".

   The new IlikeFunctionAdapter handles both: ILIKE rewrites to
   LIKE(LOWER(field), LOWER(pattern)) — wildcards and the escape character
   survive Character.toLowerCase unchanged, preserving case-insensitive
   semantics — and the 3-arg form is reduced to 2-arg by dropping the
   default-only escape, since PPL never emits a non-default escape (the
   contains operator pre-escapes user-provided literals before composing
   the wildcard pattern). Both LOWER and 2-arg LIKE are natively supported
   by DataFusion.

Registered for ScalarFunction.LIKE because Calcite's SqlLibraryOperators.ILIKE
shares SqlKind.LIKE with the standard LIKE operator and resolves to the
same adapter slot; the adapter checks the operator name to discriminate.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
Add WhereCommandIT QA test for the analytics-engine route
5f82f72 
Self-contained REST integration test under sandbox/qa/analytics-engine-rest
mirroring the surface exercised by CalciteWhereCommandIT in the SQL plugin,
adapted to the calcs dataset already shipped under
src/test/resources/datasets/calcs/.

Each test posts a PPL query through POST /_analytics/ppl (exposed by the
test-ppl-frontend plugin), exercising the same UnifiedQueryPlanner →
CalciteRelNodeVisitor → analytics-engine planner → Substrait → DataFusion
pipeline as the SQL plugin's force-routed analytics path. CI for OpenSearch
core can verify the where command end-to-end without checking out the SQL
plugin or relying on its IT classpath.

Coverage (26 cases, all passing):
  - Comparison operators (=, ==, !=, <, >, <=, >=)
  - Boolean connectives AND, OR, NOT
  - IS NULL / IS NOT NULL via isnull() / isnotnull()
  - IN / NOT IN against keyword and numeric columns
  - LIKE function and operator (with %  and _ wildcards)
  - contains (lowers to ILIKE — case-insensitive)
  - Sub-expression scalar calls inside predicates: length, abs, +

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
ahkcs force-pushed the feature/mustang-where-command branch from b0a1006 to 5f82f72 Compare May 6, 2026 17:34
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5f82f72

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 5f82f72: null

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

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 5f82f72: SUCCESS

@mch2
mch2 merged commit dcb68f9 into opensearch-project:main May 6, 2026
18 of 20 checks passed
@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.46%. Comparing base (144386c) to head (5f82f72).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #21502      +/-   ##
============================================
+ Coverage     73.38%   73.46%   +0.07%     
- Complexity    74380    74431      +51     
============================================
  Files          5970     5970              
  Lines        338267   338267              
  Branches      48753    48753              
============================================
+ Hits         248228   248498     +270     
+ Misses        70237    69912     -325     
- Partials      19802    19857      +55

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

imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request May 8, 2026
@ahkcs @imRishN
Wire PPL where command through the analytics-engine path (opensearch-…
7d1eb15 
…project#21502)

* Wire DataFusion backend for the PPL where command

Three orthogonal compatibility gaps surface when CalciteWhereCommandIT runs
through the analytics-engine route. Address them in the DataFusion backend:

1. CAST as a project capability. ReduceExpressionsRule.ProjectReduceExpressionsRule
   constant-folds field references through equality filters into typed
   literals — `where str0 = 'FURNITURE' | fields str0` becomes
   `Project[CAST('FURNITURE' AS VARCHAR)]`. Without CAST in
   STANDARD_PROJECT_OPS, OpenSearchProjectRule throws "No backend supports
   scalar function [CAST]". 5 where-command tests previously broken;
   substrait isthmus and DataFusion handle CAST natively.

2. Comparison ops as project capabilities. PPL `eval x = (a == b)`
   produces a LogicalProject whose projected expression is the comparison
   itself (returning boolean). EQUALS/NOT_EQUALS/GT/GE/LT/LE were declared
   filter-only; add them to STANDARD_PROJECT_OPS so projections of boolean
   expressions are accepted. Fixes testDoubleEqualInEvalCommand.

3. ILIKE adapter. Substrait isthmus only knows the standard SQL LIKE
   operator; the Calcite-specific ILIKE has no extension. PPL emits ILIKE
   for the `contains` operator and for `LIKE` when
   plugins.ppl.syntax.legacy.preferred is true (its default), affecting
   six where-command tests with "Unable to convert call ILIKE(...)".
   Additionally, PPLFuncImpTable always passes an explicit '\\' escape
   literal of type CHAR(1), which substrait's like signature rejects as
   "Unable to convert call LIKE(string?, char<N>, char<1>)".

   The new IlikeFunctionAdapter handles both: ILIKE rewrites to
   LIKE(LOWER(field), LOWER(pattern)) — wildcards and the escape character
   survive Character.toLowerCase unchanged, preserving case-insensitive
   semantics — and the 3-arg form is reduced to 2-arg by dropping the
   default-only escape, since PPL never emits a non-default escape (the
   contains operator pre-escapes user-provided literals before composing
   the wildcard pattern). Both LOWER and 2-arg LIKE are natively supported
   by DataFusion.

Registered for ScalarFunction.LIKE because Calcite's SqlLibraryOperators.ILIKE
shares SqlKind.LIKE with the standard LIKE operator and resolves to the
same adapter slot; the adapter checks the operator name to discriminate.

Signed-off-by: Kai Huang <ahkcs@amazon.com>

* Add WhereCommandIT QA test for the analytics-engine route

Self-contained REST integration test under sandbox/qa/analytics-engine-rest
mirroring the surface exercised by CalciteWhereCommandIT in the SQL plugin,
adapted to the calcs dataset already shipped under
src/test/resources/datasets/calcs/.

Each test posts a PPL query through POST /_analytics/ppl (exposed by the
test-ppl-frontend plugin), exercising the same UnifiedQueryPlanner →
CalciteRelNodeVisitor → analytics-engine planner → Substrait → DataFusion
pipeline as the SQL plugin's force-routed analytics path. CI for OpenSearch
core can verify the where command end-to-end without checking out the SQL
plugin or relying on its IT classpath.

Coverage (26 cases, all passing):
  - Comparison operators (=, ==, !=, <, >, <=, >=)
  - Boolean connectives AND, OR, NOT
  - IS NULL / IS NOT NULL via isnull() / isnotnull()
  - IN / NOT IN against keyword and numeric columns
  - LIKE function and operator (with %  and _ wildcards)
  - contains (lowers to ILIKE — case-insensitive)
  - Sub-expression scalar calls inside predicates: length, abs, +

Signed-off-by: Kai Huang <ahkcs@amazon.com>

---------

Signed-off-by: Kai Huang <ahkcs@amazon.com>
vishwasgarg18 pushed a commit to vishwasgarg18/OpenSearch that referenced this pull request May 8, 2026
@ahkcs Vishwas Garg
Wire PPL where command through the analytics-engine path (opensearch-…
2d1f3ce 
…project#21502)

* Wire DataFusion backend for the PPL where command

Three orthogonal compatibility gaps surface when CalciteWhereCommandIT runs
through the analytics-engine route. Address them in the DataFusion backend:

1. CAST as a project capability. ReduceExpressionsRule.ProjectReduceExpressionsRule
   constant-folds field references through equality filters into typed
   literals — `where str0 = 'FURNITURE' | fields str0` becomes
   `Project[CAST('FURNITURE' AS VARCHAR)]`. Without CAST in
   STANDARD_PROJECT_OPS, OpenSearchProjectRule throws "No backend supports
   scalar function [CAST]". 5 where-command tests previously broken;
   substrait isthmus and DataFusion handle CAST natively.

2. Comparison ops as project capabilities. PPL `eval x = (a == b)`
   produces a LogicalProject whose projected expression is the comparison
   itself (returning boolean). EQUALS/NOT_EQUALS/GT/GE/LT/LE were declared
   filter-only; add them to STANDARD_PROJECT_OPS so projections of boolean
   expressions are accepted. Fixes testDoubleEqualInEvalCommand.

3. ILIKE adapter. Substrait isthmus only knows the standard SQL LIKE
   operator; the Calcite-specific ILIKE has no extension. PPL emits ILIKE
   for the `contains` operator and for `LIKE` when
   plugins.ppl.syntax.legacy.preferred is true (its default), affecting
   six where-command tests with "Unable to convert call ILIKE(...)".
   Additionally, PPLFuncImpTable always passes an explicit '\\' escape
   literal of type CHAR(1), which substrait's like signature rejects as
   "Unable to convert call LIKE(string?, char<N>, char<1>)".

   The new IlikeFunctionAdapter handles both: ILIKE rewrites to
   LIKE(LOWER(field), LOWER(pattern)) — wildcards and the escape character
   survive Character.toLowerCase unchanged, preserving case-insensitive
   semantics — and the 3-arg form is reduced to 2-arg by dropping the
   default-only escape, since PPL never emits a non-default escape (the
   contains operator pre-escapes user-provided literals before composing
   the wildcard pattern). Both LOWER and 2-arg LIKE are natively supported
   by DataFusion.

Registered for ScalarFunction.LIKE because Calcite's SqlLibraryOperators.ILIKE
shares SqlKind.LIKE with the standard LIKE operator and resolves to the
same adapter slot; the adapter checks the operator name to discriminate.

Signed-off-by: Kai Huang <ahkcs@amazon.com>

* Add WhereCommandIT QA test for the analytics-engine route

Self-contained REST integration test under sandbox/qa/analytics-engine-rest
mirroring the surface exercised by CalciteWhereCommandIT in the SQL plugin,
adapted to the calcs dataset already shipped under
src/test/resources/datasets/calcs/.

Each test posts a PPL query through POST /_analytics/ppl (exposed by the
test-ppl-frontend plugin), exercising the same UnifiedQueryPlanner →
CalciteRelNodeVisitor → analytics-engine planner → Substrait → DataFusion
pipeline as the SQL plugin's force-routed analytics path. CI for OpenSearch
core can verify the where command end-to-end without checking out the SQL
plugin or relying on its IT classpath.

Coverage (26 cases, all passing):
  - Comparison operators (=, ==, !=, <, >, <=, >=)
  - Boolean connectives AND, OR, NOT
  - IS NULL / IS NOT NULL via isnull() / isnotnull()
  - IN / NOT IN against keyword and numeric columns
  - LIKE function and operator (with %  and _ wildcards)
  - contains (lowers to ILIKE — case-insensitive)
  - Sub-expression scalar calls inside predicates: length, abs, +

Signed-off-by: Kai Huang <ahkcs@amazon.com>

---------

Signed-off-by: Kai Huang <ahkcs@amazon.com>
This was referenced Jun 16, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mch2 mch2 mch2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahkcs @mch2