Skip to content

Commit 20bd641

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 the `UnifiedQueryContext.Builder`'s static defaults map 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.settings(Settings)` hook that the REST handler can use to inject the cluster's live `OpenSearchSettings` instance. Inside `buildSettings()`, the Builder snapshots the live value of `PPL_REX_MAX_MATCH_LIMIT` into the defaults 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 Builder's existing static defaults map is renamed `settings -> defaults` to free up the `settings` identifier for the live-cluster-settings concept. `RestUnifiedQueryAction` gains a `pluginSettings` field (the same `OpenSearchSettings` instance bound in the Guice module) and forwards it to the Builder in both `buildContext` and `buildParsingContext`. Both construction sites — `SQLPlugin.createSqlAnalyticsRouter` and `TransportPPLQueryAction.<init>` — are updated. ## Why scoped to `PPL_REX_MAX_MATCH_LIMIT` only The same architectural gap exists for every key in the defaults 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. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent 3763d17 commit 20bd641

4 files changed

Lines changed: 51 additions & 11 deletions

File tree

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

Lines changed: 35 additions & 4 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 settings;
120121

121122
/**
122123
* Setting values with defaults from SysLimit.DEFAULT. Only includes planning-required settings
@@ -136,7 +137,7 @@ public static class Builder {
136137
* {@code OpenSearchSettings.PPL_REX_MAX_MATCH_LIMIT_SETTING}, so unified-path behavior matches
137138
* v2-path behavior when neither has an explicit cluster override.
138139
*/
139-
private final Map<Settings.Key, Object> settings =
140+
private final Map<Settings.Key, Object> defaults =
140141
new HashMap<Settings.Key, Object>(
141142
Map.of(
142143
QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(),
@@ -215,7 +216,25 @@ public Builder setting(String name, Object value) {
215216
Settings.Key key =
216217
Settings.Key.of(name)
217218
.orElseThrow(() -> new IllegalArgumentException("Unknown setting name: " + name));
218-
settings.put(key, value);
219+
defaults.put(key, value);
220+
return this;
221+
}
222+
223+
/**
224+
* Injects the live cluster {@link Settings} so dynamic cluster overrides reach the unified-path
225+
* settings map. Without this, every key in {@link #defaults} resolves to its hardcoded default
226+
* and {@code _cluster/settings} updates are invisible to the unified path.
227+
*
228+
* <p>Scoped to {@link Settings.Key#PPL_REX_MAX_MATCH_LIMIT} for now — required so {@code rex
229+
* max_match=0} caps at the operator-configured limit (CalciteRexCommandIT's
230+
* testRexMaxMatchConfigurableLimit). The other keys in {@link #defaults} keep their static
231+
* defaults; widening the snapshot to all keys is a separate scope.
232+
*
233+
* @param settings the cluster's live {@code OpenSearchSettings} instance
234+
* @return this builder instance
235+
*/
236+
public Builder settings(Settings settings) {
237+
this.settings = settings;
219238
return this;
220239
}
221240

@@ -232,6 +251,7 @@ public UnifiedQueryContext build() {
232251
case SQL -> UnifiedSqlSpec.extended();
233252
case PPL -> UnifiedPplSpec.create();
234253
};
254+
235255
Settings settings = buildSettings();
236256
CalcitePlanContext planContext =
237257
CalcitePlanContext.create(
@@ -249,16 +269,27 @@ private UnifiedQueryParser<?> createParser(CalcitePlanContext planContext, Setti
249269
}
250270

251271
private Settings buildSettings() {
272+
// Snapshot PPL_REX_MAX_MATCH_LIMIT from live cluster settings if present so a
273+
// mid-test `_cluster/settings` PUT takes effect on the unified path. Snapshot
274+
// semantics (not per-call lookup) — UnifiedQueryContext is built per HTTP
275+
// request, so a one-shot read at build() is sufficient. Other keys are
276+
// intentionally not bridged here; that's a follow-up scope decision.
277+
if (settings != null) {
278+
Object liveRexLimit = settings.getSettingValue(PPL_REX_MAX_MATCH_LIMIT);
279+
if (liveRexLimit != null) {
280+
defaults.put(PPL_REX_MAX_MATCH_LIMIT, liveRexLimit);
281+
}
282+
}
252283
return new Settings() {
253284
@Override
254285
@SuppressWarnings("unchecked")
255286
public <T> T getSettingValue(Key key) {
256-
return (T) settings.get(key);
287+
return (T) defaults.get(key);
257288
}
258289

259290
@Override
260291
public List<?> getSettings() {
261-
return List.copyOf(settings.entrySet());
292+
return List.copyOf(defaults.entrySet());
262293
}
263294
};
264295
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,8 @@ 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] =
212+
new RestUnifiedQueryAction(client, clusterService, executor, pluginSettings);
212213
}
213214
return cached[0];
214215
};

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).settings(pluginSettings).build();
159162
}
160163

161164
private UnifiedQueryContext buildContext(QueryType queryType, boolean profiling) {
162165
return UnifiedQueryContext.builder()
163166
.language(queryType)
167+
.settings(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: 7 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,8 @@ 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(
112+
clientRef, clusterServiceRef, queryPlanExecutor, pluginSettingsRef);
109113
}
110114

111115
/**

0 commit comments

Comments
 (0)