Carry CalciteEvalCommandIT through the helper-managed index path by ahkcs · Pull Request #5407 · opensearch-project/sql

ahkcs · 2026-05-05T22:57:11Z

Description

Two things this PR carries together (commit-separated for clean review):

Carry CalciteEvalCommandIT through the helper-managed index path (28a58536d) — provisions the inline test_eval index via TestUtils.createIndexByRestClient instead of direct PUT /test_eval/_doc/N, so the analytics-engine compatibility run (tests.analytics.parquet_indices=true) gets a parquet-backed index. Mirrors the helper-managed pattern that CalciteFillNullCommandIT and CalcitePPLAppendCommandIT already use via loadIndex(Index.X).
Workaround for Gradle 9.4.1 TestEventReporterAsListener cast bug (435827e99) — excludes @Ignore-annotated test classes from integTest so the task no longer aborts mid-run on this branch's CI.

1. Helper-managed `test_eval` provisioning

Why this matters

When the analytics-engine compatibility path is active (tests.analytics.force_routing=true and tests.analytics.parquet_indices=true), every test-created index needs parquet-backed storage so the DataFusion backend can scan it — TestUtils.makeParquetBacked handles the injection inside createIndexByRestClient. Direct PUT /test_eval/_doc/N calls bypass that helper, so the index lands Lucene-backed and the analytics planner rejects every query with No backend can scan all requested fields on index [test_eval].

The three tests that use test_eval fail on the analytics route today; only testEvalStringConcatenationWithExistingData (which uses loadIndex(Index.BANK)) survives. With this change, test_eval is created via the same helper as every other fixture and inherits the parquet flag automatically.

The companion analytics-engine wiring (CONCAT/CAST/SAFE_CAST capabilities + null-propagating concat adapter) lands separately in opensearch-project/OpenSearch#21498. On its own, this PR is a no-op for the v2 / Calcite path — once both this and the core PR are merged, CalciteEvalCommandIT goes from 1/4 to 4/4 on the analytics route.

Changes

Helper-managed test_eval provisioning. init() now calls TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping) with an explicit mapping (name/title = keyword, age = long). The helper is a thin wrapper around the index PUT on the v2 path (no-op when tests.analytics.parquet_indices is unset), so v2 behaviour is unchanged.
Idempotency guard. The whole init body is wrapped in TestUtils.isIndexExist(client(), "test_eval") — same pattern loadIndex uses for predefined fixtures. init() runs before every @Test method; first run provisions, subsequent runs skip. Without the guard, createIndexByRestClient (unlike the original doc-level PUT) would throw resource_already_exists_exception on the second invocation.
Pinned projection order on testEvalStringConcatenation. The original query had no | fields clause and relied on the implicit projection's column order, which differs between paths (v2 returns _source-iteration order, analytics returns parquet-storage order). Added | fields name, title, age, greeting to make the assertion deterministic. Expected rows already match this order, so v2 behaviour is preserved.

V2-path impact

Surface	Before	After
`name` PPL type	`string` (text → string)	`string` (keyword → string)
`age` PPL type	`bigint`	`bigint`
`title` PPL type	`string`	`string`
Eval read source	`_source`	`_source`
Concat result	`"Hello Alice"`	`"Hello Alice"`
Test 1 expected row order	`[name, title, age, greeting]` (matched by coincidence)	`[name, title, age, greeting]` (now pinned)

verifySchema(schema("name", "string")) matches both text+keyword (dynamic) and keyword (explicit) — PPL maps both to the same logical type. No assertion changes were needed.

2. Gradle 9.4.1 `@Ignore` cast workaround

Why this is in this PR

CI on this branch (post-#5406 wrapper bump to 9.4.1) aborts the :integ-test:integTest task with:

Test process encountered an unexpected problem.
> class org.gradle.api.internal.tasks.testing.LifecycleTrackingTestEventReporter cannot be cast to class
  org.gradle.api.internal.tasks.testing.GroupTestEventReporterInternal

…even though no test assertion fails. Without a workaround, this PR's tests would be invisible — the task crashes before final result aggregation. Folding the workaround into this PR unblocks our CI; the workaround is a base-branch concern but the symptom blocks every PR until landed.

Root cause (verified via `javap` against `gradle-testing-base-9.4.1.jar`)

org/gradle/api/tasks/testing/AbstractTestTask (line 263) instantiates TestEventReporterAsListener — Gradle's internal bridge from TestListenerInternal to the new TestEventReporter API.
TestEventReporterAsListener.started line 58 stores a per-descriptor reporter in a map keyed by id and, on each child event, casts the parent's stored reporter to GroupTestEventReporterInternal.
A class-level @Ignore produces a non-composite parent descriptor with a leaf TestEventReporter rather than a group reporter — the cast fails. First failure in CI: CalciteInformationSchemaCommandIT.testTablesFromPrometheusCatalog (that class is @Ignore'd for [FEATURE] Support Prometheus DataSource with Calcite #3455).
The bridge is instantiated by Gradle's own AbstractTestTask, not by OpenSearchTestBasePlugin.addTestListener(ErrorReportingTestListener). Migrating ErrorReportingTestListener to the new event-reporter API would not stop the bridge from firing.

Workaround

Exclude all @Ignore-annotated test classes from integTest. The classes were already skipped at the JUnit layer; this just moves the skip earlier (build layer) so the buggy bridge never sees them.

	Before	After
`@Ignore`'d test runtime status	Skipped (JUnit)	Not run (Gradle exclude)
Pass/fail outcome	Skipped	Skipped (effectively — same as not-run for CI gating)
Skipped-test count in dashboards	Includes `@Ignore`'d	One-time delta (excludes them)
`integTest` task aborts mid-run	❌ yes (cast crash)	✅ no
`testLogging.events "failed"` per-test message	Preserved	Preserved

24 classes excluded (one — OrderIT — was already excluded by the existing block). Source: grep -rln '^@Ignore' integ-test/src/test/java minus already-excluded paths.

This block should be removed once Gradle ships a fix for the bridge cast. Given the bug is in Gradle's own internal class, the proper long-term fix is upstream — file an issue on gradle/gradle referencing the TestEventReporterAsListener.started:58 site if not already filed.

Test results

	v2 path (this PR alone)	analytics route (this PR + core #21498)
`testEvalStringConcatenation`	passes	passes
`testEvalStringConcatenationWithNullField`	passes	passes
`testEvalStringConcatenationWithLiterals`	passes	passes
`testEvalStringConcatenationWithExistingData`	passes	passes
Analytics-route status	unchanged (1/4)	4/4
`:integ-test:integTest` task	crashes pre-PR (Gradle 9.4.1 bug)	runs to completion

Forward note

The same direct-PUT pattern that this PR fixes for CalciteEvalCommandIT exists in roughly a dozen other Calcite ITs (CalcitePPLAggregationIT, CalcitePPLBasicIT, CalciteFieldFormatCommandIT, CalcitePPLCaseFunctionIT, etc.). Each will need the same one-line carryover when its corresponding command lands on the analytics route. This PR addresses only the eval surface.

By submitting this pull request

All commits are signed per the DCO.
My code follows the code of conduct of this project.
My change requires changes to the documentation. — N/A (test-infra change)
I have updated the documentation accordingly. — N/A
I have added tests to cover my changes. — Existing tests; this PR carries them through the helper-managed path so they survive on the analytics route.

@test

The IT's init() previously created the in-test test_eval index via direct `PUT /test_eval/_doc/N` requests, relying on dynamic mapping. Two problems with that: 1. The doc PUTs auto-create the index with whatever settings the cluster defaults to. The analytics-engine compatibility path (force-routing on; tests.analytics.parquet_indices=true) needs parquet-backed indices, which TestUtils.createIndexByRestClient applies via TestUtils.makeParquetBacked when the system property is set. Direct PUTs sidestep that helper, so test_eval lands as Lucene-backed and the analytics planner rejects it with "No backend can scan all requested fields on index [test_eval]". Three of the four tests fail at execution; the fourth uses BANK (loadIndex'd) and passes. 2. init() runs before every @test method. The doc PUTs are doc-level idempotent, so re-running was wasteful but not failing. Once we switch to createIndexByRestClient, the index-level PUT is no longer idempotent and re-running throws "resource_already_exists_exception". Both addressed in one change: - test_eval is created via TestUtils.createIndexByRestClient with an explicit mapping (name/title=keyword, age=long). The helper honours tests.analytics.parquet_indices=true and produces a parquet-backed index for the analytics-engine sweep; on the v2 path the helper is a no-op around the index PUT, so behaviour is unchanged. - The whole init body is guarded by TestUtils.isIndexExist — same idempotency idiom that loadIndex uses for predefined fixtures. First @test method provisions; subsequent methods skip. Also pins the projection order on testEvalStringConcatenation. The original query (`source=test_eval | eval greeting = 'Hello ' + name`) had no `| fields` clause and relied on the implicit projection's column order — v2 returns Lucene-source insertion order, analytics returns parquet-storage order (alphabetical), so the assertion only matched on v2 by coincidence. Adding `| fields name, title, age, greeting` makes the assertion deterministic across paths; the existing expected rows (`rows("Alice", "Engineer", 25, "Hello Alice")`) already match this order, so v2 behaviour is preserved. No semantic change for the v2 path: the explicit mapping types (keyword, long, keyword) resolve to the same PPL types ("string", "bigint", "string") that dynamic mapping inferred, and eval reads from _source either way. Analytics-route compatibility goes from 1/4 to 4/4 once the corresponding analytics-engine wiring lands in core. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-05-05T22:58:30Z

PR Reviewer Guide 🔍

(Review updated until commit `435827e`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Idempotency Risk The `isIndexExist` guard prevents re-creating the index, but if a previous test run left the index in a partial or corrupt state (e.g., only the mapping was created but no documents were inserted), subsequent runs will skip document insertion entirely. Consider adding a cleanup/delete step before creation, or ensuring the index is deleted in a `@AfterAll`/teardown method. if (!TestUtils.isIndexExist(client(), "test_eval")) { String testEvalMapping = "{\"mappings\":{\"properties\":{" + "\"name\":{\"type\":\"keyword\"}," + "\"age\":{\"type\":\"long\"}," + "\"title\":{\"type\":\"keyword\"}}}}"; TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping); // Create test data for string concatenation Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true"); request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}"); client().performRequest(request1); Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true"); request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}"); client().performRequest(request2); Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true"); request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}"); client().performRequest(request3); } Null Age Handling Document 3 sets `"age": null` explicitly. Since the mapping defines `age` as `long`, inserting a null value may behave differently across Lucene and parquet-backed engines. Verify that both execution paths handle null long values consistently, especially in the `verifySchema` and `verifyDataRows` assertions downstream. Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true"); request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}"); client().performRequest(request3);

github-actions · 2026-05-05T22:58:47Z

PR Code Suggestions ✨

Latest suggestions up to 435827e
Explore these optional code suggestions:

Category Suggestion Impact

Possible issue

Guard condition may leave index empty on partial failure

The document PUT requests inside the if block do not use ?refresh=true in a way that
guarantees all three documents are visible before the test queries run, since each
refresh only covers that single document. More critically, if the index already
exists from a previous test run but is empty (e.g., due to a partial failure), the
guard isIndexExist will skip re-inserting the documents, leaving the index empty and
causing test failures. Consider separating the existence check for the index from
the data population, or deleting and recreating the index unconditionally in init().

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java [37-57]

-if (!TestUtils.isIndexExist(client(), "test_eval")) {
-  String testEvalMapping =
-      "{\"mappings\":{\"properties\":{"
-          + "\"name\":{\"type\":\"keyword\"},"
-          + "\"age\":{\"type\":\"long\"},"
-          + "\"title\":{\"type\":\"keyword\"}}}}";
-  TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping);
+TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping);
 
-  // Create test data for string concatenation
-  Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
-  request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
-  client().performRequest(request1);
+Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
+request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
+client().performRequest(request1);
 
-  Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
-  request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
-  client().performRequest(request2);
+Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
+request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
+client().performRequest(request2);
 
-  Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
-  request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
-  client().performRequest(request3);
-}
+Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
+request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
+client().performRequest(request3);

Suggestion importance[1-10]: 4

__

Why: The concern about partial failures leaving an empty index is valid but somewhat theoretical for integration tests that typically run in clean environments. The refresh=true on each individual PUT is actually sufficient for visibility since each document is immediately refreshed. The suggestion to remove the guard entirely contradicts the PR's explicit intent to support idempotent init() calls for the analytics-engine compatibility run, making the improved_code incomplete as it omits the mapping variable and index creation guard logic.

Low

Previous suggestions

Suggestions up to commit 28a5853

Category Suggestion Impact

General

Consolidate refresh after all document inserts

The documents are inserted without a final ?refresh=true barrier after all three
PUTs, but more critically, when the index already exists (the else branch is
skipped), the test proceeds without verifying that the expected documents are
actually present. If a previous test run created the index but failed
mid-population, subsequent runs will silently operate on incomplete data. Consider
adding a document count check or always ensuring all three documents exist
regardless of the index-existence guard.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java [37-57]

 if (!TestUtils.isIndexExist(client(), "test_eval")) {
   String testEvalMapping =
       "{\"mappings\":{\"properties\":{"
           + "\"name\":{\"type\":\"keyword\"},"
           + "\"age\":{\"type\":\"long\"},"
           + "\"title\":{\"type\":\"keyword\"}}}}";
   TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping);
 
-  // Create test data for string concatenation
-  Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
+  Request request1 = new Request("PUT", "/test_eval/_doc/1");
   request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
   client().performRequest(request1);
 
-  Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
+  Request request2 = new Request("PUT", "/test_eval/_doc/2");
   request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
   client().performRequest(request2);
 
-  Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
+  Request request3 = new Request("PUT", "/test_eval/_doc/3");
   request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
   client().performRequest(request3);
+
+  // Refresh once after all documents are indexed
+  client().performRequest(new Request("POST", "/test_eval/_refresh"));
 }

Suggestion importance[1-10]: 3

__

Why: The suggestion raises a valid concern about consolidating the refresh operation, but the improved_code actually removes ?refresh=true from individual requests and adds a single _refresh call at the end — this is a minor optimization. The concern about incomplete data if a previous run failed mid-population is valid but unlikely in practice, and the suggestion doesn't fully address that concern in the improved_code.

Low

@ignore

Gradle 9.4.1 has a ClassCastException in TestEventReporterAsListener.started (line 58 of org/gradle/api/internal/tasks/testing/junit/result/TestEventReporterAsListener.class inside plugins/gradle-testing-base-9.4.1.jar): ((GroupTestEventReporterInternal) reportersById.get(parent.getId())) .reportTestDirectly(...) The bridge stores test descriptor reporters keyed by id and casts the parent's stored reporter to GroupTestEventReporterInternal when a child event arrives. A class-level @ignore produces a non-composite parent descriptor with a leaf TestEventReporter (not a group reporter), so the cast fails. The first failure in CI was on CalciteInformationSchemaCommandIT.testTablesFromPrometheusCatalog — that IT is @ignore'd at the class level pointing at issue opensearch-project#3455. Once any @ignore'd class is loaded by the test runner, the cast aborts the whole integTest task with "Test process encountered an unexpected problem", even though no actual test assertion failed and the tests themselves would have been skipped at the JUnit layer. The bridge is instantiated by Gradle's own AbstractTestTask (verified via javap of `gradle-testing-base-9.4.1.jar` — `new TestEventReporterAsListener` at AbstractTestTask:263), so we cannot prevent its registration from user code. The only available remedy until Gradle ships a fix is to keep @ignore'd classes off the test runner's input set entirely. Net behaviour for CI: - @ignore'd tests were already skipped at the JUnit layer; skipping them at the Gradle layer instead changes "skipped" to "not run". For metrics/dashboards that count skipped tests, this is a one-time minor delta. For pass/fail status, identical. - Pre-existing exclude block (`OrderIT`, `ExplainIT`, etc.) already excludes some of the same classes; this commit covers the remaining @ignore'd ones. OrderIT not duplicated. - integTest task no longer aborts mid-run; per-test FAILED messages (testLogging.events "failed") are preserved. Remove this block once Gradle 9.4.x bug is fixed upstream. The list mirrors `grep -rln '^@ignore' integ-test/src/test/java` minus already-excluded paths. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-05-05T23:55:11Z

Persistent review updated to latest commit 435827e

@ignore

Mirrors the trailing portion of #5414 onto feature/mustang-ppl-integration. The Gradle 9.4.1 wrapper bump (#5406) and the integTest exclude block for @ignore'd classes (#5407) already landed on this branch, but the matching cleanup of `CalciteNoPushdownIT` did not — the @suite still listed four classes that are now build-time excluded from integTest: - CalciteInformationSchemaCommandIT - CalciteJsonFunctionsIT - CalcitePrometheusDataSourceCommandsIT - CalciteShowDataSourcesCommandIT Leaving them in the suite means the JUnit runner attempts to load each class, finds it referenced from the suite, and registers a descriptor — which (combined with the class-level @ignore) reproduces the same TestEventReporterAsListener.started cast that the integTest exclude was meant to avoid. Dropping the entries here keeps the no-pushdown suite consistent with the exclude block: a class excluded at the build layer must not also appear in any @Suite.SuiteClasses list. No new tests are introduced or removed — the four classes are already @ignore'd and have been since they were written. This change is purely about keeping the no-pushdown suite metadata aligned with the exclusion pattern carried over from main. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@test

Same shape as opensearch-project#5407 for CalciteEvalCommandIT. The IT's init() previously created the in-test test_eval index via direct `PUT /test_eval/_doc/N` requests, relying on dynamic mapping. Two problems: 1. The doc PUTs auto-create the index with whatever settings the cluster defaults to. The analytics-engine compatibility path (force-routing on; tests.analytics.parquet_indices=true) needs parquet-backed indices, which TestUtils.createIndexByRestClient applies via TestUtils.makeParquetBacked when the system property is set. Direct PUTs sidestep that helper, so test_eval lands as Lucene-backed and the analytics planner rejects it with "No backend can scan all requested fields on index [test_eval]". All four working tests fail at execution. 2. init() runs before every @test method. The doc PUTs are doc-level idempotent, so re-running was wasteful but not failing. Once we switch to createIndexByRestClient, the index-level PUT is no longer idempotent and re-running throws "resource_already_exists_exception". Both addressed in one change: - test_eval is created via TestUtils.createIndexByRestClient with an explicit mapping (name/title=keyword, age=long). The helper honours tests.analytics.parquet_indices=true and produces a parquet-backed index for the analytics-engine sweep; on the v2 path the helper is a no-op around the index PUT, so behaviour is unchanged. - The whole init body is guarded by TestUtils.isIndexExist — same idempotency idiom that loadIndex uses for predefined fixtures. First @test method provisions; subsequent methods skip. Also pins the projection order on testFieldFormatStringConcatenation. The original query (`source=test_eval | fieldformat greeting = 'Hello ' + name`) had no `| fields` clause and relied on the implicit projection's column order — v2 returns Lucene-source insertion order, analytics returns parquet-storage order (alphabetical), so the assertion only matched on v2 by coincidence. Adding `| fields name, title, age, greeting` makes the assertion deterministic across paths; the existing expected rows (`rows("Alice", "Engineer", 25, "Hello Alice")`) already match this order, so v2 behaviour is preserved. The other four tests already had explicit `| fields ...` clauses, so no change there. No semantic change for the v2 path: the explicit mapping types (keyword, long, keyword) resolve to the same PPL types ("string", "bigint", "string") that dynamic mapping inferred, and fieldformat reads from _source either way. Analytics-route compatibility goes from 1/5 to 4/5 (verified locally against a runTask cluster with analytics-engine + opensearch-sql-plugin). The remaining `testFieldFormatStringConcatenationWithNullFieldToString` needs a `tostring()` UDF on the analytics path — a multi-mode UDF (binary / hex / commas / duration) tracked separately as out of scope. Test plan: - ./gradlew :integ-test:integTest --tests 'org.opensearch.sql.calcite.remote.CalciteFieldFormatCommandIT' -> 5/5 green (v2 path, no regression). - ./gradlew :integ-test:analyticsCompatibilityTest --tests 'org.opensearch.sql.calcite.remote.CalciteFieldFormatCommandIT' -> 4/5 pass; the 5th fails on `tostring`'s missing capability registration, which is the documented out-of-scope category. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@test

…th (#5417) Same shape as #5407 for CalciteEvalCommandIT. The IT's init() previously created the in-test test_eval index via direct `PUT /test_eval/_doc/N` requests, relying on dynamic mapping. Two problems: 1. The doc PUTs auto-create the index with whatever settings the cluster defaults to. The analytics-engine compatibility path (force-routing on; tests.analytics.parquet_indices=true) needs parquet-backed indices, which TestUtils.createIndexByRestClient applies via TestUtils.makeParquetBacked when the system property is set. Direct PUTs sidestep that helper, so test_eval lands as Lucene-backed and the analytics planner rejects it with "No backend can scan all requested fields on index [test_eval]". All four working tests fail at execution. 2. init() runs before every @test method. The doc PUTs are doc-level idempotent, so re-running was wasteful but not failing. Once we switch to createIndexByRestClient, the index-level PUT is no longer idempotent and re-running throws "resource_already_exists_exception". Both addressed in one change: - test_eval is created via TestUtils.createIndexByRestClient with an explicit mapping (name/title=keyword, age=long). The helper honours tests.analytics.parquet_indices=true and produces a parquet-backed index for the analytics-engine sweep; on the v2 path the helper is a no-op around the index PUT, so behaviour is unchanged. - The whole init body is guarded by TestUtils.isIndexExist — same idempotency idiom that loadIndex uses for predefined fixtures. First @test method provisions; subsequent methods skip. Also pins the projection order on testFieldFormatStringConcatenation. The original query (`source=test_eval | fieldformat greeting = 'Hello ' + name`) had no `| fields` clause and relied on the implicit projection's column order — v2 returns Lucene-source insertion order, analytics returns parquet-storage order (alphabetical), so the assertion only matched on v2 by coincidence. Adding `| fields name, title, age, greeting` makes the assertion deterministic across paths; the existing expected rows (`rows("Alice", "Engineer", 25, "Hello Alice")`) already match this order, so v2 behaviour is preserved. The other four tests already had explicit `| fields ...` clauses, so no change there. No semantic change for the v2 path: the explicit mapping types (keyword, long, keyword) resolve to the same PPL types ("string", "bigint", "string") that dynamic mapping inferred, and fieldformat reads from _source either way. Analytics-route compatibility goes from 1/5 to 4/5 (verified locally against a runTask cluster with analytics-engine + opensearch-sql-plugin). The remaining `testFieldFormatStringConcatenationWithNullFieldToString` needs a `tostring()` UDF on the analytics path — a multi-mode UDF (binary / hex / commas / duration) tracked separately as out of scope. Test plan: - ./gradlew :integ-test:integTest --tests 'org.opensearch.sql.calcite.remote.CalciteFieldFormatCommandIT' -> 5/5 green (v2 path, no regression). - ./gradlew :integ-test:analyticsCompatibilityTest --tests 'org.opensearch.sql.calcite.remote.CalciteFieldFormatCommandIT' -> 4/5 pass; the 5th fails on `tostring`'s missing capability registration, which is the documented out-of-scope category. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

The feature branch advanced 9 commits since this PR was opened. Bring the catch-up branch up to that tip so the PR is mergeable into feature/mustang-ppl-integration. Resolutions: integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 TestEventReporterAsListener cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs get a parquet-backed index. Added back PR HEAD's test_eval_agent setup (needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java: took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter. Feature's design supersedes our prior wiring. plugin/.../SQLPlugin.java: took feature. The same opensearch-project#5403 simplification removed loadExtensions/EngineExtensionsHolder/executionEngineExtensions plumbing (no longer needed once analytics-engine is optionally bound). Feature retains the createSqlAnalyticsRouter method this PR introduced. plugin/.../config/EngineExtensionsHolder.java: deleted. Unreferenced after taking feature's SQLPlugin/TransportPPLQueryAction; not present on feature branch. Build: :api, :core, :opensearch-sql-plugin, :legacy compile + :integ-test compileTestJava all pass; unit tests pass; spotless clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

…ation Brings the catch-up branch up to current upstream/main (4 commits since this PR was opened) and current feature/mustang-ppl-integration (9 commits since this PR was opened), so the PR is mergeable into feature/mustang-ppl-integration without conflicts. Squashed (rather than two real merge commits) for the same DCO reason the original commit was squashed: upstream commits authored by many contributors with inconsistent or missing Signed-off-by trailers would otherwise be brought into this PR's history. Newer main commits absorbed (4): - opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - opensearch-project#5408 (datetime type normalization) - opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion) - opensearch-project#5399 (FGAC-scoped SQL cursor continuation) Newer feature commits absorbed (9): - opensearch-project#5403 (analytics-engine optional dependency — major rewiring) - opensearch-project#5407 (Carry CalciteEvalCommandIT through helper-managed index path) - opensearch-project#5413 (Default plugins.calcite.enabled=true on unified path) - opensearch-project#5415, opensearch-project#5416, opensearch-project#5417, opensearch-project#5409, opensearch-project#5400, opensearch-project#5406 (smaller carryovers + bumps) Conflict resolutions (10 from main side, 3 from feature side): api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. PR's RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in opensearch-project#5408 / opensearch-project#5419. core/executor/QueryService.java: composed both sides — kept HEAD's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking. Same pattern as the original PR resolution; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took HEAD. The 3-way merge produced a duplicated handleException/getRawErrorCode block; HEAD already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so HEAD is the correct superset. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 TestEventReporterAsListener cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs get a parquet-backed index. Added back PR HEAD's test_eval_agent setup (needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java: took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter. Feature's design supersedes our prior wiring. plugin/.../SQLPlugin.java: took feature. The same opensearch-project#5403 simplification removed loadExtensions/EngineExtensionsHolder/executionEngineExtensions plumbing (no longer needed once analytics-engine is optionally bound). Feature retains the createSqlAnalyticsRouter method this PR introduced. plugin/.../config/EngineExtensionsHolder.java: deleted. Unreferenced after taking feature's SQLPlugin/TransportPPLQueryAction; not present on feature branch. Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

Single squashed commit on top of feature/mustang-ppl-integration that absorbs upstream/main's commits not yet on the feature branch. Replaces the prior catch-up squash (opensearch-project#5396 base + the original af831d3 rebase commit) so this PR is a fast-forward into feature/mustang-ppl-integration. Squashed (rather than a merge commit) because upstream main commits were authored by many contributors with inconsistent or missing Signed-off-by trailers; DCO would otherwise reject those commits. Main commits absorbed (54 since divergence; 4 since the original catch-up squash was made on 2026-04-30): - opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - opensearch-project#5408 (datetime type normalization) - opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion) - opensearch-project#5399 (FGAC-scoped SQL cursor continuation) - opensearch-project#5394 (SQL Vector Search), opensearch-project#5361 (OpenSearch 3.7), opensearch-project#5360 (unified SQL language spec), opensearch-project#5240 (PPL Union), and 46 others. Conflict resolutions: api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in opensearch-project#5408 / opensearch-project#5419. core/executor/QueryService.java: composed both sides — kept feature's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took feature. The 3-way merge produced a duplicated handleException/getRawErrorCode block; feature already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so feature is the correct superset. CLAUDE.md, docs/user/ppl/functions/condition.md: took main. explain_streamstats_global{,_null_bucket}.yaml: took main (post-opensearch-project#5359 shape). core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into PlanUtils.findInputCollation). integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT. ppl/antlr/OpenSearchPPLParser.g4: added unionCommand. ppl/calcite/CalcitePPLStreamstatsTest.java: added testMultipleStreamstatsWithWindow. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs get a parquet-backed index. Added the test_eval_agent setup (needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java: took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter, and removed the loadExtensions / EngineExtensionsHolder / executionEngineExtensions plumbing. Feature retains the createSqlAnalyticsRouter method this catch-up introduced. plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced post-opensearch-project#5403; not present on feature). Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

@ignore

…5397) Single squashed commit on top of feature/mustang-ppl-integration that absorbs upstream/main's commits not yet on the feature branch. Replaces the prior catch-up squash (#5396 base + the original af831d3 rebase commit) so this PR is a fast-forward into feature/mustang-ppl-integration. Squashed (rather than a merge commit) because upstream main commits were authored by many contributors with inconsistent or missing Signed-off-by trailers; DCO would otherwise reject those commits. Main commits absorbed (54 since divergence; 4 since the original catch-up squash was made on 2026-04-30): - #5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec) - #5408 (datetime type normalization) - #5414 (Gradle wrapper bump + @ignore exclusion) - #5399 (FGAC-scoped SQL cursor continuation) - #5394 (SQL Vector Search), #5361 (OpenSearch 3.7), #5360 (unified SQL language spec), #5240 (PPL Union), and 46 others. Conflict resolutions: api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec, UnifiedSqlSpec): took main. Main is a strict superset — adds postAnalysisRules and preCompilationRules extension points, the new FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/ date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension wiring on SQL spec. RELEVANCE category is preserved unchanged. api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java: took main. Both adopt the new postAnalysisRules / preCompilationRules hooks introduced in #5408 / #5419. core/executor/QueryService.java: composed both sides — kept feature's CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's StageErrorHandler stage tracking; both improvements are orthogonal. legacy/plugin/RestSqlAction.java: took feature. The 3-way merge produced a duplicated handleException/getRawErrorCode block; feature already contained both the delegateToV2Engine refactor and the ErrorReport unwrap from main, so feature is the correct superset. CLAUDE.md, docs/user/ppl/functions/condition.md: took main. explain_streamstats_global{,_null_bucket}.yaml: took main (post-#5359 shape). core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into PlanUtils.findInputCollation). integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT. ppl/antlr/OpenSearchPPLParser.g4: added unionCommand. ppl/calcite/CalcitePPLStreamstatsTest.java: added testMultipleStreamstatsWithWindow. integ-test/build.gradle: took feature. Both sides added the same @ignore exclusion block; feature has alphabetical ordering and a more detailed comment explaining the Gradle 9.4.1 cast bug. integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took feature's helper-managed test_eval provisioning (createIndexByRestClient + isIndexExist guard, from #5407) so analytics-engine compatibility runs get a parquet-backed index. Added the test_eval_agent setup (needed by the dotted-path eval tests for #5351) wrapped in its own isIndexExist guard for the same parquet-aware idempotency. plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java: took feature. PR #5403 made analytics-engine an optional dependency by moving QueryPlanExecutor from a required constructor parameter to an @Inject(optional=true) setter, and removed the loadExtensions / EngineExtensionsHolder / executionEngineExtensions plumbing. Feature retains the createSqlAnalyticsRouter method this catch-up introduced. plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced post-#5403; not present on feature). Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava + :integ-test compileTestJava all pass; unit tests pass; spotlessCheck clean. Signed-off-by: Kai Huang <ahkcs@amazon.com>

ahkcs requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners May 5, 2026 22:57

ahkcs added the enhancement New feature or request label May 5, 2026

mengweieric approved these changes May 5, 2026

View reviewed changes

ahkcs merged commit 6ffba26 into opensearch-project:feature/mustang-ppl-integration May 6, 2026
29 of 37 checks passed

dai-chen mentioned this pull request May 6, 2026

Bump Gradle wrapper to 9.4.1 and workaround @Ignore test failure #5414

Merged

8 tasks

ahkcs mentioned this pull request May 7, 2026

Drop @Ignore'd Calcite ITs from CalciteNoPushdownIT @Suite #5416

Merged

5 tasks

ahkcs mentioned this pull request May 7, 2026

Carry CalciteFieldFormatCommandIT through the helper-managed index path #5417

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carry CalciteEvalCommandIT through the helper-managed index path#5407

Carry CalciteEvalCommandIT through the helper-managed index path#5407
ahkcs merged 2 commits intoopensearch-project:feature/mustang-ppl-integrationfrom
ahkcs:pr/eval-it-parquet-backed

ahkcs commented May 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ahkcs commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

1. Helper-managed test_eval provisioning

Why this matters

Changes

V2-path impact

2. Gradle 9.4.1 @Ignore cast workaround

Why this is in this PR

Root cause (verified via javap against gradle-testing-base-9.4.1.jar)

Workaround

Test results

Forward note

By submitting this pull request

Uh oh!

github-actions Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 435827e)

Uh oh!

github-actions Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahkcs commented May 5, 2026 •

edited

Loading

1. Helper-managed `test_eval` provisioning

2. Gradle 9.4.1 `@Ignore` cast workaround

Root cause (verified via `javap` against `gradle-testing-base-9.4.1.jar`)

github-actions Bot commented May 5, 2026 •

edited

Loading

(Review updated until commit `435827e`)

github-actions Bot commented May 5, 2026 •

edited

Loading