Skip to content

Commit 1a401a3

Browse files
committed
Forward cluster planning settings to the Analytics Engine unified query path
The Analytics Engine (unified query) path silently ignored several cluster settings, always planning against UnifiedQueryContext.Builder's hardcoded seed value regardless of what the operator configured. The default (non-AE) pipeline honored them correctly. Root cause: RestUnifiedQueryAction.applyClusterOverrides() forwarded only a hand-picked subset of cluster settings into the UnifiedQueryContext, while the builder independently seeds a default settings map. Any planning setting present in the seed map but absent from the forward list regressed to its default -- a two-lists-drift defect. Settings fixed (all verified to reach the plan context only with this change): plugins.query.size_limit pinned to 10000 plugins.ppl.pattern.method pinned to SIMPLE_PATTERN plugins.ppl.pattern.mode pinned to LABEL plugins.ppl.pattern.max.sample.count pinned to 10 plugins.ppl.pattern.buffer.limit pinned to 100000 plugins.ppl.pattern.show.numbered.token pinned to false plugins.ppl.values.max.limit read back null, so the configured cap on values() never applied Each has a cluster-side default identical to the seeded one, so behavior is unchanged unless an operator explicitly configured the setting -- at which point the configured value now takes effect. Replaces the hand-maintained forwardClusterSetting calls with a single FORWARDED_CLUSTER_SETTINGS allow-list, and adds a drift guard (everySeededPlanningSettingIsClassified) asserting every key the builder seeds is either forwarded or explicitly documented as excluded, so this defect cannot recur silently. Deliberately not forwarded: plugins.calcite.enabled -- the unified path is Calcite-based by definition and must force it on. plugins.ppl.subsearch.maxout / plugins.ppl.join.subsearch_maxout -- seeded to 0 (unlimited) on purpose to keep LogicalSystemLimit out of plans built by external consumers of the unified query API. These do diverge from the cluster defaults (10000 / 50000) even when unconfigured; whether the in-cluster REST path should override that is a separate behavioral decision, tracked in opensearch-project#5735. Testing: the four new unit tests each fail without this change (expected:<BRAIN> but was:<SIMPLE_PATTERN>, expected:<100> but was:<null>, expected:<500> but was:<10000>) and pass with it. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent 802152e commit 1a401a3

2 files changed

Lines changed: 153 additions & 14 deletions

File tree

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

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010
import static org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.SQL_WORKER_THREAD_POOL_NAME;
1111
import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY;
1212

13+
import com.google.common.annotations.VisibleForTesting;
1314
import com.google.gson.JsonElement;
1415
import com.google.gson.JsonObject;
1516
import com.google.gson.JsonParser;
17+
import java.util.List;
1618
import java.util.Map;
1719
import java.util.Optional;
1820
import org.apache.calcite.rel.RelNode;
@@ -40,6 +42,7 @@
4042
import org.opensearch.sql.calcite.CalcitePlanContext;
4143
import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit;
4244
import org.opensearch.sql.common.response.ResponseListener;
45+
import org.opensearch.sql.common.setting.Settings.Key;
4346
import org.opensearch.sql.executor.ExecutionEngine.QueryResponse;
4447
import org.opensearch.sql.executor.QueryType;
4548
import org.opensearch.sql.executor.analytics.AnalyticsExecutionEngine;
@@ -346,26 +349,52 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task
346349
return new QueryRequestContext(ctx.clusterState(), ctx.schema(), ctx.querySource(), parentTask);
347350
}
348351

352+
/**
353+
* Cluster settings whose live values are forwarded into every {@link UnifiedQueryContext} so the
354+
* Analytics Engine plans against the same configuration as the default pipeline. This list is the
355+
* single source of truth for cluster fidelity on the unified path: any planning setting the AE
356+
* path must honor belongs here, otherwise {@link UnifiedQueryContext.Builder}'s hardcoded default
357+
* silently wins and the configured cluster value is ignored.
358+
*
359+
* <p>{@link Key#CALCITE_ENGINE_ENABLED} is deliberately excluded — the unified path is
360+
* Calcite-based by definition and forces it {@code true} regardless of the cluster value. {@link
361+
* Key#PPL_SUBSEARCH_MAXOUT} and {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} are excluded too: the
362+
* builder seeds them to {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out
363+
* of plans built by external consumers of the unified query API, and overriding that on the
364+
* in-cluster path is a separate behavioral decision tracked by
365+
* https://github.com/opensearch-project/sql/issues/5735.
366+
*
367+
* <p>{@code RestUnifiedQueryActionTest#everySeededPlanningSettingIsClassified} pins those three
368+
* exclusions, so a key added to the builder's seed map cannot silently regress to its hardcoded
369+
* default without failing a test.
370+
*/
371+
@VisibleForTesting
372+
static final List<Key> FORWARDED_CLUSTER_SETTINGS =
373+
List.of(
374+
Key.QUERY_SIZE_LIMIT,
375+
Key.PPL_REX_MAX_MATCH_LIMIT,
376+
Key.PPL_SYNTAX_LEGACY_PREFERRED,
377+
Key.MAX_EXPRESSION_DEPTH,
378+
Key.PPL_VALUES_MAX_LIMIT,
379+
Key.PATTERN_METHOD,
380+
Key.PATTERN_MODE,
381+
Key.PATTERN_MAX_SAMPLE_COUNT,
382+
Key.PATTERN_BUFFER_LIMIT,
383+
Key.PATTERN_SHOW_NUMBERED_TOKEN);
384+
349385
/**
350386
* Routes operator-configured cluster overrides into the builder via the existing {@code
351387
* setting(String, Object)} API, keeping {@link UnifiedQueryContext} decoupled from any specific
352-
* {@link org.opensearch.sql.common.setting.Settings} implementation.
353-
*
354-
* <p>Add keys here if a future PR / IT depends on cluster-side fidelity for one of the other
355-
* planning settings.
388+
* {@link org.opensearch.sql.common.setting.Settings} implementation. The forwarded keys are
389+
* {@link #FORWARDED_CLUSTER_SETTINGS}.
356390
*/
357-
private UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Builder builder) {
358-
forwardClusterSetting(
359-
builder, org.opensearch.sql.common.setting.Settings.Key.PPL_REX_MAX_MATCH_LIMIT);
360-
forwardClusterSetting(
361-
builder, org.opensearch.sql.common.setting.Settings.Key.PPL_SYNTAX_LEGACY_PREFERRED);
362-
forwardClusterSetting(
363-
builder, org.opensearch.sql.common.setting.Settings.Key.MAX_EXPRESSION_DEPTH);
391+
@VisibleForTesting
392+
UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Builder builder) {
393+
FORWARDED_CLUSTER_SETTINGS.forEach(key -> forwardClusterSetting(builder, key));
364394
return builder;
365395
}
366396

367-
private void forwardClusterSetting(
368-
UnifiedQueryContext.Builder builder, org.opensearch.sql.common.setting.Settings.Key key) {
397+
private void forwardClusterSetting(UnifiedQueryContext.Builder builder, Key key) {
369398
Object value = pluginSettings.getSettingValue(key);
370399
if (value != null) {
371400
builder.setting(key.getKeyValue(), value);

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

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55

66
package org.opensearch.sql.plugin.rest;
77

8+
import static org.junit.Assert.assertEquals;
89
import static org.junit.Assert.assertFalse;
910
import static org.junit.Assert.assertTrue;
1011
import static org.mockito.Mockito.mock;
1112
import static org.mockito.Mockito.when;
13+
import static org.opensearch.sql.plugin.rest.RestUnifiedQueryAction.FORWARDED_CLUSTER_SETTINGS;
1214

15+
import java.util.List;
16+
import java.util.Map;
1317
import org.apache.calcite.rel.RelNode;
1418
import org.junit.Before;
1519
import org.junit.Test;
@@ -22,6 +26,8 @@
2226
import org.opensearch.common.settings.Settings;
2327
import org.opensearch.index.IndexSettings;
2428
import org.opensearch.indices.IndicesService;
29+
import org.opensearch.sql.api.UnifiedQueryContext;
30+
import org.opensearch.sql.common.setting.Settings.Key;
2531
import org.opensearch.sql.executor.QueryType;
2632
import org.opensearch.transport.client.node.NodeClient;
2733

@@ -33,6 +39,7 @@ public class RestUnifiedQueryActionTest {
3339

3440
private ClusterService clusterService;
3541
private Metadata metadata;
42+
private org.opensearch.sql.common.setting.Settings pluginSettings;
3643
private RestUnifiedQueryAction action;
3744

3845
@Before
@@ -46,6 +53,7 @@ public void setUp() {
4653
// path is only exercised when this returns something other than "composite".
4754
when(clusterService.getSettings()).thenReturn(Settings.EMPTY);
4855

56+
pluginSettings = mock(org.opensearch.sql.common.setting.Settings.class);
4957
@SuppressWarnings("unchecked")
5058
QueryPlanExecutor<RelNode, Iterable<Object[]>> executor = mock(QueryPlanExecutor.class);
5159
action =
@@ -54,7 +62,7 @@ public void setUp() {
5462
clusterService,
5563
executor,
5664
mock(EngineContextProvider.class),
57-
mock(org.opensearch.sql.common.setting.Settings.class),
65+
pluginSettings,
5866
new org.opensearch.sql.executor.DirectExecutionDispatcher());
5967
}
6068

@@ -237,6 +245,108 @@ public void pplUnparseableQueryRoutesToAnalyticsUnderClusterComposite() {
237245
assertTrue(action.isAnalyticsIndex("source = parquet_logs | | fields ts", QueryType.PPL));
238246
}
239247

248+
@Test
249+
public void clusterQuerySizeLimitReachesAnalyticsContext() {
250+
// Regression: the AE path pinned QUERY_SIZE_LIMIT to the builder's hardcoded default (10000),
251+
// silently ignoring the configured cluster value. Forwarding must carry the live value through
252+
// to the plan context, since addQuerySizeLimit reads it from there.
253+
when(pluginSettings.getSettingValue(Key.QUERY_SIZE_LIMIT)).thenReturn(500);
254+
255+
assertEquals(
256+
"Cluster plugins.query.size_limit must reach the AE plan context",
257+
Integer.valueOf(500),
258+
buildAnalyticsContext().getPlanContext().sysLimit.querySizeLimit());
259+
}
260+
261+
@Test
262+
public void calciteEngineEnabledNotOverriddenByCluster() {
263+
// CALCITE_ENGINE_ENABLED is deliberately excluded from forwarding: the unified path is
264+
// Calcite-based by definition and must stay true even if the cluster disables it.
265+
when(pluginSettings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false);
266+
267+
assertEquals(
268+
"Unified path must force Calcite on regardless of the cluster setting",
269+
Boolean.TRUE,
270+
buildAnalyticsContext().getSettings().getSettingValue(Key.CALCITE_ENGINE_ENABLED));
271+
}
272+
273+
/**
274+
* Planning settings {@link UnifiedQueryContext.Builder} seeds that the REST handler deliberately
275+
* does not forward, each with the reason it stays hardcoded on the unified path.
276+
*
277+
* <ul>
278+
* <li>{@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition.
279+
* <li>{@link Key#PPL_SUBSEARCH_MAXOUT} / {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to
280+
* {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out of plans built
281+
* by external consumers of the unified query API. Whether the in-cluster path should
282+
* override that is tracked by https://github.com/opensearch-project/sql/issues/5735.
283+
* </ul>
284+
*/
285+
private static final List<Key> DELIBERATELY_NOT_FORWARDED =
286+
List.of(Key.CALCITE_ENGINE_ENABLED, Key.PPL_SUBSEARCH_MAXOUT, Key.PPL_JOIN_SUBSEARCH_MAXOUT);
287+
288+
/**
289+
* Drift guard for the defect class this forwarding exists to prevent: the builder's seed map and
290+
* the handler's forward list are maintained independently, so a planning setting seeded but not
291+
* forwarded silently regresses to its hardcoded default (this is how {@code
292+
* plugins.query.size_limit} came to be ignored on the AE path). Every seeded key must therefore
293+
* be classified — forwarded, or explicitly excluded with a reason.
294+
*/
295+
@Test
296+
public void everySeededPlanningSettingIsClassified() {
297+
UnifiedQueryContext defaults = UnifiedQueryContext.builder().language(QueryType.PPL).build();
298+
299+
for (Object entry : defaults.getSettings().getSettings()) {
300+
Key seeded = ((Map.Entry<Key, ?>) entry).getKey();
301+
assertTrue(
302+
"Setting "
303+
+ seeded.getKeyValue()
304+
+ " is seeded with a hardcoded default by UnifiedQueryContext.Builder but is neither"
305+
+ " forwarded from the cluster nor listed in DELIBERATELY_NOT_FORWARDED. Add it to"
306+
+ " RestUnifiedQueryAction.FORWARDED_CLUSTER_SETTINGS so the configured cluster value"
307+
+ " reaches the Analytics Engine, or document why it must stay hardcoded.",
308+
FORWARDED_CLUSTER_SETTINGS.contains(seeded)
309+
|| DELIBERATELY_NOT_FORWARDED.contains(seeded));
310+
}
311+
}
312+
313+
@Test
314+
public void clusterPatternSettingsReachAnalyticsContext() {
315+
// patterns command defaults are read straight off the context's settings in AstBuilder, so a
316+
// cluster-configured method/mode/limit must be visible there rather than the seeded default.
317+
when(pluginSettings.getSettingValue(Key.PATTERN_METHOD)).thenReturn("BRAIN");
318+
when(pluginSettings.getSettingValue(Key.PATTERN_MODE)).thenReturn("AGGREGATION");
319+
when(pluginSettings.getSettingValue(Key.PATTERN_MAX_SAMPLE_COUNT)).thenReturn(42);
320+
when(pluginSettings.getSettingValue(Key.PATTERN_BUFFER_LIMIT)).thenReturn(60000);
321+
when(pluginSettings.getSettingValue(Key.PATTERN_SHOW_NUMBERED_TOKEN)).thenReturn(true);
322+
323+
org.opensearch.sql.common.setting.Settings forwarded = buildAnalyticsContext().getSettings();
324+
325+
assertEquals("BRAIN", forwarded.getSettingValue(Key.PATTERN_METHOD));
326+
assertEquals("AGGREGATION", forwarded.getSettingValue(Key.PATTERN_MODE));
327+
assertEquals(Integer.valueOf(42), forwarded.getSettingValue(Key.PATTERN_MAX_SAMPLE_COUNT));
328+
assertEquals(Integer.valueOf(60000), forwarded.getSettingValue(Key.PATTERN_BUFFER_LIMIT));
329+
assertEquals(Boolean.TRUE, forwarded.getSettingValue(Key.PATTERN_SHOW_NUMBERED_TOKEN));
330+
}
331+
332+
@Test
333+
public void clusterValuesMaxLimitReachesAnalyticsContext() {
334+
// PPL_VALUES_MAX_LIMIT is not seeded at all, so without forwarding AstExpressionBuilder reads
335+
// null and falls back to unlimited — the configured cap on values() never applies.
336+
when(pluginSettings.getSettingValue(Key.PPL_VALUES_MAX_LIMIT)).thenReturn(100);
337+
338+
assertEquals(
339+
Integer.valueOf(100),
340+
buildAnalyticsContext().getSettings().getSettingValue(Key.PPL_VALUES_MAX_LIMIT));
341+
}
342+
343+
/** Builds the context the AE path plans against, with the mocked cluster settings applied. */
344+
private UnifiedQueryContext buildAnalyticsContext() {
345+
return action
346+
.applyClusterOverrides(UnifiedQueryContext.builder().language(QueryType.PPL))
347+
.build();
348+
}
349+
240350
private void enableClusterComposite() {
241351
when(clusterService.getSettings())
242352
.thenReturn(

0 commit comments

Comments
 (0)