From 1a401a346a37272c43fae1ba3500a2aa03893e37 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Mon, 6 Jul 2026 15:12:55 -0700 Subject: [PATCH 1/4] 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 #5735. Testing: the four new unit tests each fail without this change (expected: but was:, expected:<100> but was:, expected:<500> but was:<10000>) and pass with it. Signed-off-by: Jialiang Liang --- .../plugin/rest/RestUnifiedQueryAction.java | 55 +++++++-- .../rest/RestUnifiedQueryActionTest.java | 112 +++++++++++++++++- 2 files changed, 153 insertions(+), 14 deletions(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java index 531d180f7cc..f2761f77f6a 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java @@ -10,9 +10,11 @@ import static org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.SQL_WORKER_THREAD_POOL_NAME; import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; +import com.google.common.annotations.VisibleForTesting; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonParser; +import java.util.List; import java.util.Map; import java.util.Optional; import org.apache.calcite.rel.RelNode; @@ -40,6 +42,7 @@ import org.opensearch.sql.calcite.CalcitePlanContext; import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit; import org.opensearch.sql.common.response.ResponseListener; +import org.opensearch.sql.common.setting.Settings.Key; import org.opensearch.sql.executor.ExecutionEngine.QueryResponse; import org.opensearch.sql.executor.QueryType; import org.opensearch.sql.executor.analytics.AnalyticsExecutionEngine; @@ -346,26 +349,52 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task return new QueryRequestContext(ctx.clusterState(), ctx.schema(), ctx.querySource(), parentTask); } + /** + * Cluster settings whose live values are forwarded into every {@link UnifiedQueryContext} so the + * Analytics Engine plans against the same configuration as the default pipeline. This list is the + * single source of truth for cluster fidelity on the unified path: any planning setting the AE + * path must honor belongs here, otherwise {@link UnifiedQueryContext.Builder}'s hardcoded default + * silently wins and the configured cluster value is ignored. + * + *

{@link Key#CALCITE_ENGINE_ENABLED} is deliberately excluded — the unified path is + * Calcite-based by definition and forces it {@code true} regardless of the cluster value. {@link + * Key#PPL_SUBSEARCH_MAXOUT} and {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} are excluded too: the + * builder seeds them to {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out + * of plans built by external consumers of the unified query API, and overriding that on the + * in-cluster path is a separate behavioral decision tracked by + * https://github.com/opensearch-project/sql/issues/5735. + * + *

{@code RestUnifiedQueryActionTest#everySeededPlanningSettingIsClassified} pins those three + * exclusions, so a key added to the builder's seed map cannot silently regress to its hardcoded + * default without failing a test. + */ + @VisibleForTesting + static final List FORWARDED_CLUSTER_SETTINGS = + List.of( + Key.QUERY_SIZE_LIMIT, + Key.PPL_REX_MAX_MATCH_LIMIT, + Key.PPL_SYNTAX_LEGACY_PREFERRED, + Key.MAX_EXPRESSION_DEPTH, + Key.PPL_VALUES_MAX_LIMIT, + Key.PATTERN_METHOD, + Key.PATTERN_MODE, + Key.PATTERN_MAX_SAMPLE_COUNT, + Key.PATTERN_BUFFER_LIMIT, + Key.PATTERN_SHOW_NUMBERED_TOKEN); + /** * Routes operator-configured cluster overrides into the builder via the existing {@code * setting(String, Object)} API, keeping {@link UnifiedQueryContext} decoupled from any specific - * {@link org.opensearch.sql.common.setting.Settings} implementation. - * - *

Add keys here if a future PR / IT depends on cluster-side fidelity for one of the other - * planning settings. + * {@link org.opensearch.sql.common.setting.Settings} implementation. The forwarded keys are + * {@link #FORWARDED_CLUSTER_SETTINGS}. */ - private UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Builder builder) { - forwardClusterSetting( - builder, org.opensearch.sql.common.setting.Settings.Key.PPL_REX_MAX_MATCH_LIMIT); - forwardClusterSetting( - builder, org.opensearch.sql.common.setting.Settings.Key.PPL_SYNTAX_LEGACY_PREFERRED); - forwardClusterSetting( - builder, org.opensearch.sql.common.setting.Settings.Key.MAX_EXPRESSION_DEPTH); + @VisibleForTesting + UnifiedQueryContext.Builder applyClusterOverrides(UnifiedQueryContext.Builder builder) { + FORWARDED_CLUSTER_SETTINGS.forEach(key -> forwardClusterSetting(builder, key)); return builder; } - private void forwardClusterSetting( - UnifiedQueryContext.Builder builder, org.opensearch.sql.common.setting.Settings.Key key) { + private void forwardClusterSetting(UnifiedQueryContext.Builder builder, Key key) { Object value = pluginSettings.getSettingValue(key); if (value != null) { builder.setting(key.getKeyValue(), value); diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java index 516c31940c3..e2243a9b4ac 100644 --- a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java +++ b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java @@ -5,11 +5,15 @@ package org.opensearch.sql.plugin.rest; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.opensearch.sql.plugin.rest.RestUnifiedQueryAction.FORWARDED_CLUSTER_SETTINGS; +import java.util.List; +import java.util.Map; import org.apache.calcite.rel.RelNode; import org.junit.Before; import org.junit.Test; @@ -22,6 +26,8 @@ import org.opensearch.common.settings.Settings; import org.opensearch.index.IndexSettings; import org.opensearch.indices.IndicesService; +import org.opensearch.sql.api.UnifiedQueryContext; +import org.opensearch.sql.common.setting.Settings.Key; import org.opensearch.sql.executor.QueryType; import org.opensearch.transport.client.node.NodeClient; @@ -33,6 +39,7 @@ public class RestUnifiedQueryActionTest { private ClusterService clusterService; private Metadata metadata; + private org.opensearch.sql.common.setting.Settings pluginSettings; private RestUnifiedQueryAction action; @Before @@ -46,6 +53,7 @@ public void setUp() { // path is only exercised when this returns something other than "composite". when(clusterService.getSettings()).thenReturn(Settings.EMPTY); + pluginSettings = mock(org.opensearch.sql.common.setting.Settings.class); @SuppressWarnings("unchecked") QueryPlanExecutor> executor = mock(QueryPlanExecutor.class); action = @@ -54,7 +62,7 @@ public void setUp() { clusterService, executor, mock(EngineContextProvider.class), - mock(org.opensearch.sql.common.setting.Settings.class), + pluginSettings, new org.opensearch.sql.executor.DirectExecutionDispatcher()); } @@ -237,6 +245,108 @@ public void pplUnparseableQueryRoutesToAnalyticsUnderClusterComposite() { assertTrue(action.isAnalyticsIndex("source = parquet_logs | | fields ts", QueryType.PPL)); } + @Test + public void clusterQuerySizeLimitReachesAnalyticsContext() { + // Regression: the AE path pinned QUERY_SIZE_LIMIT to the builder's hardcoded default (10000), + // silently ignoring the configured cluster value. Forwarding must carry the live value through + // to the plan context, since addQuerySizeLimit reads it from there. + when(pluginSettings.getSettingValue(Key.QUERY_SIZE_LIMIT)).thenReturn(500); + + assertEquals( + "Cluster plugins.query.size_limit must reach the AE plan context", + Integer.valueOf(500), + buildAnalyticsContext().getPlanContext().sysLimit.querySizeLimit()); + } + + @Test + public void calciteEngineEnabledNotOverriddenByCluster() { + // CALCITE_ENGINE_ENABLED is deliberately excluded from forwarding: the unified path is + // Calcite-based by definition and must stay true even if the cluster disables it. + when(pluginSettings.getSettingValue(Key.CALCITE_ENGINE_ENABLED)).thenReturn(false); + + assertEquals( + "Unified path must force Calcite on regardless of the cluster setting", + Boolean.TRUE, + buildAnalyticsContext().getSettings().getSettingValue(Key.CALCITE_ENGINE_ENABLED)); + } + + /** + * Planning settings {@link UnifiedQueryContext.Builder} seeds that the REST handler deliberately + * does not forward, each with the reason it stays hardcoded on the unified path. + * + *

    + *
  • {@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition. + *
  • {@link Key#PPL_SUBSEARCH_MAXOUT} / {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to + * {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out of plans built + * by external consumers of the unified query API. Whether the in-cluster path should + * override that is tracked by https://github.com/opensearch-project/sql/issues/5735. + *
+ */ + private static final List DELIBERATELY_NOT_FORWARDED = + List.of(Key.CALCITE_ENGINE_ENABLED, Key.PPL_SUBSEARCH_MAXOUT, Key.PPL_JOIN_SUBSEARCH_MAXOUT); + + /** + * Drift guard for the defect class this forwarding exists to prevent: the builder's seed map and + * the handler's forward list are maintained independently, so a planning setting seeded but not + * forwarded silently regresses to its hardcoded default (this is how {@code + * plugins.query.size_limit} came to be ignored on the AE path). Every seeded key must therefore + * be classified — forwarded, or explicitly excluded with a reason. + */ + @Test + public void everySeededPlanningSettingIsClassified() { + UnifiedQueryContext defaults = UnifiedQueryContext.builder().language(QueryType.PPL).build(); + + for (Object entry : defaults.getSettings().getSettings()) { + Key seeded = ((Map.Entry) entry).getKey(); + assertTrue( + "Setting " + + seeded.getKeyValue() + + " is seeded with a hardcoded default by UnifiedQueryContext.Builder but is neither" + + " forwarded from the cluster nor listed in DELIBERATELY_NOT_FORWARDED. Add it to" + + " RestUnifiedQueryAction.FORWARDED_CLUSTER_SETTINGS so the configured cluster value" + + " reaches the Analytics Engine, or document why it must stay hardcoded.", + FORWARDED_CLUSTER_SETTINGS.contains(seeded) + || DELIBERATELY_NOT_FORWARDED.contains(seeded)); + } + } + + @Test + public void clusterPatternSettingsReachAnalyticsContext() { + // patterns command defaults are read straight off the context's settings in AstBuilder, so a + // cluster-configured method/mode/limit must be visible there rather than the seeded default. + when(pluginSettings.getSettingValue(Key.PATTERN_METHOD)).thenReturn("BRAIN"); + when(pluginSettings.getSettingValue(Key.PATTERN_MODE)).thenReturn("AGGREGATION"); + when(pluginSettings.getSettingValue(Key.PATTERN_MAX_SAMPLE_COUNT)).thenReturn(42); + when(pluginSettings.getSettingValue(Key.PATTERN_BUFFER_LIMIT)).thenReturn(60000); + when(pluginSettings.getSettingValue(Key.PATTERN_SHOW_NUMBERED_TOKEN)).thenReturn(true); + + org.opensearch.sql.common.setting.Settings forwarded = buildAnalyticsContext().getSettings(); + + assertEquals("BRAIN", forwarded.getSettingValue(Key.PATTERN_METHOD)); + assertEquals("AGGREGATION", forwarded.getSettingValue(Key.PATTERN_MODE)); + assertEquals(Integer.valueOf(42), forwarded.getSettingValue(Key.PATTERN_MAX_SAMPLE_COUNT)); + assertEquals(Integer.valueOf(60000), forwarded.getSettingValue(Key.PATTERN_BUFFER_LIMIT)); + assertEquals(Boolean.TRUE, forwarded.getSettingValue(Key.PATTERN_SHOW_NUMBERED_TOKEN)); + } + + @Test + public void clusterValuesMaxLimitReachesAnalyticsContext() { + // PPL_VALUES_MAX_LIMIT is not seeded at all, so without forwarding AstExpressionBuilder reads + // null and falls back to unlimited — the configured cap on values() never applies. + when(pluginSettings.getSettingValue(Key.PPL_VALUES_MAX_LIMIT)).thenReturn(100); + + assertEquals( + Integer.valueOf(100), + buildAnalyticsContext().getSettings().getSettingValue(Key.PPL_VALUES_MAX_LIMIT)); + } + + /** Builds the context the AE path plans against, with the mocked cluster settings applied. */ + private UnifiedQueryContext buildAnalyticsContext() { + return action + .applyClusterOverrides(UnifiedQueryContext.builder().language(QueryType.PPL)) + .build(); + } + private void enableClusterComposite() { when(clusterService.getSettings()) .thenReturn( From 4ca83de6706fcd641f1ce1581c3891346a1e8091 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Tue, 1 Sep 2026 11:50:58 -0700 Subject: [PATCH 2/4] Do not forward PPL_VALUES_MAX_LIMIT to the Analytics Engine path Verified against a live composite/parquet analytics-engine cluster: forwarding plugins.ppl.values.max.limit makes the AE route strictly worse, not better. The cap is applied by attaching a `limit` argument to the values() aggregate, which lowers to array_agg(DISTINCT x, limit). The DataFusion backend has no binding for that two-argument form, so once the setting actually reaches the parser the query fails outright: UnsupportedOperationException: Unable to find binding for call array_agg(DISTINCT $0, $1) served to the client as HTTP 500 "Internal error". Today the same query merely ignores the cap and returns all values. Turning a silent no-op into a hard failure is a regression, so the key stays unforwarded until the backend can bind the limited form -- the gap already tracked by Capability.VALUES_LIMIT_NOT_HONORED. The remaining forwarded settings were confirmed end to end on the same cluster, baseline build vs fixed build: plugins.query.size_limit=2 6 rows -> 2 rows plugins.query.size_limit=4 6 rows -> 4 rows plugins.ppl.pattern.mode=AGGREGATION baseline: 6 rows, schema [age, name, patterns_field] (ignored) fixed: 1 row, schema [patterns_field, pattern_count, sample_logs] Signed-off-by: Jialiang Liang Signed-off-by: Jialiang Liang --- .../sql/plugin/rest/RestUnifiedQueryAction.java | 12 ++++++++++-- .../sql/plugin/rest/RestUnifiedQueryActionTest.java | 12 +++++++----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java index f2761f77f6a..0e94299dd86 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java @@ -364,7 +364,16 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task * in-cluster path is a separate behavioral decision tracked by * https://github.com/opensearch-project/sql/issues/5735. * - *

{@code RestUnifiedQueryActionTest#everySeededPlanningSettingIsClassified} pins those three + *

{@link Key#PPL_VALUES_MAX_LIMIT} is excluded for a different reason: forwarding it makes + * things worse rather than better. The cap is applied by attaching a {@code limit} argument to + * the {@code values()} aggregate, which lowers to {@code array_agg(DISTINCT x, limit)} — a + * two-argument form the analytics-engine backend has no binding for. Verified against a live + * composite/parquet cluster: with the setting forwarded, {@code stats values(f)} fails with + * {@code UnsupportedOperationException: Unable to find binding for call array_agg(DISTINCT $0, + * $1)} (HTTP 500), where today it merely ignores the cap. Honoring the cap on this route needs + * backend support first — see {@code Capability.VALUES_LIMIT_NOT_HONORED}. + * + *

{@code RestUnifiedQueryActionTest#everySeededPlanningSettingIsClassified} pins the seeded * exclusions, so a key added to the builder's seed map cannot silently regress to its hardcoded * default without failing a test. */ @@ -375,7 +384,6 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task Key.PPL_REX_MAX_MATCH_LIMIT, Key.PPL_SYNTAX_LEGACY_PREFERRED, Key.MAX_EXPRESSION_DEPTH, - Key.PPL_VALUES_MAX_LIMIT, Key.PATTERN_METHOD, Key.PATTERN_MODE, Key.PATTERN_MAX_SAMPLE_COUNT, diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java index e2243a9b4ac..8d43625d9f7 100644 --- a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java +++ b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java @@ -7,6 +7,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -330,13 +331,14 @@ public void clusterPatternSettingsReachAnalyticsContext() { } @Test - public void clusterValuesMaxLimitReachesAnalyticsContext() { - // PPL_VALUES_MAX_LIMIT is not seeded at all, so without forwarding AstExpressionBuilder reads - // null and falls back to unlimited — the configured cap on values() never applies. + public void valuesMaxLimitIsNotForwarded() { + // Forwarding PPL_VALUES_MAX_LIMIT would attach a `limit` argument to values(), lowering to + // array_agg(DISTINCT x, limit) — a form the analytics-engine backend cannot bind, turning a + // silently-ignored cap into a 500. Verified against a live composite/parquet cluster. when(pluginSettings.getSettingValue(Key.PPL_VALUES_MAX_LIMIT)).thenReturn(100); - assertEquals( - Integer.valueOf(100), + assertNull( + "Forwarding values.max.limit breaks values() on the AE route; it must stay unset", buildAnalyticsContext().getSettings().getSettingValue(Key.PPL_VALUES_MAX_LIMIT)); } From ab190734d4b3208569ff2f8e72488b1f14b5a3f3 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 2 Sep 2026 13:50:54 -0700 Subject: [PATCH 3/4] Review: shorten the forwarding javadoc and complete the exclusion list Addresses review feedback on the FORWARDED_CLUSTER_SETTINGS javadoc. The prose had grown to ~28 lines and, worse, was incomplete: it named only four exclusions and omitted CALCITE_SUPPORT_ALL_JOIN_TYPES entirely. Re-derived the full set by tracing every getSettingValue reachable from the unified context's Settings -- SysLimit.fromSettings, AstBuilder / AstExpressionBuilder / AstBuildGuard (the parsers the context builds), UnresolvedPlanHelper, and CalcitePlanContext. Fourteen keys are read on that path: nine forwarded, five deliberately not. CALCITE_ENGINE_ENABLED unified path is Calcite by definition PPL_SUBSEARCH_MAXOUT seeded unlimited on purpose (#5735) PPL_JOIN_SUBSEARCH_MAXOUT likewise (#5735) PPL_VALUES_MAX_LIMIT forwarding 500s the route (#5736) CALCITE_SUPPORT_ALL_JOIN_TYPES never seeded; guard inactive (#5734) The javadoc is now a compact bulleted list of those five with an issue reference each -- shorter than before and, unlike before, complete. Rather than leaving that claim as prose, DELIBERATELY_NOT_FORWARDED in the test now carries all five and documentedExclusionsAreNotForwarded asserts none of them reaches the plan context, using a sentinel value so "forwarded" is distinguishable from "seeded" and from "absent". Verified the guard bites: adding PPL_VALUES_MAX_LIMIT to the forward list fails with "plugins.ppl.values.max.limit must not be forwarded ... Actual: -12345". This replaces the narrower valuesMaxLimitIsNotForwarded test. Signed-off-by: Jialiang Liang Signed-off-by: Jialiang Liang --- .../plugin/rest/RestUnifiedQueryAction.java | 44 ++++++++-------- .../rest/RestUnifiedQueryActionTest.java | 52 +++++++++++++------ 2 files changed, 57 insertions(+), 39 deletions(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java index 0e94299dd86..6592e94e974 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java @@ -350,32 +350,30 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task } /** - * Cluster settings whose live values are forwarded into every {@link UnifiedQueryContext} so the - * Analytics Engine plans against the same configuration as the default pipeline. This list is the - * single source of truth for cluster fidelity on the unified path: any planning setting the AE - * path must honor belongs here, otherwise {@link UnifiedQueryContext.Builder}'s hardcoded default - * silently wins and the configured cluster value is ignored. + * Cluster settings forwarded into every {@link UnifiedQueryContext}, so the Analytics Engine + * plans against the same configuration as the default pipeline. Any planning setting the AE path + * must honor belongs here; otherwise the value {@link UnifiedQueryContext.Builder} seeds silently + * wins and the configured cluster value is ignored. * - *

{@link Key#CALCITE_ENGINE_ENABLED} is deliberately excluded — the unified path is - * Calcite-based by definition and forces it {@code true} regardless of the cluster value. {@link - * Key#PPL_SUBSEARCH_MAXOUT} and {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} are excluded too: the - * builder seeds them to {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out - * of plans built by external consumers of the unified query API, and overriding that on the - * in-cluster path is a separate behavioral decision tracked by - * https://github.com/opensearch-project/sql/issues/5735. + *

The AE path reads exactly five other settings, each deliberately left out: * - *

{@link Key#PPL_VALUES_MAX_LIMIT} is excluded for a different reason: forwarding it makes - * things worse rather than better. The cap is applied by attaching a {@code limit} argument to - * the {@code values()} aggregate, which lowers to {@code array_agg(DISTINCT x, limit)} — a - * two-argument form the analytics-engine backend has no binding for. Verified against a live - * composite/parquet cluster: with the setting forwarded, {@code stats values(f)} fails with - * {@code UnsupportedOperationException: Unable to find binding for call array_agg(DISTINCT $0, - * $1)} (HTTP 500), where today it merely ignores the cap. Honoring the cap on this route needs - * backend support first — see {@code Capability.VALUES_LIMIT_NOT_HONORED}. + *

    + *
  • {@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition and + * must force it on. + *
  • {@link Key#PPL_SUBSEARCH_MAXOUT}, {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to + * {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out of plans built + * by external consumers of the unified query API. Overriding that in-cluster is a separate + * behavioral decision (issue #5735). + *
  • {@link Key#PPL_VALUES_MAX_LIMIT} — forwarding it breaks the route rather than fixing it: + * the cap lowers {@code values()} to {@code array_agg(DISTINCT x, limit)}, a form the + * backend cannot bind, so the query 500s where today it merely ignores the cap (issue + * #5736). + *
  • {@link Key#CALCITE_SUPPORT_ALL_JOIN_TYPES} — never seeded, so {@code + * AstBuilder.validateJoinType} reads {@code null} and skips the high-cost-join guard + * entirely. Restoring it is a user-visible tightening (issue #5734). + *
* - *

{@code RestUnifiedQueryActionTest#everySeededPlanningSettingIsClassified} pins the seeded - * exclusions, so a key added to the builder's seed map cannot silently regress to its hardcoded - * default without failing a test. + *

{@code RestUnifiedQueryActionTest} pins both this list and those exclusions. */ @VisibleForTesting static final List FORWARDED_CLUSTER_SETTINGS = diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java index 8d43625d9f7..0106ad3920b 100644 --- a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java +++ b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java @@ -7,7 +7,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -272,19 +272,28 @@ public void calciteEngineEnabledNotOverriddenByCluster() { } /** - * Planning settings {@link UnifiedQueryContext.Builder} seeds that the REST handler deliberately - * does not forward, each with the reason it stays hardcoded on the unified path. + * Every setting the AE path reads that {@link RestUnifiedQueryAction} deliberately does not + * forward, with the reason it stays unforwarded. Derived from the call sites reachable from the + * unified context's {@code Settings}: {@code SysLimit.fromSettings}, {@code AstBuilder} /{@code + * AstExpressionBuilder} / {@code AstBuildGuard}, and {@code UnresolvedPlanHelper}. * *

    *
  • {@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition. *
  • {@link Key#PPL_SUBSEARCH_MAXOUT} / {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to - * {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out of plans built - * by external consumers of the unified query API. Whether the in-cluster path should - * override that is tracked by https://github.com/opensearch-project/sql/issues/5735. + * {@code 0} (unlimited) on purpose, for external consumers of the unified query API (issue + * #5735). + *
  • {@link Key#PPL_VALUES_MAX_LIMIT} — forwarding it 500s the route (issue #5736). + *
  • {@link Key#CALCITE_SUPPORT_ALL_JOIN_TYPES} — never seeded; restoring the guard is a + * user-visible tightening (issue #5734). *
*/ private static final List DELIBERATELY_NOT_FORWARDED = - List.of(Key.CALCITE_ENGINE_ENABLED, Key.PPL_SUBSEARCH_MAXOUT, Key.PPL_JOIN_SUBSEARCH_MAXOUT); + List.of( + Key.CALCITE_ENGINE_ENABLED, + Key.PPL_SUBSEARCH_MAXOUT, + Key.PPL_JOIN_SUBSEARCH_MAXOUT, + Key.PPL_VALUES_MAX_LIMIT, + Key.CALCITE_SUPPORT_ALL_JOIN_TYPES); /** * Drift guard for the defect class this forwarding exists to prevent: the builder's seed map and @@ -331,17 +340,28 @@ public void clusterPatternSettingsReachAnalyticsContext() { } @Test - public void valuesMaxLimitIsNotForwarded() { - // Forwarding PPL_VALUES_MAX_LIMIT would attach a `limit` argument to values(), lowering to - // array_agg(DISTINCT x, limit) — a form the analytics-engine backend cannot bind, turning a - // silently-ignored cap into a 500. Verified against a live composite/parquet cluster. - when(pluginSettings.getSettingValue(Key.PPL_VALUES_MAX_LIMIT)).thenReturn(100); - - assertNull( - "Forwarding values.max.limit breaks values() on the AE route; it must stay unset", - buildAnalyticsContext().getSettings().getSettingValue(Key.PPL_VALUES_MAX_LIMIT)); + public void documentedExclusionsAreNotForwarded() { + // Pins the exclusion list in RestUnifiedQueryAction's javadoc: each of these is a conscious + // decision, not an oversight, so a well-meaning "just forward everything" change fails here. + // PPL_VALUES_MAX_LIMIT in particular must stay out: forwarding it lowers values() to + // array_agg(DISTINCT x, limit), which the AE backend cannot bind, turning a silently-ignored + // cap into a 500 (verified on a live composite/parquet cluster). + DELIBERATELY_NOT_FORWARDED.forEach( + key -> when(pluginSettings.getSettingValue(key)).thenReturn(SENTINEL)); + + org.opensearch.sql.common.setting.Settings forwarded = buildAnalyticsContext().getSettings(); + + for (Key key : DELIBERATELY_NOT_FORWARDED) { + assertNotEquals( + key.getKeyValue() + " must not be forwarded to the Analytics Engine context", + SENTINEL, + forwarded.getSettingValue(key)); + } } + /** Value no seeded default uses, so "forwarded" is distinguishable from "seeded" or "absent". */ + private static final Integer SENTINEL = -12345; + /** Builds the context the AE path plans against, with the mocked cluster settings applied. */ private UnifiedQueryContext buildAnalyticsContext() { return action From 6e2d9baca338b274debc55e59ffef09c766a0814 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 2 Sep 2026 13:58:45 -0700 Subject: [PATCH 4/4] Review: trim the forwarding javadoc further, drop issue links from comments Per review, the production javadoc is cut to the rule plus a pointer: the per-key exclusion reasons live in one place only, the test's DELIBERATELY_NOT_FORWARDED, which is also what pins them. Down from ~28 lines originally to 8. Issue references are removed from code comments in both files; the reasons stand on their own, and tracking belongs in the PR and issues rather than in comments that go stale. Also dropped the hardcoded "five" from the javadoc so the count cannot rot as the list changes -- the enumerated list is the answer. Signed-off-by: Jialiang Liang Signed-off-by: Jialiang Liang --- .../plugin/rest/RestUnifiedQueryAction.java | 24 ++++--------------- .../rest/RestUnifiedQueryActionTest.java | 17 +++++++------ 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java index 6592e94e974..9dee33c0d5f 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java @@ -351,29 +351,13 @@ private static QueryRequestContext withParentTask(QueryRequestContext ctx, Task /** * Cluster settings forwarded into every {@link UnifiedQueryContext}, so the Analytics Engine - * plans against the same configuration as the default pipeline. Any planning setting the AE path + * plans against the same configuration as the default pipeline. A planning setting the AE path * must honor belongs here; otherwise the value {@link UnifiedQueryContext.Builder} seeds silently * wins and the configured cluster value is ignored. * - *

The AE path reads exactly five other settings, each deliberately left out: - * - *

    - *
  • {@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition and - * must force it on. - *
  • {@link Key#PPL_SUBSEARCH_MAXOUT}, {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to - * {@code 0} (unlimited) on purpose, to keep {@code LogicalSystemLimit} out of plans built - * by external consumers of the unified query API. Overriding that in-cluster is a separate - * behavioral decision (issue #5735). - *
  • {@link Key#PPL_VALUES_MAX_LIMIT} — forwarding it breaks the route rather than fixing it: - * the cap lowers {@code values()} to {@code array_agg(DISTINCT x, limit)}, a form the - * backend cannot bind, so the query 500s where today it merely ignores the cap (issue - * #5736). - *
  • {@link Key#CALCITE_SUPPORT_ALL_JOIN_TYPES} — never seeded, so {@code - * AstBuilder.validateJoinType} reads {@code null} and skips the high-cost-join guard - * entirely. Restoring it is a user-visible tightening (issue #5734). - *
- * - *

{@code RestUnifiedQueryActionTest} pins both this list and those exclusions. + *

The other settings the AE path reads are deliberately not forwarded; {@code + * RestUnifiedQueryActionTest#DELIBERATELY_NOT_FORWARDED} enumerates them with a reason each, and + * pins both lists. */ @VisibleForTesting static final List FORWARDED_CLUSTER_SETTINGS = diff --git a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java index 0106ad3920b..304d17b095d 100644 --- a/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java +++ b/plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java @@ -274,17 +274,20 @@ public void calciteEngineEnabledNotOverriddenByCluster() { /** * Every setting the AE path reads that {@link RestUnifiedQueryAction} deliberately does not * forward, with the reason it stays unforwarded. Derived from the call sites reachable from the - * unified context's {@code Settings}: {@code SysLimit.fromSettings}, {@code AstBuilder} /{@code - * AstExpressionBuilder} / {@code AstBuildGuard}, and {@code UnresolvedPlanHelper}. + * unified context's {@code Settings}: {@code SysLimit.fromSettings}, the {@code AstBuilder} / + * {@code AstExpressionBuilder} / {@code AstBuildGuard} behind its parser, and {@code + * UnresolvedPlanHelper}. * *

    *
  • {@link Key#CALCITE_ENGINE_ENABLED} — the unified path is Calcite-based by definition. *
  • {@link Key#PPL_SUBSEARCH_MAXOUT} / {@link Key#PPL_JOIN_SUBSEARCH_MAXOUT} — seeded to - * {@code 0} (unlimited) on purpose, for external consumers of the unified query API (issue - * #5735). - *
  • {@link Key#PPL_VALUES_MAX_LIMIT} — forwarding it 500s the route (issue #5736). - *
  • {@link Key#CALCITE_SUPPORT_ALL_JOIN_TYPES} — never seeded; restoring the guard is a - * user-visible tightening (issue #5734). + * {@code 0} (unlimited) on purpose, for external consumers of the unified query API. + *
  • {@link Key#PPL_VALUES_MAX_LIMIT} — forwarding it lowers {@code values()} to {@code + * array_agg(DISTINCT x, limit)}, which the backend cannot bind, so the query fails outright + * where today it merely ignores the cap. + *
  • {@link Key#CALCITE_SUPPORT_ALL_JOIN_TYPES} — never seeded, so {@code + * AstBuilder.validateJoinType} reads {@code null} and skips the high-cost-join guard. + * Restoring it is a user-visible tightening. *
*/ private static final List DELIBERATELY_NOT_FORWARDED =