Skip to content

Commit 62d617d

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. ## Why scoped to PPL_REX_MAX_MATCH_LIMIT only 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. ## Wire-up `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`. ## Test results 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. ## Companion PR 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 0f1c8ce commit 62d617d

4 files changed

Lines changed: 44 additions & 7 deletions

File tree

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ public static class Builder {
113113
private String defaultNamespace;
114114
private boolean cacheMetadata = false;
115115
private boolean profiling = false;
116+
private Settings liveSettings;
116117

117118
/**
118119
* Setting values with defaults from SysLimit.DEFAULT. Only includes planning-required settings
@@ -215,6 +216,25 @@ public Builder setting(String name, Object value) {
215216
return this;
216217
}
217218

219+
/**
220+
* Injects the live cluster {@link Settings} so dynamic cluster overrides reach the
221+
* unified-path settings map. Without this, every key in {@link #settings} resolves to
222+
* its hardcoded default and {@code _cluster/settings} updates are invisible to the
223+
* unified path.
224+
*
225+
* <p>Scoped to {@link Settings.Key#PPL_REX_MAX_MATCH_LIMIT} for now — required so
226+
* {@code rex max_match=0} caps at the operator-configured limit (CalciteRexCommandIT's
227+
* testRexMaxMatchConfigurableLimit). The other keys in {@link #settings} keep their
228+
* static defaults; widening the snapshot to all keys is a separate scope.
229+
*
230+
* @param liveSettings the cluster's live {@code OpenSearchSettings} instance
231+
* @return this builder instance
232+
*/
233+
public Builder liveSettings(Settings liveSettings) {
234+
this.liveSettings = liveSettings;
235+
return this;
236+
}
237+
218238
/**
219239
* Builds a {@link UnifiedQueryContext} with the configuration.
220240
*
@@ -223,6 +243,18 @@ public Builder setting(String name, Object value) {
223243
public UnifiedQueryContext build() {
224244
Objects.requireNonNull(queryType, "Must specify language before build");
225245

246+
// Snapshot PPL_REX_MAX_MATCH_LIMIT from live cluster settings if present so a
247+
// mid-test `_cluster/settings` PUT takes effect on the unified path. Snapshot
248+
// semantics (not per-call lookup) — UnifiedQueryContext is built per HTTP
249+
// request, so a one-shot read at build() is sufficient. Other keys are
250+
// intentionally not bridged here; that's a follow-up scope decision.
251+
if (liveSettings != null) {
252+
Object liveRexLimit = liveSettings.getSettingValue(PPL_REX_MAX_MATCH_LIMIT);
253+
if (liveRexLimit != null) {
254+
settings.put(PPL_REX_MAX_MATCH_LIMIT, liveRexLimit);
255+
}
256+
}
257+
226258
Settings settings = buildSettings();
227259
CalcitePlanContext planContext =
228260
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: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,17 +78,18 @@ public TransportPPLQueryAction(
7878

7979
ModulesBuilder modules = new ModulesBuilder();
8080
modules.add(new OpenSearchPluginModule());
81+
org.opensearch.sql.common.setting.Settings pluginSettings =
82+
new OpenSearchSettings(clusterService.getClusterSettings());
8183
modules.add(
8284
b -> {
8385
b.bind(NodeClient.class).toInstance(client);
84-
b.bind(org.opensearch.sql.common.setting.Settings.class)
85-
.toInstance(new OpenSearchSettings(clusterService.getClusterSettings()));
86+
b.bind(org.opensearch.sql.common.setting.Settings.class).toInstance(pluginSettings);
8687
b.bind(DataSourceService.class).toInstance(dataSourceService);
8788
});
8889
this.injector = Guice.createInjector(modules);
8990
AnalyticsExecutorHolder.set(queryPlanExecutor);
9091
this.unifiedQueryHandler =
91-
new RestUnifiedQueryAction(client, clusterService, queryPlanExecutor);
92+
new RestUnifiedQueryAction(client, clusterService, queryPlanExecutor, pluginSettings);
9293
this.pplEnabled =
9394
() ->
9495
MULTI_ALLOW_EXPLICIT_INDEX.get(clusterSettings)

0 commit comments

Comments
 (0)