Skip to content

Commit 6335803

Browse files
committed
Merge upstream/feature/mustang-ppl-integration into PR catch-up branch
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>
2 parents a3ee2ea + c64a8c6 commit 6335803

10 files changed

Lines changed: 453 additions & 184 deletions

File tree

api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package org.opensearch.sql.api;
77

8+
import static org.opensearch.sql.common.setting.Settings.Key.CALCITE_ENGINE_ENABLED;
89
import static org.opensearch.sql.common.setting.Settings.Key.PPL_JOIN_SUBSEARCH_MAXOUT;
910
import static org.opensearch.sql.common.setting.Settings.Key.PPL_SUBSEARCH_MAXOUT;
1011
import static org.opensearch.sql.common.setting.Settings.Key.QUERY_SIZE_LIMIT;
@@ -119,13 +120,21 @@ public static class Builder {
119120
/**
120121
* Setting values with defaults from SysLimit.DEFAULT. Only includes planning-required settings
121122
* to avoid coupling with OpenSearchSettings.
123+
*
124+
* <p>{@link Settings.Key#CALCITE_ENGINE_ENABLED} defaults to {@code true} here because the
125+
* unified query path is by definition Calcite-based — every query reaching this context flows
126+
* through Calcite's planner, never the v2 engine. The PPL {@link
127+
* org.opensearch.sql.api.parser.PPLQueryParser} reuses the v2 {@code AstBuilder}, which gates
128+
* Calcite-only commands (e.g. {@code visitTableCommand}) on this setting; without the default,
129+
* those commands fail at parse time even when the cluster setting is true.
122130
*/
123131
private final Map<Settings.Key, Object> settings =
124132
new HashMap<Settings.Key, Object>(
125133
Map.of(
126134
QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(),
127135
PPL_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.subsearchLimit(),
128-
PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit()));
136+
PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit(),
137+
CALCITE_ENGINE_ENABLED, true));
129138

130139
/**
131140
* Sets the query language frontend to be used.

integ-test/build.gradle

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -557,10 +557,15 @@ integTest {
557557
// Exclude this IT, because they executed in another task (:integTestWithSecurity)
558558
exclude 'org/opensearch/sql/security/**'
559559

560-
// Workaround for Gradle 9.4.1 bug: TestEventReporterAsListener crashes with ClassCastException
561-
// when encountering class-level @Ignore annotations. These classes were already skipped by JUnit;
562-
// this moves the skip to the Gradle layer to avoid the buggy bridge.
563-
// Remove once Gradle ships a fix (not fixed as of 9.5.0).
560+
// Workaround for Gradle 9.4.1 ClassCastException in TestEventReporterAsListener.started
561+
// (line 58) — the bridge casts a parent test descriptor's reporter to
562+
// GroupTestEventReporterInternal but a class-level @Ignore produces a non-composite parent
563+
// descriptor with a leaf reporter, so the cast fails and aborts the entire integTest task
564+
// even though the tests would have been skipped anyway. The bridge is registered by Gradle's
565+
// own AbstractTestTask (we can't bypass it from user code), so the only available remedy is
566+
// to keep these classes off the test runner's input set. Net behaviour for CI: still
567+
// skipped, just at the build layer instead of inside JUnit. Remove once Gradle ships a fix.
568+
// OrderIT is already excluded above.
564569
exclude 'org/opensearch/sql/calcite/remote/CalciteInformationSchemaCommandIT.class'
565570
exclude 'org/opensearch/sql/calcite/remote/CalciteJsonFunctionsIT.class'
566571
exclude 'org/opensearch/sql/calcite/remote/CalcitePrometheusDataSourceCommandsIT.class'
@@ -570,18 +575,18 @@ integTest {
570575
exclude 'org/opensearch/sql/legacy/DateFunctionsIT.class'
571576
exclude 'org/opensearch/sql/legacy/HashJoinIT.class'
572577
exclude 'org/opensearch/sql/legacy/HavingIT.class'
573-
exclude 'org/opensearch/sql/legacy/JoinIT.class'
574578
exclude 'org/opensearch/sql/legacy/JSONRequestIT.class'
579+
exclude 'org/opensearch/sql/legacy/JoinIT.class'
575580
exclude 'org/opensearch/sql/legacy/MathFunctionsIT.class'
576581
exclude 'org/opensearch/sql/legacy/MetricsIT.class'
577582
exclude 'org/opensearch/sql/legacy/MultiQueryIT.class'
578583
exclude 'org/opensearch/sql/legacy/NestedFieldQueryIT.class'
579584
exclude 'org/opensearch/sql/legacy/PreparedStatementIT.class'
580585
exclude 'org/opensearch/sql/legacy/QueryFunctionsIT.class'
581586
exclude 'org/opensearch/sql/legacy/QueryIT.class'
587+
exclude 'org/opensearch/sql/legacy/SQLFunctionsIT.class'
582588
exclude 'org/opensearch/sql/legacy/ShowIT.class'
583589
exclude 'org/opensearch/sql/legacy/SourceFieldIT.class'
584-
exclude 'org/opensearch/sql/legacy/SQLFunctionsIT.class'
585590
exclude 'org/opensearch/sql/legacy/SubqueryIT.class'
586591
exclude 'org/opensearch/sql/ppl/JsonFunctionsIT.class'
587592
exclude 'org/opensearch/sql/sql/ExpressionIT.class'

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.json.JSONObject;
1818
import org.junit.jupiter.api.Test;
1919
import org.opensearch.client.Request;
20+
import org.opensearch.sql.legacy.TestUtils;
2021
import org.opensearch.sql.ppl.PPLIntegTestCase;
2122

2223
public class CalciteEvalCommandIT extends PPLIntegTestCase {
@@ -29,38 +30,63 @@ public void init() throws Exception {
2930
loadIndex(Index.BANK);
3031
loadIndex(Index.TELEMETRY);
3132

32-
// Create test data for string concatenation
33-
Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
34-
request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
35-
client().performRequest(request1);
36-
37-
Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
38-
request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
39-
client().performRequest(request2);
40-
41-
Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
42-
request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
43-
client().performRequest(request3);
33+
// Pre-create test_eval through the helper so the analytics-engine compatibility run
34+
// (tests.analytics.parquet_indices=true) provisions it as a parquet-backed composite
35+
// index. Plain auto-mapping via the doc PUTs would create a Lucene-backed index, which
36+
// the analytics-engine planner cannot scan ("No backend can scan all requested fields").
37+
// Explicit mapping pins types so both v2 (verifySchema "string"/"bigint") and analytics
38+
// paths see the same shape regardless of dynamic-mapping behavior on the parquet engine.
39+
// Guarded by isIndexExist for idempotency — init() runs before each @Test method.
40+
if (!TestUtils.isIndexExist(client(), "test_eval")) {
41+
String testEvalMapping =
42+
"{\"mappings\":{\"properties\":{"
43+
+ "\"name\":{\"type\":\"keyword\"},"
44+
+ "\"age\":{\"type\":\"long\"},"
45+
+ "\"title\":{\"type\":\"keyword\"}}}}";
46+
TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping);
47+
48+
// Create test data for string concatenation
49+
Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
50+
request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
51+
client().performRequest(request1);
52+
53+
Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
54+
request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
55+
client().performRequest(request2);
56+
57+
Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
58+
request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
59+
client().performRequest(request3);
60+
}
4461

4562
// Index with a struct field `agent` to reproduce the reviewer's case from PR #5351:
4663
// source=<idx with agent struct> | fields agent | eval agent.name = "test"
4764
// Rely on dynamic mapping — OpenSearch infers `agent` as an object with string children
4865
// from the document contents. Using dynamic mapping keeps the init idempotent across
4966
// repeated `@Before` invocations in the preserved cluster.
50-
Request agentDoc1 = new Request("PUT", "/test_eval_agent/_doc/1?refresh=true");
51-
agentDoc1.setJsonEntity(
52-
"{\"agent\": {\"name\": \"winlogbeat\", \"version\": \"7.0\"}, \"message\": \"hello\"}");
53-
client().performRequest(agentDoc1);
54-
55-
Request agentDoc2 = new Request("PUT", "/test_eval_agent/_doc/2?refresh=true");
56-
agentDoc2.setJsonEntity(
57-
"{\"agent\": {\"name\": \"filebeat\", \"version\": \"8.1\"}, \"message\": \"world\"}");
58-
client().performRequest(agentDoc2);
67+
if (!TestUtils.isIndexExist(client(), "test_eval_agent")) {
68+
Request agentDoc1 = new Request("PUT", "/test_eval_agent/_doc/1?refresh=true");
69+
agentDoc1.setJsonEntity(
70+
"{\"agent\": {\"name\": \"winlogbeat\", \"version\": \"7.0\"}, \"message\": \"hello\"}");
71+
client().performRequest(agentDoc1);
72+
73+
Request agentDoc2 = new Request("PUT", "/test_eval_agent/_doc/2?refresh=true");
74+
agentDoc2.setJsonEntity(
75+
"{\"agent\": {\"name\": \"filebeat\", \"version\": \"8.1\"}, \"message\": \"world\"}");
76+
client().performRequest(agentDoc2);
77+
}
5978
}
6079

6180
@Test
6281
public void testEvalStringConcatenation() throws IOException {
63-
JSONObject result = executeQuery("source=test_eval | eval greeting = 'Hello ' + name");
82+
// Pin the projection so column order is deterministic across execution paths — the
83+
// analytics-engine route reads parquet schema in storage order, which can differ from the
84+
// v2 / Lucene path's _source-iteration order. Adding an explicit | fields makes the test
85+
// a strict assertion on the eval expression rather than a coincidence of projection order.
86+
JSONObject result =
87+
executeQuery(
88+
"source=test_eval | eval greeting = 'Hello ' + name | fields name, title, age,"
89+
+ " greeting");
6490
verifySchema(
6591
result,
6692
schema("name", "string"),

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteFieldFormatCommandIT.java

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.json.JSONObject;
1313
import org.junit.jupiter.api.Test;
1414
import org.opensearch.client.Request;
15+
import org.opensearch.sql.legacy.TestUtils;
1516
import org.opensearch.sql.ppl.PPLIntegTestCase;
1617

1718
public class CalciteFieldFormatCommandIT extends PPLIntegTestCase {
@@ -23,26 +24,48 @@ public void init() throws Exception {
2324

2425
loadIndex(Index.BANK);
2526

26-
// Create test data for string concatenation
27-
Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
28-
request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
29-
client().performRequest(request1);
27+
// Pre-create test_eval through the helper so the analytics-engine compatibility run
28+
// (tests.analytics.parquet_indices=true) provisions it as a parquet-backed composite
29+
// index. Plain auto-mapping via the doc PUTs would create a Lucene-backed index, which
30+
// the analytics-engine planner cannot scan ("No backend can scan all requested fields").
31+
// Explicit mapping pins types so both v2 (verifySchema "string"/"bigint") and analytics
32+
// paths see the same shape regardless of dynamic-mapping behavior on the parquet engine.
33+
// Guarded by isIndexExist for idempotency — init() runs before each @Test method.
34+
if (!TestUtils.isIndexExist(client(), "test_eval")) {
35+
String testEvalMapping =
36+
"{\"mappings\":{\"properties\":{"
37+
+ "\"name\":{\"type\":\"keyword\"},"
38+
+ "\"age\":{\"type\":\"long\"},"
39+
+ "\"title\":{\"type\":\"keyword\"}}}}";
40+
TestUtils.createIndexByRestClient(client(), "test_eval", testEvalMapping);
3041

31-
Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
32-
request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
33-
client().performRequest(request2);
42+
// Create test data for string concatenation
43+
Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true");
44+
request1.setJsonEntity("{\"name\": \"Alice\", \"age\": 25, \"title\": \"Engineer\"}");
45+
client().performRequest(request1);
3446

35-
Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
36-
request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
37-
client().performRequest(request3);
47+
Request request2 = new Request("PUT", "/test_eval/_doc/2?refresh=true");
48+
request2.setJsonEntity("{\"name\": \"Bob\", \"age\": 30, \"title\": \"Manager\"}");
49+
client().performRequest(request2);
50+
51+
Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true");
52+
request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}");
53+
client().performRequest(request3);
54+
}
3855
}
3956

4057
@Test
4158
public void testFieldFormatStringConcatenation() throws IOException {
59+
// Pin the projection so column order is deterministic across execution paths — the
60+
// analytics-engine route reads parquet schema in storage order, which can differ from the
61+
// v2 / Lucene path's _source-iteration order. Adding an explicit | fields makes the test
62+
// a strict assertion on the fieldformat expression rather than a coincidence of projection
63+
// order.
4264
JSONObject result =
4365
executeQuery(
4466
StringEscapeUtils.escapeJson(
45-
"source=test_eval | fieldformat greeting = 'Hello ' + name"));
67+
"source=test_eval | fieldformat greeting = 'Hello ' + name | fields name, title,"
68+
+ " age, greeting"));
4669
verifySchema(
4770
result,
4871
schema("name", "string"),

0 commit comments

Comments
 (0)