Skip to content

Commit 968e781

Browse files
committed
Bridge live cluster settings into UnifiedQueryContext for PPL_REX_MAX_MATCH_LIMIT
The previous commit defaulted `PPL_REX_MAX_MATCH_LIMIT=10` in `UnifiedQueryContext.Builder.settings` to fix the NPE in `AstBuilder.visitRexCommand` on the unified path. The default is correct, but it doesn't respect mid-run cluster overrides — every key in the static map returns its hardcoded value regardless of `_cluster/settings` updates. This breaks `CalciteRexCommandIT.testRexMaxMatchConfigurableLimit`, which explicitly sets the cluster-side limit to 5 and asserts that `max_match=0` caps at 5; on the unified path it stayed at 10. This change introduces a `Builder.liveSettings(Settings)` hook that the REST handler can use to inject the cluster's live `OpenSearchSettings` instance. At `build()` time the Builder snapshots the live value of `PPL_REX_MAX_MATCH_LIMIT` (only — see scoping note below) into the static map, overriding the hardcoded default when the operator has set a cluster value. Snapshot-at-build matches the per-HTTP-request lifecycle of `UnifiedQueryContext` and avoids per-call lookup overhead. The same architectural gap exists for every key in the static map (`QUERY_SIZE_LIMIT`, `PPL_SUBSEARCH_MAXOUT`, `PPL_JOIN_SUBSEARCH_MAXOUT`, `CALCITE_ENGINE_ENABLED`). For three of those, the static defaults are fine in practice (no test overrides them mid-run; `head N` covers `QUERY_SIZE_LIMIT` per-query). `CALCITE_ENGINE_ENABLED` is intentionally pinned to `true` for the unified path — a cluster override toggling it off would defeat the point of routing here. So this PR widens only the one key that demonstrably needs it; widening the snapshot to the rest is a future scope decision tied to whichever new IT first depends on it. `RestUnifiedQueryAction` gains a `pluginSettings` field (the same `OpenSearchSettings` instance bound in the Guice module) and forwards it to the Builder in both `buildContext` (per-request execution path) and `buildParsingContext` (analytics-routing index name probe). Both construction sites — `SQLPlugin.createSqlAnalyticsRouter` and `TransportPPLQueryAction.<init>` — are updated to pass the existing plugin-side `Settings` instance. `buildParsingContext` had been `static` because it didn't need any instance state; it's now an instance method since it reads `pluginSettings`. CalciteRexCommandIT through the analytics-engine route (every PPL query forced through `/_analytics/ppl` via `tests.analytics.force_routing=true`): * Before this change: 17/18 — `testRexMaxMatchConfigurableLimit` fails with `expected:<5> but was:<10>` (cluster override doesn't reach the unified path). * After this change: 18/18 — all `testRexMaxMatch*` variants honor the cluster setting. opensearch-project/OpenSearch#21550 — onboards PPL `rex` to DataFusion via the analytics-engine path. The 17/18 baseline reported in that PR's description was measured against the previous commit on this branch; with this change the route hits 18/18. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent 3763d17 commit 968e781

4 files changed

Lines changed: 47 additions & 7 deletions

File tree

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ public static class Builder {
117117
private String defaultNamespace;
118118
private boolean cacheMetadata = false;
119119
private boolean profiling = false;
120+
private Settings liveSettings;
120121

121122
/**
122123
* Setting values with defaults from SysLimit.DEFAULT. Only includes planning-required settings
@@ -219,6 +220,25 @@ public Builder setting(String name, Object value) {
219220
return this;
220221
}
221222

223+
/**
224+
* Injects the live cluster {@link Settings} so dynamic cluster overrides reach the
225+
* unified-path settings map. Without this, every key in {@link #settings} resolves to
226+
* its hardcoded default and {@code _cluster/settings} updates are invisible to the
227+
* unified path.
228+
*
229+
* <p>Scoped to {@link Settings.Key#PPL_REX_MAX_MATCH_LIMIT} for now — required so
230+
* {@code rex max_match=0} caps at the operator-configured limit (CalciteRexCommandIT's
231+
* testRexMaxMatchConfigurableLimit). The other keys in {@link #settings} keep their
232+
* static defaults; widening the snapshot to all keys is a separate scope.
233+
*
234+
* @param liveSettings the cluster's live {@code OpenSearchSettings} instance
235+
* @return this builder instance
236+
*/
237+
public Builder liveSettings(Settings liveSettings) {
238+
this.liveSettings = liveSettings;
239+
return this;
240+
}
241+
222242
/**
223243
* Builds a {@link UnifiedQueryContext} with the configuration.
224244
*
@@ -232,6 +252,19 @@ public UnifiedQueryContext build() {
232252
case SQL -> UnifiedSqlSpec.extended();
233253
case PPL -> UnifiedPplSpec.create();
234254
};
255+
256+
// Snapshot PPL_REX_MAX_MATCH_LIMIT from live cluster settings if present so a
257+
// mid-test `_cluster/settings` PUT takes effect on the unified path. Snapshot
258+
// semantics (not per-call lookup) — UnifiedQueryContext is built per HTTP
259+
// request, so a one-shot read at build() is sufficient. Other keys are
260+
// intentionally not bridged here; that's a follow-up scope decision.
261+
if (liveSettings != null) {
262+
Object liveRexLimit = liveSettings.getSettingValue(PPL_REX_MAX_MATCH_LIMIT);
263+
if (liveRexLimit != null) {
264+
settings.put(PPL_REX_MAX_MATCH_LIMIT, liveRexLimit);
265+
}
266+
}
267+
235268
Settings settings = buildSettings();
236269
CalcitePlanContext planContext =
237270
CalcitePlanContext.create(

plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ private BiFunction<SQLQueryRequest, RestChannel, Boolean> createSqlAnalyticsRout
208208
if (executor == null) {
209209
return null;
210210
}
211-
cached[0] = new RestUnifiedQueryAction(client, clusterService, executor);
211+
cached[0] = new RestUnifiedQueryAction(client, clusterService, executor, pluginSettings);
212212
}
213213
return cached[0];
214214
};

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,17 @@ public class RestUnifiedQueryAction {
5959
private final AnalyticsExecutionEngine analyticsEngine;
6060
private final NodeClient client;
6161
private final ClusterService clusterService;
62+
private final org.opensearch.sql.common.setting.Settings pluginSettings;
6263

6364
public RestUnifiedQueryAction(
6465
NodeClient client,
6566
ClusterService clusterService,
66-
QueryPlanExecutor<RelNode, Iterable<Object[]>> planExecutor) {
67+
QueryPlanExecutor<RelNode, Iterable<Object[]>> planExecutor,
68+
org.opensearch.sql.common.setting.Settings pluginSettings) {
6769
this.client = client;
6870
this.clusterService = clusterService;
6971
this.analyticsEngine = new AnalyticsExecutionEngine(planExecutor);
72+
this.pluginSettings = pluginSettings;
7073
}
7174

7275
/**
@@ -154,13 +157,14 @@ public void explain(
154157
* Build a lightweight context for parsing only (index name extraction). Does not require cluster
155158
* state or catalog schema.
156159
*/
157-
private static UnifiedQueryContext buildParsingContext(QueryType queryType) {
158-
return UnifiedQueryContext.builder().language(queryType).build();
160+
private UnifiedQueryContext buildParsingContext(QueryType queryType) {
161+
return UnifiedQueryContext.builder().language(queryType).liveSettings(pluginSettings).build();
159162
}
160163

161164
private UnifiedQueryContext buildContext(QueryType queryType, boolean profiling) {
162165
return UnifiedQueryContext.builder()
163166
.language(queryType)
167+
.liveSettings(pluginSettings)
164168
.catalog(SCHEMA_NAME, OpenSearchSchemaBuilder.buildSchema(clusterService.state()))
165169
.defaultNamespace(SCHEMA_NAME)
166170
.profiling(profiling)

plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ public class TransportPPLQueryAction
6767

6868
private final NodeClient clientRef;
6969
private final ClusterService clusterServiceRef;
70+
private final org.opensearch.sql.common.setting.Settings pluginSettingsRef;
7071

7172
@Inject
7273
public TransportPPLQueryAction(
@@ -82,11 +83,13 @@ public TransportPPLQueryAction(
8283

8384
ModulesBuilder modules = new ModulesBuilder();
8485
modules.add(new OpenSearchPluginModule());
86+
org.opensearch.sql.common.setting.Settings pluginSettings =
87+
new OpenSearchSettings(clusterService.getClusterSettings());
88+
this.pluginSettingsRef = pluginSettings;
8589
modules.add(
8690
b -> {
8791
b.bind(NodeClient.class).toInstance(client);
88-
b.bind(org.opensearch.sql.common.setting.Settings.class)
89-
.toInstance(new OpenSearchSettings(clusterService.getClusterSettings()));
92+
b.bind(org.opensearch.sql.common.setting.Settings.class).toInstance(pluginSettings);
9093
b.bind(DataSourceService.class).toInstance(dataSourceService);
9194
});
9295
this.injector = Guice.createInjector(modules);
@@ -105,7 +108,7 @@ public void setQueryPlanExecutor(
105108
QueryPlanExecutor<RelNode, Iterable<Object[]>> queryPlanExecutor) {
106109
AnalyticsExecutorHolder.set(queryPlanExecutor);
107110
this.unifiedQueryHandler =
108-
new RestUnifiedQueryAction(clientRef, clusterServiceRef, queryPlanExecutor);
111+
new RestUnifiedQueryAction(clientRef, clusterServiceRef, queryPlanExecutor, pluginSettingsRef);
109112
}
110113

111114
/**

0 commit comments

Comments
 (0)