[QA] Add AppendPipeCommandIT and TableCommandIT for the analytics-engine REST path

[ahkcs]

Summary

Lands two QA ITs against the analytics-engine REST path so PPL appendpipe and table are verified inside core without cross-plugin dependencies on the SQL plugin. Both reuse the existing calcs parquet-backed dataset via DatasetProvisioner; no new fixtures.

AppendPipeCommandIT — 3 tests

appendpipe already passes 4/4 of its v2-side CalcitePPLAppendPipeCommandIT cases on the analytics-engine route under tests.analytics.force_routing=true. The existing capability surface (LogicalUnion + LogicalAggregate over SUM + SchemaUnifier type-conflict) is sufficient — no code changes in core for this command.

Test	Shape	Notes
testAppendPipeSort	Duplicate post-stats stream, re-sort the duplicate inline	Exercises Union over identical schemas. The outer sort str0 and the inner sort -sum_int0_by_str0 pin both branches' orders, so positional row comparison works.
testAppendPipeWithMergedColumn	Duplicate post-stats stream, collapse via inner stats sum(sum) as sum	Exercises SchemaUnifier merging the str0-bearing original branch with the inner-branch single-row that has only sum. Branch arrival order at the coordinator's Union is non-deterministic, so multiset comparison.
testAppendPipeWithConflictTypeColumn	Inner pipeline rewrites same-named column to different type	Exercises the SchemaUnifier validation path that surfaces "due to incompatible types" before execution.

testAppendDifferentIndex from the v2-side IT is intentionally not ported: it exercises append (separate source=... sub-search), which is already covered by AppendCommandIT.

TableCommandIT — 5 tests + 1 small fix in test-ppl-frontend

table is a syntactic alias of fields — the v2 AstBuilder dispatches both through buildProjectCommand once plugins.calcite.enabled=true is visible to the AstBuilder via the UnifiedQueryContext. The mirror fix on the SQL plugin side landed in opensearch-project/sql#5413; this PR closes the same gap on the test-ppl-frontend side, where the UnifiedQueryContext is constructed locally rather than coming from the SQL plugin.

Two changes:

• UnifiedQueryService now sets plugins.calcite.enabled=true on the context. The unified path is Calcite-based by definition; without this flag the AstBuilder rejects table / regex / rex / convert.
• New TableCommandIT covering surfaces specific to the table keyword: comma-delimited, space-delimited, suffix wildcard, leading-- exclusion, and fields ↔ table row-and-schema equivalence on identical inputs. Plain projection semantics already covered by FieldsCommandIT are not duplicated.

Test	Shape
testTableCommaDelimited	table str0, num0 — same as fields a, b; sanity on the table keyword reaching buildProjectCommand.
testTableSpaceDelimited	table str0 num0 int0 — unique to table; lexer accepts whitespace as separator.
testTableSuffixWildcard	table *0 — wildcard expansion at parse time; analyzer-dependent column order, so set-equality.
testTableMinusExclusion	table - num0, num1, num2, num3, num4 — leading-- exclusion form; analytics path retains exclusion semantics.
testFieldsAndTableEquivalence	fields a, b, c vs table a, b, c — identical schema and rows; alias claim made explicit at the response level.

Test plan

• ./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*AppendPipeCommandIT" — 3 / 3 green
• ./gradlew :sandbox:qa:analytics-engine-rest:integTest -Dsandbox.enabled=true --tests "*TableCommandIT" — 5 / 5 green
• SQL-plugin-side CalcitePPLAppendPipeCommandIT against this branch with -Dtests.analytics.force_routing=true -Dtests.analytics.parquet_indices=true — 4 / 4 (verified before this PR — analytics path was already fine, no code changes needed in core)

Out of scope

PPL multisearch was originally bundled with this PR. Triage showed ~12 of its 15 analytics-route failures are blocked on a Substrait-side issue: DataFusion's substrait consumer rejects the Plan emitted for LogicalUnion(StageInputScan, StageInputScan) with

Substrait error: Names list must match exactly to nested schema, but found 2 uses for 12 names
  at NativeBridge.executeLocalPlan(...)
  at DatafusionReduceSink.<init>(...)
  at DataFusionAnalyticsBackendPlugin.lambda$getExchangeSinkProvider$0(...)
  at LocalStageScheduler.createExecution(...)

The registered child-stage Arrow schema width vs. the Plan.Root.names width disagree somewhere between LocalStageScheduler.buildChildInputs and DataFusion's make_renamed_schema. Diagnostic logging confirms we send 12 names + a Substrait set rel with two read.named_table inputs whose base_schema each has 12 fields — DataFusion's make_renamed_schema only consumes 2 of the 12 names against the deserialized Union output schema, suggesting the registered partition table for input-<stageId> is exposing a 2-field schema instead of the expected 12-field one. Root cause not yet identified — out of scope here; tracked for a follow-up PR.

Other categories are kept out of scope by the same scope discipline as #21521:

• Aggregation surface (AVG Substrait-isthmus binding)
• TIMESTAMP / DATE type-system (ArrowSchemaFromCalcite.toArrowType doesn't handle TIMESTAMP / DATE / TIME / TIMESTAMP_WITH_LOCAL_TIME_ZONE)
• Eval-predicate scalars (AND/OR/NOT/IS_NULL/etc. in CASE projections)
• PPL span time-bucketing

Each is its own focused PR.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 94245b8)

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 AppendPipeCommandIT and TableCommandIT integration tests Relevant files: sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TableCommandIT.java Sub-PR theme: Enable plugins.calcite.enabled in UnifiedQueryContext for table command support Relevant files: sandbox/plugins/test-ppl-frontend/src/main/java/org/opensearch/ppl/action/UnifiedQueryService.java

⚡ Recommended focus areas for review Static State Risk The dataProvisioned static boolean flag is shared across test instances. If tests run in parallel or the test class is reused across JVM forks, this flag may not reset correctly, causing tests to run against an unprovisioned dataset or skip provisioning when it's needed. Consider using a @BeforeClass / @Before setup method or a proper synchronization mechanism instead. private static boolean dataProvisioned = false; private void ensureDataProvisioned() throws IOException { if (dataProvisioned == false) { DatasetProvisioner.provision(client(), DATASET); dataProvisioned = true; } } JSON Injection Risk The escapeJson helper (inherited from base class) is used to build the JSON body for PPL queries. If escapeJson does not properly escape all special characters, a crafted PPL string could break the JSON structure. This is especially relevant since PPL queries are constructed via string concatenation with DATASET.indexName and user-controlled-like values. Request request = new Request("POST", "/_analytics/ppl"); request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); Response response = client().performRequest(request); return assertOkAndParse(response, "PPL: " + ppl); Incomplete Column Assertion In testTableSuffixWildcard, the response's columns field is cast to List<String> but the actual response may return a list of column descriptor maps (e.g., {name: "str0", type: "keyword"}) rather than plain strings, depending on the analytics-engine response format. This could cause a silent ClassCastException at runtime or incorrect set comparison. @SuppressWarnings("unchecked") List<String> columns = (List<String>) response.get("columns"); assertNotNull("Response missing 'columns'", columns); java.util.Set<String> actual = new java.util.HashSet<>(columns); java.util.Set<String> expected = new java.util.HashSet<>( java.util.Arrays.asList("num0", "str0", "int0", "bool0", "date0", "time0", "datetime0") ); assertEquals("Wildcard *0 column set", expected, actual); Hardcoded Setting Setting plugins.calcite.enabled to true unconditionally in the UnifiedQueryContext builder may override cluster-level or node-level configuration, potentially causing unexpected behavior in environments where Calcite is intentionally disabled. Consider reading the actual cluster setting and only applying this override when appropriate, or documenting the intentional override more prominently. // The unified PPL parser reuses the v2 AstBuilder, which gates Calcite-only // commands (table, regex, rex, convert) on plugins.calcite.enabled. The unified // path is by definition Calcite-based — flag it on so those commands lower // through the same Project/Filter RelNodes as their non-aliased counterparts. .setting("plugins.calcite.enabled", true)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 94245b8
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix potential ClassCastException on column extraction The columns field in the response may contain column name objects (e.g., Map<String, Object> with a "name" key) rather than plain String values, depending on the API response format. Casting directly to List could cause a ClassCastException at runtime. Verify the actual response structure and extract column names accordingly. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/TableCommandIT.java [79-81] -List<String> columns = (List<String>) response.get("columns"); -assertNotNull("Response missing 'columns'", columns); -java.util.Set<String> actual = new java.util.HashSet<>(columns); +List<Map<String, Object>> columnObjs = (List<Map<String, Object>>) response.get("columns"); +assertNotNull("Response missing 'columns'", columnObjs); +java.util.Set<String> actual = columnObjs.stream() + .map(col -> (String) col.get("name")) + .collect(java.util.stream.Collectors.toSet()); Suggestion importance[1-10]: 5 __ Why: This is a valid concern — if the API returns column objects with a "name" key rather than plain strings, the cast to List<String> would fail at runtime. However, the same pattern is used in assertColumns without being flagged, and the actual response format depends on the specific API implementation which may already return plain strings.	Low
	Prevent race condition in static flag Using a non-volatile static boolean for cross-test state in a multi-threaded test environment can cause race conditions where data provisioning is skipped or attempted multiple times. Mark the field as volatile to ensure visibility across threads. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java [45] -private static boolean dataProvisioned = false; +private static volatile boolean dataProvisioned = false; Suggestion importance[1-10]: 4 __ Why: While volatile is a valid concern for multi-threaded environments, integration tests typically run sequentially in a single thread, making this a low-impact suggestion. The same pattern is used in TableCommandIT without being flagged there.	Low
General	Ensure safe JSON construction for requests The same raw string JSON construction pattern is used in both AppendPipeCommandIT and TableCommandIT. If escapeJson does not properly handle all special characters (e.g., backslashes, control characters), the resulting JSON will be malformed and the request will fail with a cryptic error. Ensure escapeJson is robust, or use a proper JSON serializer to build the request body. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java [212] -request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}"); +request.setJsonEntity(String.format("{\"query\": \"%s\"}", escapeJson(ppl))); Suggestion importance[1-10]: 1 __ Why: The improved_code uses String.format which is functionally identical to the existing string concatenation — it does not address the actual concern about escapeJson robustness. The suggestion doesn't meaningfully improve the code.	Low

---

Previous suggestions

Suggestions up to commit 55f4cae

Category	Suggestion	Impact
General	Propagate IOException from provisioning in error-check test testAppendPipeSort calls assertRows which internally calls executePpl → ensureDataProvisioned, but testAppendPipeWithConflictTypeColumn calls assertErrorContains which also calls executePpl → ensureDataProvisioned. However, testAppendPipeWithConflictTypeColumn does not declare throws IOException, so if ensureDataProvisioned throws an IOException during provisioning inside executePpl, it will be caught by the outer catch (IOException e) block and silently converted to a fail(), masking provisioning errors as test failures rather than errors. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java [98-111] -public void testAppendPipeSort() throws IOException { - assertRows( +public void testAppendPipeWithConflictTypeColumn() throws IOException { + assertErrorContains( "source=" + DATASET.indexName - ... + + " | stats sum(int0) as sum by str0 | sort str0" + + " | appendpipe [ eval sum = cast(sum as double) ]" + + " | head 5", + "due to incompatible types" ); } Suggestion importance[1-10]: 5 __ Why: The observation is valid — a provisioning IOException inside assertErrorContains would be silently swallowed by the catch (IOException e) block and converted to a fail(), masking the real error. Adding throws IOException to testAppendPipeWithConflictTypeColumn would improve error visibility.	Low
	Use safe JSON serialization for request body The PPL query is embedded directly into a JSON string using string concatenation. If escapeJson does not properly escape all special characters (e.g., backslashes, control characters), this could produce malformed JSON. Consider using a proper JSON serialization library to build the request body safely. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java [209-215] 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 to safely serialize the query value + org.opensearch.common.xcontent.XContentBuilder builder = + org.opensearch.common.xcontent.XContentFactory.jsonBuilder(); + builder.startObject().field("query", ppl).endObject(); + request.setJsonEntity(builder.toString()); Response response = client().performRequest(request); return assertOkAndParse(response, "PPL: " + ppl); } Suggestion importance[1-10]: 4 __ Why: The concern about escapeJson not properly handling all special characters is valid, but the suggestion introduces a dependency on XContentFactory without verifying it's available, and the improved_code adds a comment about using a JSON library while still using a non-standard approach. The risk depends on the implementation of escapeJson in the base class.	Low
Possible issue	Fix thread-safety of static provisioning flag The static dataProvisioned flag is not thread-safe and can cause race conditions when tests run in parallel. Use a volatile modifier or an AtomicBoolean to ensure visibility across threads and prevent double-provisioning. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/AppendPipeCommandIT.java [45-52] -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 (AppendPipeCommandIT.class) { + if (dataProvisioned == false) { + DatasetProvisioner.provision(client(), DATASET); + dataProvisioned = true; + } + } } } Suggestion importance[1-10]: 4 __ Why: While the thread-safety concern is valid in general, integration tests in OpenSearch typically run sequentially within a single JVM thread, making this a low-priority issue. The double-checked locking pattern suggested is correct but likely unnecessary for this test context.	Low

[github-actions]

Persistent review updated to latest commit 94245b8

[github-actions]

✅ Gradle check result for 94245b8: SUCCESS

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21526      +/-   ##
============================================
- Coverage     73.44%   73.41%   -0.04%     
+ Complexity    74426    74396      -30     
============================================
  Files          5970     5970              
  Lines        338267   338267              
  Branches      48753    48753              
============================================
- Hits         248451   248345     -106     
- Misses        70042    70108      +66     
- Partials      19774    19814      +40     

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