Skip to content

Commit a74bbaf

Browse files
committed
Bridge cluster overrides for PPL_REX_MAX_MATCH_LIMIT via the existing setting() API
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 sets the cluster-side limit to 5 and asserts `max_match=0` caps at 5; on the unified path it stayed at 10. Rather than introducing a new `Settings`-typed Builder API, the REST handler reads the live cluster value itself and routes it through the existing `Builder.setting(String, Object)` method — keeping `UnifiedQueryContext` decoupled from any specific `Settings` implementation: RestUnifiedQueryAction.applyClusterOverrides(builder) └── pluginSettings.getSettingValue(PPL_REX_MAX_MATCH_LIMIT) └── builder.setting("plugins.ppl.rex.max_match.limit", value) `RestUnifiedQueryAction` gains a `pluginSettings` field (the same `OpenSearchSettings` instance bound in the Guice module). Both construction sites — `SQLPlugin.createSqlAnalyticsRouter` and `TransportPPLQueryAction.<init>` — pass it through. `RestUnifiedQueryActionTest` updated to pass a `mock(Settings.class)` for the new constructor parameter. ## Why scoped to PPL_REX_MAX_MATCH_LIMIT only The same architectural gap exists for every key in the static 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. 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 a74bbaf

5 files changed

Lines changed: 52 additions & 16 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,12 @@ public static class Builder {
131131
*
132132
* <p>{@link Settings.Key#PPL_REX_MAX_MATCH_LIMIT} defaults to {@code 10} here because {@code
133133
* AstBuilder.visitRexCommand} reads it unconditionally and unboxes to {@code int} — a {@code
134-
* null} return from {@code getSettingValue} NPEs the planner before any operator- level
134+
* null} return from {@code getSettingValue} NPEs the planner before any operator-level
135135
* capability check runs. The value mirrors the cluster-side default of {@code 10} registered by
136-
* {@code OpenSearchSettings.PPL_REX_MAX_MATCH_LIMIT_SETTING}, so unified-path behavior matches
137-
* v2-path behavior when neither has an explicit cluster override.
136+
* {@code OpenSearchSettings.PPL_REX_MAX_MATCH_LIMIT_SETTING}. Cluster-side overrides reach this
137+
* map via {@link #setting(String, Object)} — the REST handler reads the live value from {@code
138+
* OpenSearchSettings} and routes it through that existing API, keeping {@link
139+
* UnifiedQueryContext} decoupled from any specific {@link Settings} implementation.
138140
*/
139141
private final Map<Settings.Key, Object> settings =
140142
new HashMap<Settings.Key, Object>(
@@ -232,6 +234,7 @@ public UnifiedQueryContext build() {
232234
case SQL -> UnifiedSqlSpec.extended();
233235
case PPL -> UnifiedPplSpec.create();
234236
};
237+
235238
Settings settings = buildSettings();
236239
CalcitePlanContext planContext =
237240
CalcitePlanContext.create(

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: 34 additions & 8 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,19 +157,42 @@ 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 applyClusterOverrides(UnifiedQueryContext.builder().language(queryType)).build();
159162
}
160163

161164
private UnifiedQueryContext buildContext(QueryType queryType, boolean profiling) {
162-
return UnifiedQueryContext.builder()
163-
.language(queryType)
164-
.catalog(SCHEMA_NAME, OpenSearchSchemaBuilder.buildSchema(clusterService.state()))
165-
.defaultNamespace(SCHEMA_NAME)
166-
.profiling(profiling)
165+
return applyClusterOverrides(
166+
UnifiedQueryContext.builder()
167+
.language(queryType)
168+
.catalog(SCHEMA_NAME, OpenSearchSchemaBuilder.buildSchema(clusterService.state()))
169+
.defaultNamespace(SCHEMA_NAME)
170+
.profiling(profiling))
167171
.build();
168172
}
169173

174+
/**
175+
* Routes operator-configured cluster overrides into the builder via the existing {@code
176+
* setting(String, Object)} API, keeping {@link UnifiedQueryContext} decoupled from any specific
177+
* {@link org.opensearch.sql.common.setting.Settings} implementation.
178+
*
179+
* <p>Currently scoped to {@code plugins.ppl.rex.max_match.limit} — required so the unified path
180+
* honors {@code _cluster/settings} updates for {@code rex max_match} (CalciteRexCommandIT's
181+
* testRexMaxMatchConfigurableLimit). Add keys here if a future PR / IT depends on cluster-side
182+
* fidelity for one of the other planning settings.
183+
*/
184+
private UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Builder builder) {
185+
Object rexLimit =
186+
pluginSettings.getSettingValue(
187+
org.opensearch.sql.common.setting.Settings.Key.PPL_REX_MAX_MATCH_LIMIT);
188+
if (rexLimit != null) {
189+
builder.setting(
190+
org.opensearch.sql.common.setting.Settings.Key.PPL_REX_MAX_MATCH_LIMIT.getKeyValue(),
191+
rexLimit);
192+
}
193+
return builder;
194+
}
195+
170196
/**
171197
* Extract the source index name by parsing the query and visiting the AST to find the Relation
172198
* node. Uses the context's parser which supports both PPL and SQL.

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
/**

plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.junit.Test;
1515
import org.opensearch.analytics.exec.QueryPlanExecutor;
1616
import org.opensearch.cluster.service.ClusterService;
17+
import org.opensearch.sql.common.setting.Settings;
1718
import org.opensearch.sql.executor.QueryType;
1819
import org.opensearch.transport.client.node.NodeClient;
1920

@@ -30,7 +31,8 @@ public void setUp() {
3031
@SuppressWarnings("unchecked")
3132
QueryPlanExecutor<RelNode, Iterable<Object[]>> executor = mock(QueryPlanExecutor.class);
3233
action =
33-
new RestUnifiedQueryAction(mock(NodeClient.class), mock(ClusterService.class), executor);
34+
new RestUnifiedQueryAction(
35+
mock(NodeClient.class), mock(ClusterService.class), executor, mock(Settings.class));
3436
}
3537

3638
@Test

0 commit comments

Comments
 (0)