[Enhancement] Forward cluster planning settings to the Analytics Engine unified query path

[RyanL1997]

Description

Several cluster settings were silently ignored on the Analytics Engine (unified query) path — the AE route always planned against UnifiedQueryContext.Builder's hardcoded seed value regardless of the configured cluster value. 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 UnifiedQueryContext.Builder independently seeds a default settings map. Any planning setting present in the seed map but absent from the forward list silently regressed to its default. This is a two-lists-drift defect: the two lists are maintained independently, so the drift is invisible until someone configures the setting and it does nothing.

Settings fixed

Setting	Value the AE path actually used	Effect
plugins.query.size_limit	pinned to 10000	configured result cap ignored
plugins.ppl.pattern.method	pinned to SIMPLE_PATTERN	configured patterns default ignored
plugins.ppl.pattern.mode	pinned to LABEL	ditto
plugins.ppl.pattern.max.sample.count	pinned to 10	ditto
plugins.ppl.pattern.buffer.limit	pinned to 100000	ditto
plugins.ppl.pattern.show.numbered.token	pinned to false	ditto

Every one of these has a cluster-side default identical to the seeded value, so behavior is unchanged unless an operator explicitly configured the setting — at which point the configured value now takes effect. No query that works today changes behavior on defaults.

Fix

• Replace the hand-maintained forwardClusterSetting calls with a single FORWARDED_CLUSTER_SETTINGS allow-list — one source of truth for which cluster settings the unified path honors. This matches the existing shape of the sibling handler RestQuerySettingsAction, which declares its own settings allow/deny lists the same way.
• Add a drift guard (everySeededPlanningSettingIsClassified) asserting that every key the builder seeds is either forwarded or explicitly listed as deliberately excluded, with a reason. Adding a key to the builder's seed map without classifying it now fails a test instead of silently regressing. The guard reads the seeded keys through the existing public Settings#getSettings() API — no reflection, no new production API.

Verified end to end on a live Analytics Engine cluster

Single-node cluster with composite-engine, parquet-data-format, analytics-engine, analytics-backend-datafusion, analytics-backend-lucene + this plugin, querying parquet-backed composite indices. Same cluster, same data, baseline build vs this PR's build — every forwarded key this PR adds:

Setting probed	Baseline (setting ignored)	This PR (setting honored)
query.size_limit=2	6 rows	2 rows
query.size_limit=4	6 rows	4 rows
pattern.mode=AGGREGATION	6 rows, schema [age, name, patterns_field]	1 row, schema [patterns_field, pattern_count, sample_logs]
pattern.method=BRAIN	<*> <*> <*> <*> <*> <*>.<*>.<*>.<*> <*> <*> <*>	user <*> logged in from <*IP*> at port <*>
pattern.show.numbered.token=true	<*> <*> <*> …	<token1> <token2> <token3> …
pattern.max.sample.count=2 / =5	n/a — pattern_count field absent, since pattern.mode was ignored too	len(sample_logs) = 2 / 5 respectively

plugins.ppl.pattern.buffer.limit is forwarded by the same mechanism but is not covered by a live-cluster probe: its minimum is 50000, so distinguishing values requires more buffered patterns than a probe dataset produces. It is covered only by the unit test asserting the value reaches the plan context.

Deliberately not forwarded

• plugins.calcite.enabled — the unified path is Calcite-based by definition and must force it on regardless of the cluster value.
• plugins.ppl.values.max.limit — forwarding this one makes the AE route strictly worse. The cap is applied by attaching a limit argument to values(), which lowers to array_agg(DISTINCT x, limit); the DataFusion backend has no binding for that two-argument form. Confirmed on the cluster above: with the key forwarded, stats values(f) fails with UnsupportedOperationException: Unable to find binding for call array_agg(DISTINCT $0, $1), surfaced as HTTP 500 — where today it merely ignores the cap. Turning a silent no-op into a hard failure is a regression, so the key stays out until the backend can bind the limited form. Tracked in [BUG] plugins.ppl.values.max.limit cannot be honored on the Analytics Engine route (no binding for array_agg(DISTINCT x, limit)) #5736; the gap is already recorded in-repo as Capability.VALUES_LIMIT_NOT_HONORED.
• plugins.ppl.subsearch.maxout / plugins.ppl.join.subsearch_maxout — the builder seeds these to 0 (unlimited) on purpose, to keep LogicalSystemLimit out of plans built by external consumers of the unified query API. Note these do diverge from the cluster defaults (10000 / 50000) even when unconfigured, so the AE path currently runs subsearches unbounded. Whether the in-cluster REST path should override that documented choice is a separate behavioral decision, tracked in [BUG] PPL subsearch maxout settings do not apply on the Analytics Engine path (diverges at defaults) #5735.

One further gap found during this work is also out of scope here, tracked in #5734: plugins.calcite.all_join_types.allowed is not seeded, so AstBuilder.validateJoinType reads null and its config != null && !config check skips validation entirely — the high-cost-join guardrail is inactive on the AE path. Restoring it is a user-visible tightening, so it belongs in its own PR.

Check List

• New functionality includes testing.
• New functionality has been documented in code.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6e2d9ba)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ab19073

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure list immutability The FORWARDED_CLUSTER_SETTINGS list should be immutable to prevent accidental modification at runtime. Consider using List.copyOf() or wrapping with Collections.unmodifiableList() to ensure the list cannot be modified after initialization. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [379-389] @VisibleForTesting static final List<Key> FORWARDED_CLUSTER_SETTINGS = - List.of( + List.copyOf(List.of( Key.QUERY_SIZE_LIMIT, Key.PPL_REX_MAX_MATCH_LIMIT, Key.PPL_SYNTAX_LEGACY_PREFERRED, Key.MAX_EXPRESSION_DEPTH, Key.PATTERN_METHOD, Key.PATTERN_MODE, Key.PATTERN_MAX_SAMPLE_COUNT, Key.PATTERN_BUFFER_LIMIT, - Key.PATTERN_SHOW_NUMBERED_TOKEN); + Key.PATTERN_SHOW_NUMBERED_TOKEN)); Suggestion importance[1-10]: 2 __ Why: List.of() already returns an immutable list in Java 9+, so wrapping it with List.copyOf() is redundant and adds no value. The suggestion is technically correct but unnecessary.	Low

---

Previous suggestions

Suggestions up to commit 1a401a3

Category	Suggestion	Impact
General	Log missing cluster settings The method silently skips settings when getSettingValue returns null, which could mask configuration errors or unintended defaults. Consider logging when a setting is not forwarded, or validate that all expected settings are present before forwarding. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [397-401] private void forwardClusterSetting(UnifiedQueryContext.Builder builder, Key key) { Object value = pluginSettings.getSettingValue(key); if (value != null) { builder.setting(key.getKeyValue(), value); + } else { + logger.debug("Cluster setting {} not found, using default value", key.getKeyValue()); } } Suggestion importance[1-10]: 4 __ Why: While logging missing settings could aid debugging, the current behavior of silently using defaults when getSettingValue returns null is intentional and documented. The suggestion adds marginal value for observability but doesn't address a critical issue.	Low

Suggestions up to commit 506411d

Category	Suggestion	Impact
General	Log when settings are not forwarded The method silently skips settings when getSettingValue returns null, which could mask configuration errors or unintended defaults. Consider logging when a setting is not forwarded, or validating that all expected settings are present to ensure cluster configuration is properly applied. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [396-400] private void forwardClusterSetting(UnifiedQueryContext.Builder builder, Key key) { Object value = pluginSettings.getSettingValue(key); if (value != null) { builder.setting(key.getKeyValue(), value); + } else { + logger.debug("Cluster setting {} not forwarded (null value)", key.getKeyValue()); } } Suggestion importance[1-10]: 4 __ Why: Adding debug logging for null settings could help with troubleshooting, but this is a minor observability improvement. The current behavior of silently skipping null values is intentional and acceptable, as not all settings may be configured. The suggestion assumes a logger field exists without verifying it in the PR diff.	Low

Suggestions up to commit a59cdb7

Category	Suggestion	Impact
General	Ensure consistent Integer type return The mock returns a primitive int (500) but getSettingValue likely returns an Integer object. Ensure type consistency to avoid potential ClassCastException or autoboxing issues. Explicitly return Integer.valueOf(500) for clarity. plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java [242-244] when(pluginSettings.getSettingValue( org.opensearch.sql.common.setting.Settings.Key.QUERY_SIZE_LIMIT)) - .thenReturn(500); + .thenReturn(Integer.valueOf(500)); Suggestion importance[1-10]: 3 __ Why: While using Integer.valueOf(500) is more explicit, Java's autoboxing handles the conversion automatically. The suggestion is technically correct but offers minimal practical benefit since autoboxing is standard and reliable in modern Java.	Low
	Use Boolean object for consistency The mock returns a primitive boolean (false) but getSettingValue likely returns a Boolean object. Use Boolean.FALSE instead of false to ensure type consistency and avoid potential autoboxing issues. plugin/src/test/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryActionTest.java [259-261] when(pluginSettings.getSettingValue( org.opensearch.sql.common.setting.Settings.Key.CALCITE_ENGINE_ENABLED)) - .thenReturn(false); + .thenReturn(Boolean.FALSE); Suggestion importance[1-10]: 3 __ Why: Similar to the previous suggestion, using Boolean.FALSE instead of false is more explicit but autoboxing handles this automatically. The change provides marginal improvement in code clarity without addressing any actual bug.	Low

[github-actions]

Persistent review updated to latest commit 506411d

[github-actions]

Persistent review updated to latest commit 1a401a3

[github-actions]

Persistent review updated to latest commit 4ca83de

[github-actions]

Persistent review updated to latest commit ab19073

[github-actions]

Persistent review updated to latest commit 6e2d9ba