Carry CalciteEvalCommandIT through the helper-managed index path (#5407)

ahkcs · web-flow · commit 6ffba26a26c4 · 2026-05-05T20:32:14.000-07:00
* Carry CalciteEvalCommandIT through the helper-managed index path 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> * Exclude @ignore'd test classes from integTest as Gradle 9.4.1 workaround 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 #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> --------- Signed-off-by: Kai Huang <ahkcs@amazon.com>
diff --git a/integ-test/build.gradle b/integ-test/build.gradle
@@ -557,6 +557,40 @@ integTest {
     // Exclude this IT, because they executed in another task (:integTestWithSecurity)
     exclude 'org/opensearch/sql/security/**'
 
+    // Workaround for Gradle 9.4.1 ClassCastException in TestEventReporterAsListener.started
+    // (line 58) — the bridge casts a parent test descriptor's reporter to
+    // GroupTestEventReporterInternal but a class-level @Ignore produces a non-composite parent
+    // descriptor with a leaf reporter, so the cast fails and aborts the entire integTest task
+    // even though the tests would have been skipped anyway. The bridge is registered by Gradle's
+    // own AbstractTestTask (we can't bypass it from user code), so the only available remedy is
+    // to keep these classes off the test runner's input set. Net behaviour for CI: still
+    // skipped, just at the build layer instead of inside JUnit. Remove once Gradle ships a fix.
+    // OrderIT is already excluded above.
+    exclude 'org/opensearch/sql/calcite/remote/CalciteInformationSchemaCommandIT.class'
+    exclude 'org/opensearch/sql/calcite/remote/CalciteJsonFunctionsIT.class'
+    exclude 'org/opensearch/sql/calcite/remote/CalcitePrometheusDataSourceCommandsIT.class'
+    exclude 'org/opensearch/sql/calcite/remote/CalciteShowDataSourcesCommandIT.class'
+    exclude 'org/opensearch/sql/legacy/AggregationIT.class'
+    exclude 'org/opensearch/sql/legacy/DateFormatIT.class'
+    exclude 'org/opensearch/sql/legacy/DateFunctionsIT.class'
+    exclude 'org/opensearch/sql/legacy/HashJoinIT.class'
+    exclude 'org/opensearch/sql/legacy/HavingIT.class'
+    exclude 'org/opensearch/sql/legacy/JSONRequestIT.class'
+    exclude 'org/opensearch/sql/legacy/JoinIT.class'
+    exclude 'org/opensearch/sql/legacy/MathFunctionsIT.class'
+    exclude 'org/opensearch/sql/legacy/MetricsIT.class'
+    exclude 'org/opensearch/sql/legacy/MultiQueryIT.class'
+    exclude 'org/opensearch/sql/legacy/NestedFieldQueryIT.class'
+    exclude 'org/opensearch/sql/legacy/PreparedStatementIT.class'
+    exclude 'org/opensearch/sql/legacy/QueryFunctionsIT.class'
+    exclude 'org/opensearch/sql/legacy/QueryIT.class'
+    exclude 'org/opensearch/sql/legacy/SQLFunctionsIT.class'
+    exclude 'org/opensearch/sql/legacy/ShowIT.class'
+    exclude 'org/opensearch/sql/legacy/SourceFieldIT.class'
+    exclude 'org/opensearch/sql/legacy/SubqueryIT.class'
+    exclude 'org/opensearch/sql/ppl/JsonFunctionsIT.class'
+    exclude 'org/opensearch/sql/sql/ExpressionIT.class'
+
     finalizedBy 'printIntegTestPaths'
 }
 
diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java
@@ -15,6 +15,7 @@
 import org.json.JSONObject;
 import org.junit.jupiter.api.Test;
 import org.opensearch.client.Request;
+import org.opensearch.sql.legacy.TestUtils;
 import org.opensearch.sql.ppl.PPLIntegTestCase;
 
 public class CalciteEvalCommandIT extends PPLIntegTestCase {
@@ -26,23 +27,46 @@ public void init() throws Exception {
 
     loadIndex(Index.BANK);
 
-    // 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);
+    // Pre-create test_eval through the helper so the analytics-engine compatibility run
+    // (tests.analytics.parquet_indices=true) provisions it as a parquet-backed composite
+    // index. Plain auto-mapping via the doc PUTs would create a Lucene-backed index, which
+    // the analytics-engine planner cannot scan ("No backend can scan all requested fields").
+    // Explicit mapping pins types so both v2 (verifySchema "string"/"bigint") and analytics
+    // paths see the same shape regardless of dynamic-mapping behavior on the parquet engine.
+    // Guarded by isIndexExist for idempotency — init() runs before each @Test 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);
 
-    Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
-    request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
-    client().performRequest(request2);
+      // 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 request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
-    request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
-    client().performRequest(request3);
+      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);
+    }
   }
 
   @Test
   public void testEvalStringConcatenation() throws IOException {
-    JSONObject result = executeQuery("source=test_eval | eval greeting = 'Hello ' + name");
+    // Pin the projection so column order is deterministic across execution paths — the
+    // analytics-engine route reads parquet schema in storage order, which can differ from the
+    // v2 / Lucene path's _source-iteration order. Adding an explicit | fields makes the test
+    // a strict assertion on the eval expression rather than a coincidence of projection order.
+    JSONObject result =
+        executeQuery(
+            "source=test_eval | eval greeting = 'Hello ' + name | fields name, title, age,"
+                + " greeting");
     verifySchema(
         result,
         schema("name", "string"),
