Wire DataFusion backend for PPL fields/rename/head/sort + QA ITs

[ahkcs]

Summary

Bucket-1 capability-registry expansion for the analytics-engine route, scoped strictly to the PPL fields / rename / head / sort commands. No aggregation-side work in this PR — AVG / COUNT / SUM / STDDEV / VAR and the surface that depends on them (stats, count(eval(...)), etc.) belong to a separate work track and stay broken on the analytics path until that lands.

After this PR (with #21498's eval-side surface already on main):

• CalciteFieldsCommandIT, CalciteRenameCommandIT, CalciteHeadCommandIT 100% green on the analytics path under tests.analytics.force_routing=true
• CalciteSortCommandIT and CalcitePPLSortIT pick up the cast / abs / substring push-down failures (CAST is already in STANDARD_PROJECT_OPS upstream from Add 3 different types of PPL scalar functions to analytics-engine - prove wiring based on DataFusion capabilities #21476; ABS and SUBSTRING are this PR's contribution)
• CalcitePPLRenameIT 20 / 22 (the 12 column-order asserts are fixed by the companion opensearch-project/sql#5413; the 2 remaining failures are aggregation-blocked)

Changes

1. DataFusionAnalyticsBackendPlugin — STANDARD_PROJECT_OPS += ABS, SUBSTRING.
PPL sort push-down lifts an expression like abs(num0) or substring(str0, 1, 3) into a LogicalProject child of the sort, which is what the project rule's capability check sees. DataFusion has both natively; isthmus' default extension catalog already binds them. Without this, the analytics planner rejects the projection with No backend supports scalar function [ABS] among [datafusion].

2. QA ITs in sandbox/qa/analytics-engine-rest — one per command, each self-contained and provisioning the existing calcs parquet-backed dataset via DatasetProvisioner. Tests fire through POST /_analytics/ppl so the core build can validate the analytics-engine path without the SQL plugin. Mirror the failing surface in Calcite{Fields,Rename,Head,Sort}CommandIT:

IT	Tests	Coverage
FieldsCommandIT	5	basic projection, single-column, explicit order, suffix-wildcard *0 (set-equality — wildcard expansion order isn't part of the contract), and fields - num* exclusion
RenameCommandIT	4	single rename, multi-rename, post-rename reference fails with "not found", backtick-quoted target names
HeadCommandIT	5	default-10 cap, explicit count, count > total rows, head N from M offset, value-equality on first 5 rows
SortCommandIT	5	plain ASC/DESC by integer (with calcs' 6 null int0 entries placed per Calcite's nulls-first/last defaults), eval n = abs(num0) | sort n (9 null + 8 non-null abs values), eval s = substring(str2, 1, 3) | sort s (validates SUBSTRING capability end-to-end against the 17-row calcs dataset)

SQL-plugin Calcite IT status (analytics-engine route, tests.analytics.force_routing=true)

Companion test fixes in opensearch-project/sql#5413. Routing verified end-to-end: 348 analytics-engine PlannerImpl entries, 0 v2 PPLService.Incoming entries during the 7-IT sweep.

IT	Pass	Notes
CalciteFieldsCommandIT	39 / 39	✅ scope of this PR
CalciteRenameCommandIT	2 / 2	✅ scope of this PR
CalciteHeadCommandIT	4 / 4 (2 skipped)	✅ scope of this PR
CalcitePPLRenameIT	20 / 22	12 column-order asserts fixed by sql#5413's column-name-aware row matcher; 2 remaining are testRenameInAgg / testRenameWithBackticksInAgg — out of scope (aggregation work)
CalcitePPLSortIT	14 / 18	4 fail: tie-break ordering (parquet vs Lucene return equal sort keys in different stable position) and date-format diff (2017-10-23T00:00 vs 2017-10-23 00:00:00). Output-shape diffs that need their own follow-up PRs (date-format alignment, schema-position-aware row matcher); not aggregation-blocked, just outside this PR's scope
CalciteSortCommandIT	26 / 30	4 fail: weblogs index has IP-type fields the analytics-engine planner doesn't yet support scanning. Storage-engine gap, not aggregation, but still outside this PR's scope

Out of scope — explicit list

This PR does not cover any of the following surfaces. Each is tracked as a separate work track:

1. Aggregation surface (AVG / SUM / COUNT / STDDEV_* / VAR_*). The Substrait isthmus default AggregateFunctionConverter doesn't bind the SQL plugin's custom NullableSqlAvgAggFunction (it extends SqlAggFunction, not SqlAvgAggFunction, so isthmus' AggregateFunctions.toSubstraitAggVariant translator skips it and the lookup falls through to a non-existent map key → Unable to find binding for call AVG($N)). Resolving this is a separate cross-plugin design discussion: either rebase the SQL plugin's nullable agg variants on SqlAvgAggFunction directly, or add an additionalAggregateSigs SPI hook to BackendCapabilityProvider so backends can declare custom-class → substrait-name mappings. Out of scope for this PR; blocks the 2 remaining CalcitePPLRenameIT failures, all of CalcitePPLAggregationIT, and the stats/count(eval(...)) tail of every other IT.
2. Window functions. dedup lowers to ROW_NUMBER OVER, which RexOver doesn't recognize via ScalarFunction.fromSqlKind. Blocks CalciteDedupCommandIT and CalcitePPLDedupIT.
3. Advanced aggregates / PPL functions. first, last, take, arg_max, percentile_approx, distinct_count_approx, PPL span. Each needs a new AggregateFunction enum constant plus a DataFusion adapter or YAML extension.
4. Date-format alignment / schema-position-aware row matcher / IP-type scan. The 4 + 4 failures in CalcitePPLSortIT / CalciteSortCommandIT. Independent fixes, each meriting its own PR.

Test plan

• ./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*FieldsCommandIT" --tests "*RenameCommandIT" --tests "*HeadCommandIT" --tests "*SortCommandIT" — 19 / 19 green.
• ./gradlew check -p sandbox -Dsandbox.enabled=true — green (the unrelated ScalarDateTimeFunctionIT.testConvertTz flake from a stale local libopensearch_native.dylib resolved by rebuilding the Rust crate; not caused by this PR — convert_tz UDF was added by Add 3 different types of PPL scalar functions to analytics-engine - prove wiring based on DataFusion capabilities #21476 after my last cargo build).
• SQL-plugin Calcite ITs against this branch + companion opensearch-project/sql#5413, with -Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true: see the per-IT table above.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 89a2358)

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 ABS and SUBSTRING to DataFusion STANDARD_PROJECT_OPS Relevant files: sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java Sub-PR theme: Add QA integration tests for PPL fields/rename/head/sort analytics-engine route Relevant files: sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/FieldsCommandIT.java sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/HeadCommandIT.java sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/RenameCommandIT.java sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SortCommandIT.java

⚡ Recommended focus areas for review Shared Mutable State The dataProvisioned flag is a static boolean field. In JUnit-based test runners, static state persists across test class instances within the same JVM, which can cause provisioning to be skipped if tests are run in certain orders or re-run. The same pattern is repeated in HeadCommandIT, RenameCommandIT, and SortCommandIT. Consider using a @BeforeClass/@Before setup method or a proper once-per-suite mechanism instead. private static boolean dataProvisioned = false; private void ensureDataProvisioned() throws IOException { if (dataProvisioned == false) { DatasetProvisioner.provision(client(), DATASET); dataProvisioned = true; } } Off-by-one in Sort Validation In testSortBySubstringExpression, the alphabetical order check loop starts at index 5 (for (int i = 5; i < rows.size(); i++)) but the null block is only 4 rows (indices 0–3). Index 4 is skipped entirely, meaning the first non-null value is never compared against anything, and the transition from null to non-null at row 4 is not validated. The loop should start at i = 5 only if row 4 is also expected to be null, otherwise it should start at i = 4 (comparing row 4 vs row 3 would fail since row 3 is null). The intent seems to be to start at i = 5 to compare row 5 against row 4, but row 4 is the first non-null and is never checked for non-nullness. for (int i = 5; i < rows.size(); i++) { String prev = (String) rows.get(i - 1).get(0); String curr = (String) rows.get(i).get(0); assertNotNull("Non-null after null block", curr); assertTrue( "Sort order violation at row " + i + ": " + prev + " > " + curr, prev.compareTo(curr) <= 0 ); } Missing ensureDataProvisioned testFieldsSuffixWildcard and testFieldsExclusion call executePpl which calls ensureDataProvisioned, so provisioning is handled. However, assertColumns and assertRowsEqual helpers do not call ensureDataProvisioned directly — they rely on executePpl doing so. This is fine as long as executePpl is always the entry point, but the indirection makes it easy to accidentally bypass provisioning if a future test calls a helper directly without going through executePpl. private void assertColumns(String ppl, String... expectedColumns) throws IOException { Map<String, Object> response = executePpl(ppl); @SuppressWarnings("unchecked") List<String> columns = (List<String>) response.get("columns"); assertNotNull("Response missing 'columns' for query: " + ppl, columns); assertEquals( "Column count for query: " + ppl, expectedColumns.length, columns.size() ); for (int i = 0; i < expectedColumns.length; i++) { assertEquals( "Column at position " + i + " for query: " + ppl, expectedColumns[i], columns.get(i) ); } } Hardcoded Dataset Assumptions Tests like testSortAscByInt, testSortByAbsExpression, and testSortBySubstringExpression hardcode specific row counts and values (e.g., "17 rows", "9 nulls", specific double values) that are tightly coupled to the calcs dataset content. If the dataset changes, these tests will silently produce misleading failures. Consider adding a comment or assertion that validates the total row count upfront, or document the dataset version dependency more explicitly. public void testSortAscByInt() throws IOException { // int0 across the 17 calcs rows: [1, null, null, null, 7, 3, 8, null, null, 8, 4, 10, // null, 4, 11, 4, 8] — 6 nulls and 11 integers. Default sort is ASC nulls-first. assertRowsEqual( "source=" + DATASET.indexName + " | sort int0 | fields int0", row((Object) null), row((Object) null), row((Object) null), row((Object) null), row((Object) null), row((Object) null), row(1), row(3), row(4), row(4), row(4), row(7), row(8), row(8), row(8), row(10), row(11) ); } public void testSortDescByInt() throws IOException { // DESC nulls-last (the analytics path follows Calcite's default DESC = NULLS LAST). assertRowsEqual( "source=" + DATASET.indexName + " | sort -int0 | fields int0", row(11), row(10), row(8), row(8), row(8), row(7), row(4), row(4), row(4), row(3), row(1), row((Object) null), row((Object) null), row((Object) null), row((Object) null), row((Object) null), row((Object) null) ); } // ── push-down sort by scalar expression — exercises ABS / SUBSTRING capabilities ── public void testSortByAbsExpression() throws IOException { // `abs(num0)` lowers to ABS($N) inside a LogicalProject child of the sort. Without // ABS in STANDARD_PROJECT_OPS, the analytics planner rejects the projection with // "No backend supports scalar function [ABS] among [datafusion]". // // Calcs num0: [12.3, -12.3, 15.7, -15.7, 3.5, -3.5, 0, null, 10, null x8] — 9 nulls // and 8 non-nulls. abs(num0) preserves null and yields {0, 3.5, 3.5, 10, 12.3, 12.3, // 15.7, 15.7} for the non-null tail. Sorted ASC nulls-first puts the 9 nulls first. Map<String, Object> response = executePpl( "source=" + DATASET.indexName + " | eval n = abs(num0) | sort n | fields n | head 9" ); @SuppressWarnings("unchecked") List<List<Object>> rows = (List<List<Object>>) response.get("rows"); assertNotNull("Response missing 'rows'", rows); assertEquals("Row count", 9, rows.size()); for (int i = 0; i < 9; i++) { assertNull("Row " + i + " should be null", rows.get(i).get(0)); } } public void testSortByAbsTakesNonNullsFromTail() throws IOException { // Skip past the 9 nulls and verify the 8 non-null abs values appear in ASC order. Map<String, Object> response = executePpl( "source=" + DATASET.indexName + " | eval n = abs(num0) | sort n | fields n | head 8 from 9" ); @SuppressWarnings("unchecked") List<List<Object>> rows = (List<List<Object>>) response.get("rows"); assertNotNull("Response missing 'rows'", rows); assertEquals("Row count after 9 nulls", 8, rows.size()); double[] expectedSorted = { 0, 3.5, 3.5, 10, 12.3, 12.3, 15.7, 15.7 }; for (int i = 0; i < expectedSorted.length; i++) { Object v = rows.get(i).get(0); assertNotNull("Row " + i + " unexpectedly null", v); assertEquals( "abs(num0) sorted value at row " + i, expectedSorted[i], ((Number) v).doubleValue(), 1e-9 ); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 89a2358
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix off-by-one in sort order verification loop The alphabetical sort verification loop starts at index 5 instead of 4, skipping the first non-null value at index 4 from the comparison. Since nulls occupy rows 0–3, the first non-null is at row 4, and the loop comparing rows.get(i-1) to rows.get(i) should start at i = 5 but rows.get(4) (the first non-null) is never compared against rows.get(5). Actually the loop start is correct for comparisons, but the comment says "remaining 13" while the loop starts at 5 (skipping row 4 entirely from the ordering check). Change the loop to start at i = 4 with a guard, or start at i = 5 and verify row 4 is non-null separately, to ensure all 13 non-null rows are covered. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SortCommandIT.java [131-133] +// First 4 rows must be nulls (4 null str2 values in calcs). +for (int i = 0; i < 4; i++) { + assertNull("Expected null at row " + i + " (sorted ASC nulls-first)", rows.get(i).get(0)); +} +// The remaining 13 must be sorted alphabetically (rows 4..16). +assertNotNull("Row 4 should be non-null", rows.get(4).get(0)); for (int i = 5; i < rows.size(); i++) { String prev = (String) rows.get(i - 1).get(0); String curr = (String) rows.get(i).get(0); + assertNotNull("Non-null after null block", curr); + assertTrue( + "Sort order violation at row " + i + ": " + prev + " > " + curr, + prev.compareTo(curr) <= 0 + ); +} Suggestion importance[1-10]: 6 __ Why: The loop starts at i = 5 but the comment says "remaining 13" non-null rows (rows 4–16). Row 4 (the first non-null) is never verified to be in order relative to row 5. The suggestion correctly identifies this gap and adds an explicit non-null assertion for row 4, ensuring all 13 non-null rows are covered.	Low
General	Prevent silent masking of error body parse failures The assertErrorContains method silently falls back to e.getMessage() when parsing the response body fails, which may cause the assertTrue to pass even if the actual error message doesn't contain the expected substring (e.g., if getMessage() returns a generic HTTP error string). The fallback should at minimum log or rethrow to avoid masking a real assertion failure. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/RenameCommandIT.java [97-115] 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 (org.opensearch.client.ResponseException e) { String body; try { body = org.opensearch.test.rest.OpenSearchRestTestCase.entityAsMap(e.getResponse()).toString(); } catch (IOException ioe) { - body = e.getMessage(); + throw new AssertionError("Failed to parse error response body for query: " + ppl, ioe); } assertTrue( "Expected response body to contain [" + expectedSubstring + "] but was: " + body, body.contains(expectedSubstring) ); } catch (IOException e) { fail("Unexpected IOException: " + e); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that falling back to e.getMessage() could mask assertion failures if the response body can't be parsed. Throwing an AssertionError instead makes test failures more explicit and easier to diagnose.	Low
	Guard static provisioning flag against race conditions The executePpl method calls ensureDataProvisioned() which uses a static dataProvisioned flag. However, dataProvisioned is a static field but ensureDataProvisioned is an instance method, and the same pattern is repeated across FieldsCommandIT, HeadCommandIT, RenameCommandIT, and SortCommandIT. If tests run in parallel or the test framework creates multiple instances, the static flag may not be reliably set before use. Consider making dataProvisioned volatile or using @BeforeClass to ensure provisioning happens exactly once per class. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/FieldsCommandIT.java [155-161] -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); +private static volatile boolean dataProvisioned = false; + +private void ensureDataProvisioned() throws IOException { + if (dataProvisioned == false) { + synchronized (FieldsCommandIT.class) { + if (dataProvisioned == false) { + DatasetProvisioner.provision(client(), DATASET); + dataProvisioned = true; + } + } + } } Suggestion importance[1-10]: 3 __ Why: While the suggestion about thread-safety is valid in general, integration test frameworks like OpenSearch's REST test runner typically run tests sequentially within a class, making this a low-priority concern. The synchronized block and volatile flag add complexity that may not be necessary in this context.	Low

---

Previous suggestions

Suggestions up to commit e4a522a

Category	Suggestion	Impact
General	Use dedicated factory method for approximate aggregates The APPROXIMATE case creates an AggregateCapability directly via constructor, bypassing any factory-method validation. If AggregateCapability has a dedicated factory method for approximate functions (similar to simple(), statistical(), and stateExpanding()), it should be used here for consistency and correctness. Using the raw constructor may skip important initialization or validation logic. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java [200] -case APPROXIMATE -> caps.add(new AggregateCapability(func, Set.of(type), formats)); +case APPROXIMATE -> caps.add(AggregateCapability.approximate(func, Set.of(type), formats)); Suggestion importance[1-10]: 5 __ Why: The suggestion is valid in principle — using a factory method like approximate() would be more consistent. However, the PR comment explicitly notes that APPROXIMATE uses new AggregateCapability(...) directly, suggesting there may not be a dedicated factory method. The suggestion assumes AggregateCapability.approximate() exists without evidence from the diff.	Low
	Avoid fragile reference equality for operator matching The check op == org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT uses reference equality, which will fail if Calcite ever wraps or decorates the operator instance. A more robust check would compare by operator name and kind, e.g., "||".equals(op.getName()), to avoid fragile identity comparison. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchProjectRule.java [167-169] -if (op == org.apache.calcite.sql.fun.SqlStdOperatorTable.CONCAT) { +if ("||".equals(op.getName())) { scalarFunc = ScalarFunction.CONCAT; } else if (op instanceof SqlFunction sqlFunction) { Suggestion importance[1-10]: 5 __ Why: The suggestion to use "||".equals(op.getName()) instead of reference equality is a reasonable robustness improvement. However, the PR comment explicitly explains that || is a SqlBinaryOperator (not a SqlFunction), and using reference equality against SqlStdOperatorTable.CONCAT is a common Calcite pattern that is generally reliable within a single Calcite version.	Low
	Correct category assignment for logical operators AND, OR, and NOT are logical operators, not comparison operators. Categorizing them under Category.COMPARISON is semantically incorrect and could cause issues if the category is used for filtering or routing logic elsewhere. They should use a Category.LOGICAL or equivalent category if available, or at minimum be documented as a deliberate workaround. sandbox/libs/analytics-framework/src/main/java/org/opensearch/analytics/spi/ScalarFunction.java [35-37] -AND(Category.COMPARISON, SqlKind.AND), -OR(Category.COMPARISON, SqlKind.OR), -NOT(Category.COMPARISON, SqlKind.NOT), +AND(Category.LOGICAL, SqlKind.AND), +OR(Category.LOGICAL, SqlKind.OR), +NOT(Category.LOGICAL, SqlKind.NOT), Suggestion importance[1-10]: 4 __ Why: The suggestion is semantically correct — AND, OR, NOT are logical, not comparison operators. However, the suggestion assumes Category.LOGICAL exists, which is not confirmed by the diff. The PR may be intentionally reusing Category.COMPARISON as the closest available category.	Low

[github-actions]

✅ Gradle check result for e4a522a: SUCCESS

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21521      +/-   ##
============================================
- Coverage     73.44%   73.44%   -0.01%     
+ Complexity    74426    74409      -17     
============================================
  Files          5970     5970              
  Lines        338267   338267              
  Branches      48753    48753              
============================================
- Hits         248451   248440      -11     
+ Misses        70042    69951      -91     
- Partials      19774    19876     +102     

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

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
sandbox/plugins/analytics-engine/build.gradle	117	high	New runtime dependency added: org.apache.commons:commons-text:1.11.0. Per mandatory supply chain policy, all dependency additions must be flagged regardless of apparent legitimacy. Maintainers should verify the artifact hash against the Apache Commons release and confirm this resolves the stated NoClassDefFoundError without introducing regressions (commons-text has historically carried critical CVEs, e.g., Text4Shell in versions prior to 1.10.0).

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | 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]

Persistent review updated to latest commit 89a2358

[github-actions]

❕ Gradle check result for 89a2358: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.