[Feature] Add PPL rest command

[noCharger]

Description

Add a leading rest <endpoint> command that exposes a curated, read-only, fixed-schema set of in-cluster management endpoints as a PPL table, modeled as a system row source bridged through visitRelation (the same seam as describe and the system-index family), so it runs on the Calcite engine without the unsupported table-function path.

• Grammar/AST: REST/TIMEOUT tokens, restCommand rule, RestRelation, AstBuilder visit, query anonymizer.
• Execution: RestSourceTable -> CalciteLogicalRestScan / CalciteEnumerableRestScan; RestEndpointRegistry (read-only allow-list + fixed schema + accepted args); RestEnumerator/RestRequest dispatch via OpenSearchClient (NodeClient in-cluster, RestClient standalone).
• 9 endpoints: cluster health/state/settings, cat indices/nodes/cluster_manager/ plugins/shards, resolve/index.
• Output shaping: numeric type normalization, id-to-name resolution, role-name expansion, structural flattening, graceful null degrade.
• Args: count caps emitted rows; timeout reserved but rejected with 400; get-args applied server-side with per-arg value validation (local on health, health on cat/indices, expand_wildcards on resolve/index). Undeclared arg or out-of-domain value is rejected with a 400. level and include_defaults are deferred to a later release; flat_settings is dropped as redundant.
• Error handling: blank endpoint, negative count, disallowed arg, and uncoercible value all surface clean 400s rather than 500s.

Response redaction, endpoint allow-list, and access control

Two node-level settings (NodeScope, set in opensearch.yml) gate the command. The allow-list defaults to empty, so the command is disabled by default — a deployment must explicitly opt endpoints in. The redaction setting defaults to native (off). They are intentionally not dynamically updatable, so the gate cannot be turned off (or the allow-list widened) at runtime via _cluster/settings or _plugins/_query/settings once an operator has set it.

• plugins.ppl.rest.redaction.enabled (Boolean, default false): when enabled, masks network identifiers in /_cat/* output — IPv4, IPv6, inet[/...] addresses, EC2-style host names (ip-a-b-c-d), and availability-zone names — and availability-zone names in /_cluster/settings values. Default false preserves the native _cat output (real values).
• plugins.ppl.rest.allowed_endpoints (List, default []): the command is disabled by default; a deployment opts endpoints in explicitly. [] (the default) disables the command entirely; an explicit subset allows only those endpoints (others return a clear "not enabled on this cluster" 400); ["*"] allows every registered endpoint. This lets open source ship closed while a managed service (for example AOS) enables only the endpoints it supports, and AOSS leaves it empty. Enforced at a single choke point in OpenSearchStorageEngine#getTable.

Access control. The command is already subject to the security plugin's fine-grained access control with no special integration. Each endpoint dispatches a standard transport action (for example cluster:monitor/nodes/stats, cluster:monitor/state, indices:admin/resolve/index) through the node client under the caller's identity, so the security ActionFilter authorizes every call by action name. A caller lacking the privilege is rejected, exactly as on the native endpoint, and /_resolve/index requires the indices:admin/resolve/index privilege because the command resolves all indices. Document- and field-level security do not apply because the command issues no document searches, which matches the native _cat and _cluster APIs. Consequently, once a deployment opts endpoints in, behavior matches native 1:1 — real IPs and the enabled endpoints are visible only to callers who already hold the corresponding cluster:monitor/* privilege — while the empty-by-default allow-list and optional redaction are additional hardening for managed or restricted deployments.

Related Issues

Resolves #5597

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 03d21c3.

Path	Line	Severity	Description
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestSettingsFilterHolder.java	32	medium	Static volatile holder publishes the node SettingsFilter via a global singleton. If set() is called before getRestHandlers() completes (e.g., race at startup or in tests that forget to reset), the filter is null and clusterSettings() fails closed — but the window where a null filter allows an unredacted response is a latent risk if the fail-closed guard is ever weakened.
opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchRestClient.java	370	low	The standalone-mode clusterSettings() fetches /_cluster/settings via the low-level HTTP client with flat_settings=true but applies no SettingsFilter redaction (unlike the node-client path which enforces the filter). Secrets registered with Property.Filtered or plugin filter patterns would be returned unredacted to callers using the REST client.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 03d21c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The /_cluster/settings endpoint can expose cluster configuration, including potentially sensitive settings. The PR mitigates this by requiring a SettingsFilter to redact Property.Filtered keys before returning settings. However, the RestSettingsFilterHolder uses a static volatile field to share the filter instance. If the filter is not set (e.g., due to a plugin initialization failure), the clusterSettings method in OpenSearchNodeClient throws an IllegalStateException and refuses to return unredacted settings (fail-closed). This is correct. However, the standalone OpenSearchRestClient path for /_cluster/settings calls the REST endpoint with flat_settings=true and does not apply any additional filtering, relying on the server-side filter. If the server-side filter is misconfigured or bypassed, secrets could leak. The PR assumes the server-side REST handler applies the filter, which is standard, but this is not verified in the code. Additionally, the RestResponseRedactor masks network identifiers (IPs, hostnames, availability zones) when redaction.enabled=true, but this is optional and defaults to false. Deployments that must not expose topology should enable it, but the PR does not enforce this. The security model relies on: (1) the allow-list being explicitly configured (default empty, so disabled), (2) the SettingsFilter being published at startup, and (3) the caller having the required cluster-monitor privilege. If any of these assumptions fail, sensitive data could be exposed.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In the toNumber method, parsing an empty string after trim throws NumberFormatException("empty string"), but this exception extends IllegalArgumentException, which is caught and wrapped into a client error. However, the check if (s.isEmpty()) occurs after trim(), so a string containing only whitespace will be trimmed to empty and throw. This is correct behavior, but the method does not handle the case where the original value is a non-numeric string that looks numeric but fails parsing (e.g., "123abc"). The Long.parseLong(s) or Double.parseDouble(s) will throw NumberFormatException, which extends IllegalArgumentException and will be caught and re-thrown as a client error. This is intended, but the logic could be clearer about what constitutes an "uncoercible value" versus a malformed numeric string. /** Coerce a transport/JSON value to a Number, parsing numeric strings (e.g. the cat JSON API). */ private static Number toNumber(Object value) { if (value instanceof Number n) { return n; } String s = String.valueOf(value).trim(); if (s.isEmpty()) { throw new NumberFormatException("empty string"); } if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) { return Double.parseDouble(s); } return Long.parseLong(s); } Possible Issue In clusterSettings, the method calls filter.filter() on persistentSettings and transientSettings to redact secrets. However, if response.getState().metadata().persistentSettings() or transientSettings() returns null, the code passes null to filter.filter(), which may throw NullPointerException depending on the SettingsFilter implementation. The collectSettings method does check if (settings == null) and returns early, but the filter is applied before that check. If the cluster state metadata can return null settings, this will fail. org.opensearch.common.settings.Settings persistent = filter.filter(response.getState().metadata().persistentSettings()); org.opensearch.common.settings.Settings transientSettings = filter.filter(response.getState().metadata().transientSettings()); collectSettings(persistent, "persistent", rows); collectSettings(transientSettings, "transient", rows); return rows; Possible Issue The validateArgValue method for expand_wildcards splits the value by comma and trims each token, but it does not handle the case where a token is empty after trimming (e.g., "open,,closed" or "open, ,closed"). An empty token will not be in EXPAND_WILDCARDS_VALUES and will throw an exception, which is correct, but the error message will show the original value, not the specific empty token. This could confuse users about which part of the comma-separated list is invalid. } else if ("expand_wildcards".equals(arg)) { if (value == null || value.isBlank()) { throw new IllegalArgumentException( "rest endpoint [" + endpoint + "] arg [expand_wildcards] has an unsupported value [" + value + "]. Allowed values: " + EXPAND_WILDCARDS_VALUES); } for (String token : value.toLowerCase(java.util.Locale.ROOT).split(",")) { if (!EXPAND_WILDCARDS_VALUES.contains(token.trim())) { throw new IllegalArgumentException( "rest endpoint [" + endpoint + "] arg [expand_wildcards] has an unsupported value [" + value + "]. Allowed values: " + EXPAND_WILDCARDS_VALUES); } } } Possible Issue In restTable, the method retrieves the PPL_REST_ALLOWED_ENDPOINTS setting and checks if it is null or empty. If allowed is null, the error message says "the rest command is disabled on this cluster". However, the condition allowed == null || allowed.isEmpty() treats both null and empty list identically. If the setting is misconfigured and returns null unexpectedly (e.g., due to a settings initialization issue), the error message may be misleading. The code should distinguish between "setting not initialized" (null) and "explicitly set to empty list" (disabled). List<String> allowed = settings.getSettingValue(Settings.Key.PPL_REST_ALLOWED_ENDPOINTS); if (allowed == null || !(allowed.contains("*") || allowed.contains(spec.getEndpoint()))) { throw new IllegalArgumentException( allowed == null || allowed.isEmpty() ? "the rest command is disabled on this cluster" : "rest endpoint [" + spec.getEndpoint() + "] is not enabled on this cluster. Enabled endpoints: " + allowed); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 03d21c3

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Reject malformed token lines explicitly The decoder silently skips malformed lines (no '=' sign) which could hide corruption or injection attempts. Consider logging a warning or throwing an exception when encountering unexpected line formats to detect potential security issues or data corruption early. core/src/main/java/org/opensearch/sql/utils/SystemIndexUtils.java [78-117] public static RestSpec decodeRestSpec(String indexName) { if (!isRestSource(indexName)) { throw new IllegalArgumentException("not a valid rest source token: " + indexName); } String body = indexName.substring( REST_SOURCE_PREFIX.length(), indexName.length() - REST_SOURCE_SUFFIX.length()); String decoded = fromHex(body); ... for (String line : decoded.split("\n")) { if (line.isEmpty()) { continue; } int eq = line.indexOf('='); if (eq < 0) { - continue; + throw new IllegalArgumentException("malformed rest source token line (missing '='): " + indexName); } ... } if (endpoint == null) { throw new IllegalArgumentException("rest source token is missing the endpoint: " + indexName); } return new RestSpec(endpoint, args, count, timeout); } Suggestion importance[1-10]: 7 __ Why: The suggestion to throw an exception instead of silently skipping malformed lines is a good security and data integrity improvement. The current code at lines 98-100 silently continues on malformed lines, which could hide corruption or injection attempts. Failing fast with a clear error is more secure.	Medium
General	Validate empty tokens in comma-separated values The validation logic for expand_wildcards splits on comma but doesn't handle potential empty tokens from consecutive commas (e.g., "open,,closed"). Add validation to reject malformed input with empty tokens after splitting and trimming. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [324-360] private static void validateArgValue(String endpoint, String arg, String value) { Set<String> domain = ARG_VALUE_DOMAINS.get(arg); if (domain != null) { if (value == null || !domain.contains(value.toLowerCase(java.util.Locale.ROOT))) { throw new IllegalArgumentException(...); } } else if ("expand_wildcards".equals(arg)) { if (value == null || value.isBlank()) { throw new IllegalArgumentException(...); } for (String token : value.toLowerCase(java.util.Locale.ROOT).split(",")) { - if (!EXPAND_WILDCARDS_VALUES.contains(token.trim())) { + String trimmed = token.trim(); + if (trimmed.isEmpty() || !EXPAND_WILDCARDS_VALUES.contains(trimmed)) { throw new IllegalArgumentException(...); } } } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that consecutive commas could produce empty tokens after splitting. The improved code adds validation to reject empty tokens explicitly, which strengthens input validation and prevents potential edge cases in the expand_wildcards argument handling.	Low
	Handle potential NullPointerException in coercion The coercion logic catches IllegalArgumentException and ClassCastException but doesn't handle potential NullPointerException from toNumber(value) or toBoolean(value) when value is unexpectedly null after the null check. Add explicit null handling in helper methods or ensure the catch block includes NPE. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } - } catch (IllegalArgumentException | ClassCastException e) { + } catch (IllegalArgumentException | ClassCastException | NullPointerException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 5 __ Why: The suggestion to catch NullPointerException is reasonable for defensive programming, though the existing null check at line 363 should prevent NPE in normal flow. The improvement is marginal since the helper methods toNumber and toBoolean are designed to handle non-null values after the check.	Low
	Improve error message for filter initialization The method fails closed when the filter is null, which is correct. However, consider adding a check to verify the filter is properly initialized at startup rather than at query time to prevent runtime failures. This could be done in a plugin initialization hook. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + + " settings. Ensure RestSettingsFilterHolder.set() is called during plugin initialization."); } ... } Suggestion importance[1-10]: 4 __ Why: The suggestion to enhance the error message is helpful for debugging, but the improvement is minor. The existing message already clearly states the filter is not initialized. Adding initialization guidance is a documentation improvement rather than a functional fix.	Low

---

Previous suggestions

Suggestions up to commit 4b6262f

Category	Suggestion	Impact
General	Handle STRING type explicitly The coerce method should explicitly handle the STRING type case before the fallback to avoid potential confusion. Currently, if type is STRING, it falls through to the default stringValue conversion, which works but is implicit. Add an explicit if (type == STRING) check for clarity and to ensure the type-checking logic is complete. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } - return stringValue(String.valueOf(value)); + throw new IllegalArgumentException("Unsupported type: " + type); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the STRING type handling is implicit. Adding an explicit check improves code clarity and maintainability. However, the current code works correctly, so this is a minor improvement rather than a bug fix.	Low
	Add blank check before toLowerCase The validateArgValue method should validate that value is not null or blank before calling toLowerCase() on it in the first domain check. While the null check exists, it's after the toLowerCase() call in the condition, which could cause a NullPointerException if the order of evaluation changes or if the code is refactored. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [324-360] private static void validateArgValue(String endpoint, String arg, String value) { Set<String> domain = ARG_VALUE_DOMAINS.get(arg); if (domain != null) { - if (value == null || !domain.contains(value.toLowerCase(java.util.Locale.ROOT))) { + if (value == null || value.isBlank() || !domain.contains(value.toLowerCase(java.util.Locale.ROOT))) { throw new IllegalArgumentException(...); } } else if ("expand_wildcards".equals(arg)) { if (value == null || value.isBlank()) { throw new IllegalArgumentException(...); } for (String token : value.toLowerCase(java.util.Locale.ROOT).split(",")) { if (!EXPAND_WILDCARDS_VALUES.contains(token.trim())) { throw new IllegalArgumentException(...); } } } } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a potential issue where value.toLowerCase() could be called on a null value. However, the null check already exists in the condition (value == null), and Java's short-circuit evaluation prevents the NPE. Adding isBlank() is a minor defensive improvement but not critical.	Low
	Improve error message clarity The clusterSettings method fails closed when the SettingsFilter is not initialized, which is correct. However, the error message should be more actionable by indicating this is a configuration issue. Consider logging this error at a higher severity level since it represents a critical initialization failure that prevents the command from working. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { - throw new IllegalStateException( - "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + String errorMsg = "cluster settings redaction filter is not initialized; refusing to return unredacted settings. This indicates a plugin initialization error."; + throw new IllegalStateException(errorMsg); } ... } Suggestion importance[1-10]: 3 __ Why: The suggestion to improve the error message is valid but minor. The current message is already clear about the issue. Adding "This indicates a plugin initialization error" provides marginal additional value.	Low

Suggestions up to commit f4b6086

Category	Suggestion	Impact
General	Validate empty tokens in comma-separated values The validation logic for expand_wildcards splits on comma but doesn't handle potential empty tokens from consecutive commas or leading/trailing commas. Add validation to reject malformed input like "open,,closed" or ",open" to prevent unexpected behavior. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [324-359] private static void validateArgValue(String endpoint, String arg, String value) { Set<String> domain = ARG_VALUE_DOMAINS.get(arg); if (domain != null) { if (value == null || !domain.contains(value.toLowerCase(java.util.Locale.ROOT))) { throw new IllegalArgumentException(...); } } else if ("expand_wildcards".equals(arg)) { if (value == null || value.isBlank()) { throw new IllegalArgumentException(...); } for (String token : value.toLowerCase(java.util.Locale.ROOT).split(",")) { - if (!EXPAND_WILDCARDS_VALUES.contains(token.trim())) { + String trimmed = token.trim(); + if (trimmed.isEmpty() || !EXPAND_WILDCARDS_VALUES.contains(trimmed)) { throw new IllegalArgumentException(...); } } } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the expand_wildcards validation does not reject empty tokens from malformed input like "open,,closed". Adding a check for trimmed.isEmpty() would improve input validation and prevent unexpected behavior. This is a valid improvement with moderate impact on robustness.	Low
	Add explicit STRING type handling The coercion logic should handle the case where type is STRING explicitly before falling through to the default stringValue conversion. This ensures consistent behavior and prevents potential issues if additional type checks are added later. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 4 __ Why: The suggestion adds an explicit STRING type check before the fallback stringValue conversion. While this improves code clarity and consistency, the existing code already handles STRING correctly via the fallback, so the impact is minor. The suggestion is valid but offers only marginal improvement.	Low
	Prevent potential race condition The method should validate that the SettingsFilter is initialized before processing any cluster settings request. However, the current implementation could lead to a race condition if the filter is set to null after the check but before usage. Consider using a local variable to capture the filter reference once. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-479] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } - ... + org.opensearch.action.admin.cluster.state.ClusterStateResponse response = + client + .admin() + .cluster() + .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) + .actionGet(); + List<Map<String, Object>> rows = new java.util.ArrayList<>(); + org.opensearch.common.settings.Settings persistent = filter.filter(response.getState().metadata().persistentSettings()); + org.opensearch.common.settings.Settings transientSettings = filter.filter(response.getState().metadata().transientSettings()); + collectSettings(persistent, "persistent", rows); + collectSettings(transientSettings, "transient", rows); + return rows; } Suggestion importance[1-10]: 3 __ Why: The suggestion addresses a theoretical race condition where the SettingsFilter could be set to null between the null check and usage. However, the improved_code is identical to the existing_code and does not actually implement the suggested fix (capturing the filter reference in a local variable). The concern is valid but the provided solution does not address it.	Low

Suggestions up to commit 408c730

Category	Suggestion	Impact
General	Explicitly handle STRING type coercion The coercion logic should explicitly handle the STRING type case before the fallback. Currently, if type == STRING, the code falls through to the catch block's return statement, which is correct but implicit. Add an explicit if (type == STRING) check before the final return to make the logic clearer and prevent potential issues if the type system changes. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the STRING type handling is implicit in the fallback return statement. However, the current code is functionally correct and the improvement is minor - it only adds explicit clarity without changing behavior. The score reflects that this is a valid but low-impact code style improvement.	Low
	Optimize count truncation logic The count truncation logic should be applied before materializing all rows to avoid unnecessary memory allocation when count is small. Consider passing the count limit to the endpoint fetcher so it can limit results at the source, especially for endpoints that may return large result sets. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestRequest.java [44-51] @Override public List<ExprValue> search() { List<ExprValue> rows = endpoint.toRows(client, spec, redact); - if (spec.getCount() != null && spec.getCount() >= 0 && rows.size() > spec.getCount()) { - return rows.subList(0, spec.getCount()); + if (spec.getCount() != null && spec.getCount() >= 0) { + int limit = Math.min(spec.getCount(), rows.size()); + return rows.subList(0, limit); } return rows; } Suggestion importance[1-10]: 4 __ Why: The improved_code uses Math.min which is slightly cleaner, but the suggestion's main point about avoiding unnecessary memory allocation is not addressed in the code change. The improved code still materializes all rows before truncating. The actual optimization described would require architectural changes not shown in the improved_code.	Low
	Improve error message for filter initialization The null check for the SettingsFilter should be performed at a higher level or during initialization rather than at every invocation. This creates a potential race condition where the filter might not be set when the first request arrives, causing legitimate requests to fail. Consider validating filter availability during node startup or in the storage engine initialization. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( - "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + "cluster settings redaction filter is not initialized; this indicates a plugin" + + " initialization ordering issue. Please report this error."); } ... } Suggestion importance[1-10]: 3 __ Why: The suggestion proposes a slightly better error message but doesn't address the claimed race condition issue. The existing error message is already clear about the problem. The improved message adds minimal value and the suggestion's analysis about moving the check to initialization is not reflected in the improved_code.	Low

Suggestions up to commit 44c9bb0

Category	Suggestion	Impact
Security	Verify filter null-check consistency The fail-closed check for the SettingsFilter should be performed consistently across both client implementations. Verify that OpenSearchRestClient.clusterSettings also performs the same null-check and fails closed when the filter is unavailable, ensuring consistent security behavior across both client paths. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [453-465] @Override public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( - "cluster settings redaction filter is not initialized; refusing to return unredacted" - + " settings"); + "cluster settings redaction filter is not initialized; refusing to return unredacted settings"); } ... } Suggestion importance[1-10]: 7 __ Why: This is a valid security concern asking to verify consistency across client implementations. The suggestion correctly identifies that OpenSearchRestClient should also perform the same fail-closed check. However, it only asks for verification rather than providing a concrete fix, and the PR diff shows OpenSearchRestClient.clusterSettings uses a different implementation path (REST API) that may not need the same filter check.	Medium
General	Handle STRING type explicitly The coercion logic should explicitly handle the STRING type case before the fallback. Currently, any non-numeric/non-boolean type falls through to the string conversion, which could mask unexpected type mismatches. Add an explicit if (type == STRING) branch before the final return to make the type handling more explicit and maintainable. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { return integerValue(toNumber(value).intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } + if (type == STRING) { + return stringValue(String.valueOf(value)); + } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } - return stringValue(String.valueOf(value)); + throw new IllegalArgumentException( + "unsupported type for column [" + column + "]: " + type); } Suggestion importance[1-10]: 6 __ Why: The suggestion improves code clarity by making the STRING type handling explicit and adding a final fallback error for unsupported types. However, the current code already handles STRING correctly as a fallback, so this is a minor improvement in maintainability rather than a bug fix.	Low
	Validate numeric conversion bounds The numeric parsing logic should handle potential overflow when converting from Double to Integer/Long in the coerce method. When a Double value exceeds Integer.MAX_VALUE or Long.MAX_VALUE, the narrowing conversion will silently overflow. Add range validation before the narrowing conversions to prevent data corruption. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [396-409] private static Number toNumber(Object value) { if (value instanceof Number n) { return n; } String s = String.valueOf(value).trim(); if (s.isEmpty()) { throw new NumberFormatException("empty string"); } if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) { - return Double.parseDouble(s); + double d = Double.parseDouble(s); + if (Double.isInfinite(d) || Double.isNaN(d)) { + throw new NumberFormatException("invalid numeric value: " + s); + } + return d; } return Long.parseLong(s); } Suggestion importance[1-10]: 5 __ Why: The suggestion adds validation for infinite/NaN values which is a reasonable improvement. However, it doesn't fully address the stated concern about overflow during narrowing conversions (Double to Integer/Long), which would need to be checked in the coerce method, not in toNumber. The improvement is partial and the location doesn't match the described issue.	Low

Suggestions up to commit aef4663

Category	Suggestion	Impact
General	Validate numeric range before type conversion The coercion logic may produce incorrect results when converting numeric types. Using intValue() on a Number can truncate values without validation. Verify that the numeric value is within the valid range for the target type before conversion to prevent silent data loss. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [362-394] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); } try { if (type == INTEGER) { - return integerValue(toNumber(value).intValue()); + Number num = toNumber(value); + if (num.longValue() < Integer.MIN_VALUE || num.longValue() > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Value out of range for INTEGER"); + } + return integerValue(num.intValue()); } if (type == LONG) { return longValue(toNumber(value).longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } } catch (IllegalArgumentException | ClassCastException e) { throw new IllegalArgumentException( "rest endpoint value for column [" + column + "] could not be coerced to " + type + ": [" + value + "]"); } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a real issue where intValue() truncation could silently lose data when converting numbers outside the INTEGER range. The proposed range check would catch this and throw a clear error. This is a valid improvement for data integrity, though the impact depends on whether the REST endpoints actually return out-of-range values in practice.	Medium
	Add exception handling for cluster state request The method should handle potential exceptions from the cluster state request. If the request fails or times out, the method will throw an uncaught exception. Wrap the actionGet() call in a try-catch block to provide a more informative error message to the caller. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [454-479] public List<Map<String, Object>> clusterSettings(Map<String, String> params) { org.opensearch.common.settings.SettingsFilter filter = org.opensearch.sql.opensearch.storage.rest.RestSettingsFilterHolder.get(); if (filter == null) { throw new IllegalStateException( "cluster settings redaction filter is not initialized; refusing to return unredacted" + " settings"); } - org.opensearch.action.admin.cluster.state.ClusterStateResponse response = - client - .admin() - .cluster() - .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) - .actionGet(); - List<Map<String, Object>> rows = new java.util.ArrayList<>(); - org.opensearch.common.settings.Settings persistent = - filter.filter(response.getState().metadata().persistentSettings()); - org.opensearch.common.settings.Settings transientSettings = - filter.filter(response.getState().metadata().transientSettings()); - collectSettings(persistent, "persistent", rows); - collectSettings(transientSettings, "transient", rows); - return rows; + try { + org.opensearch.action.admin.cluster.state.ClusterStateResponse response = + client + .admin() + .cluster() + .state(new org.opensearch.action.admin.cluster.state.ClusterStateRequest()) + .actionGet(); + List<Map<String, Object>> rows = new java.util.ArrayList<>(); + org.opensearch.common.settings.Settings persistent = + filter.filter(response.getState().metadata().persistentSettings()); + org.opensearch.common.settings.Settings transientSettings = + filter.filter(response.getState().metadata().transientSettings()); + collectSettings(persistent, "persistent", rows); + collectSettings(transientSettings, "transient", rows); + return rows; + } catch (Exception e) { + throw new IllegalStateException("Failed to retrieve cluster settings", e); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the actionGet() call could throw exceptions. However, the existing code already throws IllegalStateException for other error conditions, and uncaught exceptions from actionGet() would propagate as runtime exceptions. The suggestion adds defensive error handling, which is a reasonable improvement but not critical since the method is already designed to fail fast on errors.	Low
	Optimize regex pattern application for redaction The redaction method applies multiple regex patterns sequentially, which can be inefficient for large strings. Consider compiling a single combined pattern or using a more efficient matching strategy to reduce the number of passes over the input text. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestResponseRedactor.java [43-52] public static String redact(String text) { if (text == null || text.isEmpty()) { return text; } - String out = text; + StringBuilder result = new StringBuilder(text.length()); + String remaining = text; for (Mask mask : MASKS) { - out = mask.pattern().matcher(out).replaceAll(mask.replacement()); + remaining = mask.pattern().matcher(remaining).replaceAll(mask.replacement()); } - return out; + return remaining; } Suggestion importance[1-10]: 3 __ Why: The suggestion claims to optimize the redaction method but the improved_code is functionally identical to the existing_code (just uses a different variable name). The suggestion correctly identifies that multiple sequential regex passes could be inefficient, but the proposed code doesn't actually implement any optimization like combining patterns or reducing passes.	Low

[github-actions]

Persistent review updated to latest commit a7a8a20

[github-actions]

Persistent review updated to latest commit d43f28c

[github-actions]

Persistent review updated to latest commit 7d9df84

[github-actions]

Persistent review updated to latest commit aef4663

[dai-chen]

np: remove all these?

[noCharger]

np: remove all these?

I tried this and had to revert it. It turns out these can't be removed. OpenSearchClient is also implemented downstream by opensearch-project/sql-cli (client/http5/OpenSearchRestClientImpl), which doesn't implement the rest command methods.

Ref https://github.com/opensearch-project/sql/actions/runs/29349113963/job/87140469074?pr=5599

[dai-chen]

I only see RestResponseRedactor focus on IP address redaction. But in my test (Dev Tools in AOS domain), I can see many other masked values, e.g., version in _cat/plugins, cluster.routing.allocation.awareness.force.zone in _cluster/settings, host in _cat/cluster_manager. Are all these redacted automatically? If not, any better way to avoid such risk, like sending what's in REST command go through the exact same process as a REST request sent to the domain by users?

[noCharger]

I only see RestResponseRedactor focus on IP address redaction. But in my test (Dev Tools in AOS domain), I can see many other masked values, e.g., version in _cat/plugins, cluster.routing.allocation.awareness.force.zone in _cluster/settings, host in _cat/cluster_manager. Are all these redacted automatically? If not, any better way to avoid such risk, like sending what's in REST command go through the exact same process as a REST request sent to the domain by users?

The redaction replicates the AOS-side logic exactly. As called out in the PR description, it masks more than IPv4 addresses — IPv6, inet[/…], EC2-style hostnames (ip-a-b-c-d), and availability-zone names are all covered. Every case is asserted in RestResponseRedactorTest.

[github-actions]

Persistent review updated to latest commit f3a6226

[github-actions]

Persistent review updated to latest commit aef4663

[dai-chen]

we need to update this whenever there is a new region?

[noCharger]

Enhanced this logic on 44c9bb0

[github-actions]

Persistent review updated to latest commit 44c9bb0

[github-actions]

Persistent review updated to latest commit 408c730

[github-actions]

Persistent review updated to latest commit f4b6086

[github-actions]

Persistent review updated to latest commit 4b6262f

[github-actions]

Persistent review updated to latest commit 03d21c3