Add a generic, extensible rest-endpoint provider SPI

[noCharger]

Description

Rebuild of the reverted PPL rest command (#5599, reverted in #5635) as a separated, generic, extensible framework. rest <name> exposes read-only, fixed-schema management endpoints as a relational row source behind one operator allow-list, so results feed straight into where / stats / head.

Key design points

• Generic endpoint SPI. A thin new ppl-rest-spi module exposes RestEndpointProvider (an ExtensiblePlugin loadExtensions SPI) plus RestEndpointDefinition / RestEndpointHandler / RestEndpointContext and Column / ColumnType / ArgSpec. New endpoints are registrations, not new grammar keywords — the language stays a single generic rest <name> command.
• Built-in is a uniform SPI client. /_cluster/health ships via CoreEndpointsProvider, which fetches through the SPI context's transport node client (ctx.client()) exactly like an external plugin — there is no privileged in-repo path. ColumnType keeps the SPI module free of any dependency on the sql engine's ExprType.
• Allow-list choke point. plugins.ppl.rest.allowed_endpoints is the single resolve() gate; it defaults to ["/_cluster/health"] (health on, every other endpoint off until an operator opts in). Health stays gated by the cluster's own cluster:monitor/health permission.
• Lazy relational scan. The handler runs at scan execution, never at planning, so EXPLAIN is side-effect free and where / head / stats compose and push down. RestRequest mirrors the system-index OpenSearchSystemIndexScan behind one shared catalog enumerator.
• Central redaction seam. Rows pass through a single Redactor ((endpoint, row) -> row) at the choke point before coercion; the default is Redactor.NONE (no-op). Redaction lives at the platform choke point rather than in the endpoint SPI.
• Scope. This PR adds the framework plus the /_cluster/health reference endpoint; further endpoints are left to follow-up PRs.

Related Issues

Rebuild of #5599 (reverted in #5635) as a separated, extensible framework.

How it works: handler + ctx flow (bootstrap to fetch)

Each endpoint's handler is captured into its Endpoint at bootstrap and travels bundled inside it; the per-query ctx (args + transport client) is assembled at search(); the two meet at Endpoint.toRows -> handler.fetch(ctx). The registry and redactor cross the bootstrap-to-query Guice gap through static holders.

flowchart TB
  subgraph BOOT["Phase 1 · Plugin bootstrap (once per node; loadExtensions runs before the Guice injector)"]
    direction TB
    prov["RestEndpointProvider SPI<br/>CoreEndpointsProvider.getEndpoints()<br/>→ RestEndpointDefinition(name, schema, argSpec, handler)"]
    load["SQLPlugin.loadExtensions<br/>loader.loadExtensions(RestEndpointProvider.class)"]
    build["publishRestCommandRegistries()<br/>new RestEndpointRegistry(providers)<br/>each Definition → new Endpoint(def)<br/>★ Endpoint.handler = def.handler()"]
    hold["RestEndpointRegistryHolder.set(registry)<br/>RedactorHolder.set(Redactor.NONE)"]
    prov -->|discovered| load
    load -->|providers| build
    build -->|publish| hold
  end
  subgraph QUERY["Phase 2 · Query execution (per query: rest '/_cluster/health' ...)"]
    direction TB
    gt["OpenSearchStorageEngine.restTable()<br/>registry = RestEndpointRegistryHolder.get()<br/>redactor = RedactorHolder.get()"]
    src["new RestCatalogSource(registry, spec, client, redactor)<br/>★ endpoint = registry.resolve(spec.endpoint) «allow-list gate»"]
    scan["CalciteEnumerableCatalogScan<br/>source.createRequest()"]
    req["RestCatalogSource.createRequest()<br/>→ new RestRequest(client, endpoint, spec, redactor)"]
    en["OpenSearchCatalogEnumerator<br/>request.search()"]
    se["RestRequest.search()<br/>★ ctx = RestEndpointContext.of(spec.args, nodeClient)<br/>endpoint.toRows(ctx, redactor)"]
    tr["Endpoint.toRows(ctx, redactor)<br/>★★ handler.fetch(ctx) «handler + ctx meet»<br/>→ redactor.redact(endpoint, raw) → coerce → rows"]
    gt --> src
    src --> scan
    scan --> req
    req --> en
    en --> se
    se --> tr
  end
  hold -.->|"registry / redactor (static bridge: bootstrap → query)"| gt
  build ==>|"handler travels bundled inside Endpoint"| tr

Loading

Testing

• Unit tests green on JDK 21: rest endpoint registry, catalog source, cross-plugin extensibility, the centralized Redactor seam, storage engine, and settings.
• CalcitePPLRestIT and RestCommandSecurityIT cover /_cluster/health plus the allow-list and per-arg gates.

Check List

• New functionality includes testing.
• New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added (docs/user/ppl/cmd/rest.md).
• 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 84f5773.

Path	Line	Severity	Description
opensearch/build.gradle	33	high	New internal module dependency added: `api project(':ppl-rest-spi')`. Per mandatory rule, all module/dependency additions must be flagged regardless of apparent legitimacy. Maintainers should verify the new module's build artifacts and publishing configuration.
settings.gradle	20	high	New Gradle subproject `ppl-rest-spi` included in the build. This introduces a new module with its own build pipeline and published artifact surface. Per mandatory rule, any module addition must be reviewed.
plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java	195	medium	`loader.loadExtensions(RestEndpointProvider.class)` allows any co-installed OpenSearch plugin to contribute arbitrary REST endpoint handlers that execute transport actions against the cluster under caller identity. A malicious co-installed plugin could register endpoints that exfiltrate cluster data or perform unintended reads.
integ-test/build.gradle	427	medium	`setting 'cluster.pluggable.dataformat', 'composite'` is added to the `remoteIntegTest` cluster block but is unrelated to the `rest` command feature being implemented. This silently changes query routing for the analytics-engine integration test cluster and could mask failures or alter test outcomes in ways not covered by review.
opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RedactorHolder.java	27	medium	A static, globally settable `Redactor` hook is deliberately designed to be replaced by 'managed distributions via a wrapper patch'. The OSS default is a no-op (`Redactor.NONE`). If a distribution supplies a faulty or malicious `Redactor` implementation, it controls whether sensitive columns in REST responses are masked or exposed, with no secondary enforcement.
opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java	655	low	`registerNonDynamicSettings` now skips `latestSettings.put` when `clusterSettings.get(setting)` returns null. This change was not present before and could silently leave `PPL_REST_ALLOWED_ENDPOINTS` absent from `latestSettings`, potentially causing the allow-list check in `OpenSearchStorageEngine` to receive null and fall through to a disabled-command error instead of the configured default.

The table above displays the top 10 most important findings.

Total: 6 | Critical: 0 | High: 2 | Medium: 3 | 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 7637f4d)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The registerNonDynamicSettings method now conditionally populates latestSettings only if clusterSettings.get(setting) is non-null. If a setting has a default value but is not explicitly configured, clusterSettings.get() may return null, causing the default to be absent from latestSettings. This breaks the contract that latestSettings always reflects the effective value (either configured or default). The original unconditional put ensured defaults were always present. This manifests when code reads settings.getSettingValue(Key.PPL_REST_ALLOWED_ENDPOINTS) and expects the default ["/_cluster/health"] but receives null instead, causing the "rest command is disabled" error even when it should be enabled by default. settingBuilder.put(key, setting); if (clusterSettings.get(setting) != null) { latestSettings.put(key, clusterSettings.get(setting)); } } Possible Issue The toNumber method parses numeric strings but does not handle leading/trailing whitespace consistently with empty-string rejection. After trimming, an empty string throws NumberFormatException("empty string"), but a string like " " (only whitespace) is trimmed to "" and throws the same exception. However, the method checks s.isEmpty() after trim, so this is actually correct. The real issue is that Long.parseLong(s) will throw NumberFormatException for values outside Long range (e.g., very large integers), but the method does not explicitly handle this, relying on the catch block in coerce. This is acceptable for the current design, but if a value is a valid double but not a valid long (e.g., "1e20"), it will be parsed as a double when the schema expects INTEGER, leading to precision loss or overflow when cast to int. This occurs when an endpoint returns a very large integer as a string and the schema declares it as INTEGER. 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 The search method retrieves the node client via client.getNodeClient().orElse(null) and passes it to the endpoint context. If the client is null or the Optional is empty, nodeClient becomes null. The built-in CoreEndpointsProvider.clusterHealth then throws IllegalStateException("requires an in-cluster node client") when client == null. However, RestRequest is constructed with an OpenSearchClient that may legitimately lack a node client in certain configurations (e.g., remote clusters, or when the storage engine is used outside a node context). The error message is clear, but the failure occurs at execution time rather than at construction or validation time, so a query that would fail is not rejected until the scan opens. This manifests when a user runs rest '/_cluster/health' on a deployment where the storage engine's client is not a node client, causing a runtime error instead of a validation error. public List<ExprValue> search() { // The node transport client every provider handler fetches through, sourced from the same // OpenSearchClient the storage engine uses so it runs under the caller's security // thread-context. NodeClient nodeClient = client == null ? null : client.getNodeClient().orElse(null); RestEndpointContext ctx = RestEndpointContext.of(spec.getArgs(), nodeClient); List<ExprValue> rows = endpoint.toRows(ctx, redactor); if (spec.getCount() != null && spec.getCount() >= 0 && rows.size() > spec.getCount()) { return rows.subList(0, spec.getCount()); } return rows;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7637f4d

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate hex characters before decoding The fromHex method does not validate that Character.digit returns a valid digit (it returns -1 for invalid hex characters). If the hex string contains invalid characters, this will produce incorrect byte values without raising an error. Add validation to ensure each character is a valid hex digit before constructing the byte array. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [127-139] private static String fromHex(String h) { if (h.length() % 2 != 0) { throw new IllegalArgumentException("not a valid rest source token: odd-length hex body"); } byte[] bytes = new byte[h.length() / 2]; for (int i = 0; i < bytes.length; i++) { - bytes[i] = - (byte) - ((Character.digit(h.charAt(2 * i), 16) << 4) - + Character.digit(h.charAt(2 * i + 1), 16)); + int high = Character.digit(h.charAt(2 * i), 16); + int low = Character.digit(h.charAt(2 * i + 1), 16); + if (high == -1 || low == -1) { + throw new IllegalArgumentException("not a valid rest source token: invalid hex character"); + } + bytes[i] = (byte) ((high << 4) + low); } return new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 8 __ Why: The fromHex method does not validate that Character.digit returns valid hex digits (-1 indicates invalid). This could silently produce incorrect byte values for malformed tokens. Adding validation ensures clear error messages for invalid input, which is important for security and correctness.	Medium
General	Check allow-list before resolving endpoint The restTable method calls registry.resolve(spec.getEndpoint()) but does not use its return value. This call is made before checking the allow-list, which means the endpoint is resolved even if it's not in the allowed list. Move the allow-list check before the resolve call to fail fast and avoid unnecessary resolution work for disallowed endpoints. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchStorageEngine.java [56-79] private Table restTable(String name) { RestSpec spec = decodeRestSpec(name); RestEndpointRegistry registry = RestEndpointRegistryHolder.get(); if (registry == null) { throw new IllegalStateException( "the rest command endpoint registry is not initialized on this node"); } - registry.resolve(spec.getEndpoint()); 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); } + registry.resolve(spec.getEndpoint()); ... } Suggestion importance[1-10]: 6 __ Why: Moving the allow-list check before registry.resolve() is a fail-fast optimization that avoids unnecessary work for disallowed endpoints. While the current order is functionally correct (both checks happen), checking the allow-list first improves efficiency and clarity.	Low
	Handle STRING type explicitly before coercion The coerce method does not handle the case where type is STRING explicitly before the fallback. If type is STRING and value is not null, the method should directly return stringValue(String.valueOf(value)) without attempting numeric or boolean coercion. This prevents unnecessary exception handling paths when the type is already known to be a string. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [182-214] private static ExprValue coerce(String column, ExprType type, Object value) { if (value == null) { return ExprNullValue.of(); + } + if (type == STRING) { + return stringValue(String.valueOf(value)); } 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) { ... } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 5 __ Why: Adding an explicit STRING type check before the try-catch block is a minor optimization that avoids unnecessary exception handling paths. However, the current fallback already handles STRING correctly, so this is a style improvement rather than a bug fix.	Low

---

Previous suggestions

Suggestions up to commit 84f5773

Category	Suggestion	Impact
Security	Check allow-list before resolving endpoint The restTable method calls registry.resolve() before checking the allow-list, which means an endpoint that exists in the registry but is not in the cluster's enabled list will still be resolved. This could leak information about available endpoints. Consider checking the allow-list before calling resolve() to fail fast and avoid unnecessary resolution. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchStorageEngine.java [56-80] private Table restTable(String name) { RestSpec spec = decodeRestSpec(name); RestEndpointRegistry registry = RestEndpointRegistryHolder.get(); if (registry == null) { throw new IllegalStateException( "the rest command endpoint registry is not initialized on this node"); } - registry.resolve(spec.getEndpoint()); 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); } + registry.resolve(spec.getEndpoint()); ... } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that registry.resolve() is called before the allow-list check, which could leak information about available endpoints. Moving the allow-list check before resolve() is a valid security improvement that prevents unnecessary resolution and provides fail-fast behavior.	Low
General	Validate numeric coercion for fractional values The coerce method may produce incorrect results for numeric types when the value is a string representation of a floating-point number but the target type is INTEGER or LONG. For example, toNumber("3.14") returns a Double, and calling .intValue() on it truncates to 3 without validation. Consider adding explicit range and format checks before coercion to ensure the value is appropriate for the target type. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [182-214] 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 n = toNumber(value); + if (n.doubleValue() != n.intValue()) { + throw new IllegalArgumentException("value has fractional part"); + } + return integerValue(n.intValue()); } if (type == LONG) { - return longValue(toNumber(value).longValue()); + Number n = toNumber(value); + if (n.doubleValue() != n.longValue()) { + throw new IllegalArgumentException("value has fractional part"); + } + return longValue(n.longValue()); } if (type == DOUBLE) { return doubleValue(toNumber(value).doubleValue()); } if (type == BOOLEAN) { return booleanValue(toBoolean(value)); } } catch (IllegalArgumentException | ClassCastException e) { ... } return stringValue(String.valueOf(value)); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that toNumber("3.14").intValue() truncates without validation. However, the improved code's check n.doubleValue() != n.intValue() may have precision issues and doesn't validate range overflow. A more robust approach would check the string format or use stricter parsing.	Low
	Handle exceptions during endpoint row fetching The search method does not handle potential exceptions from endpoint.toRows(), which could throw during endpoint invocation or row shaping. If an endpoint handler fails or returns malformed data, the exception will propagate as an unhandled error. Consider wrapping the call in a try-catch block to provide a clearer error message that identifies the failing endpoint. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestRequest.java [43-55] @Override public List<ExprValue> search() { NodeClient nodeClient = client == null ? null : client.getNodeClient().orElse(null); RestEndpointContext ctx = RestEndpointContext.of(spec.getArgs(), nodeClient); - List<ExprValue> rows = endpoint.toRows(ctx, redactor); + List<ExprValue> rows; + try { + rows = endpoint.toRows(ctx, redactor); + } catch (Exception e) { + throw new IllegalStateException( + "Failed to fetch rows from rest endpoint [" + endpoint.getPath() + "]: " + e.getMessage(), e); + } if (spec.getCount() != null && spec.getCount() >= 0 && rows.size() > spec.getCount()) { return rows.subList(0, spec.getCount()); } return rows; } Suggestion importance[1-10]: 4 __ Why: The suggestion to wrap endpoint.toRows() in a try-catch for clearer error messages is reasonable. However, the existing code already has error handling in RestEndpointRegistry.coerce() that surfaces client errors (HTTP 400). Adding another wrapper may be redundant unless the goal is to enrich the error context with the endpoint path.	Low

Suggestions up to commit 5ed738d

Category	Suggestion	Impact
Possible issue	Validate hex digit conversion The fromHex method does not validate that Character.digit returns a valid digit (returns -1 for invalid hex characters). This can lead to incorrect byte values or unexpected behavior when decoding malformed hex strings. Add validation to ensure each character is a valid hexadecimal digit before processing. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [127-139] private static String fromHex(String h) { if (h.length() % 2 != 0) { throw new IllegalArgumentException("not a valid rest source token: odd-length hex body"); } byte[] bytes = new byte[h.length() / 2]; for (int i = 0; i < bytes.length; i++) { - bytes[i] = - (byte) - ((Character.digit(h.charAt(2 * i), 16) << 4) - + Character.digit(h.charAt(2 * i + 1), 16)); + int high = Character.digit(h.charAt(2 * i), 16); + int low = Character.digit(h.charAt(2 * i + 1), 16); + if (high == -1 || low == -1) { + throw new IllegalArgumentException("not a valid rest source token: invalid hex character"); + } + bytes[i] = (byte) ((high << 4) + low); } return new String(bytes, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 8 __ Why: The fromHex method in SystemIndexUtils does not validate that Character.digit returns valid digits (-1 indicates invalid hex). This could lead to incorrect byte values when decoding malformed tokens, potentially causing security or correctness issues when decoding REST endpoint specifications.	Medium
General	Reorder validation for clearer errors The restTable method calls registry.resolve(spec.getEndpoint()) but does not use its return value before performing allow-list validation. If resolve throws an exception for an unknown endpoint, the allow-list check never executes. Consider reordering to validate the allow-list first, providing clearer error messages when an endpoint is disabled versus unknown. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchStorageEngine.java [56-79] private Table restTable(String name) { RestSpec spec = decodeRestSpec(name); + List<String> allowed = settings.getSettingValue(Settings.Key.PPL_REST_ALLOWED_ENDPOINTS); + if (allowed == null || allowed.isEmpty()) { + throw new IllegalArgumentException("the rest command is disabled on this cluster"); + } + if (!(allowed.contains("*") || allowed.contains(spec.getEndpoint()))) { + throw new IllegalArgumentException( + "rest endpoint [" + + spec.getEndpoint() + + "] is not enabled on this cluster. Enabled endpoints: " + + allowed); + } RestEndpointRegistry registry = RestEndpointRegistryHolder.get(); if (registry == null) { throw new IllegalStateException( "the rest command endpoint registry is not initialized on this node"); } registry.resolve(spec.getEndpoint()); - 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); - } ... } Suggestion importance[1-10]: 6 __ Why: Reordering the allow-list check before registry.resolve would provide clearer error messages when an endpoint is disabled versus unknown. This improves user experience by distinguishing between "endpoint disabled" and "endpoint not in registry" errors, though the current order is functionally correct.	Low
	Improve numeric parsing error handling The toNumber method attempts to parse numeric strings but does not handle potential NumberFormatException from Double.parseDouble or Long.parseLong. While the caller catches these exceptions, the method should validate numeric format more robustly to prevent parsing edge cases like "1.2.3" or handle overflow scenarios explicitly. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [241-253] 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); + try { + if (s.indexOf('.') >= 0 || s.indexOf('e') >= 0 || s.indexOf('E') >= 0) { + return Double.parseDouble(s); + } + return Long.parseLong(s); + } catch (NumberFormatException e) { + throw new NumberFormatException("invalid number format: " + s); } - return Long.parseLong(s); } Suggestion importance[1-10]: 4 __ Why: While the suggestion to add explicit try-catch is reasonable, the caller already catches NumberFormatException and surfaces a clear client error. The additional try-catch provides marginal value and the method already handles edge cases like empty strings.	Low

Suggestions up to commit 8bb45cd

Category	Suggestion	Impact
General	Prevent memory retention from subList The count truncation logic creates a subList view that retains a reference to the entire original list, potentially causing memory retention issues if the original list is large. For better memory efficiency, consider creating a new list with only the required elements instead of using subList. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestRequest.java [47-59] @Override public List<ExprValue> search() { NodeClient nodeClient = client == null ? null : client.getNodeClient().orElse(null); RestEndpointContext ctx = RestEndpointContext.of(spec.getArgs(), nodeClient); List<ExprValue> rows = endpoint.toRows(ctx, redaction); if (spec.getCount() != null && spec.getCount() >= 0 && rows.size() > spec.getCount()) { - return rows.subList(0, spec.getCount()); + return new ArrayList<>(rows.subList(0, spec.getCount())); } return rows; } Suggestion importance[1-10]: 5 __ Why: Valid concern about subList retaining a reference to the original list. Creating a new ArrayList from the subList would prevent memory retention issues when the original list is large, improving memory efficiency.	Low
	Handle decimal-formatted integer values correctly The numeric parsing logic may produce incorrect results for edge cases. When a string contains a decimal point but represents an integer value (e.g., "3.0"), it will be parsed as a Double instead of maintaining integer semantics. Consider checking if the parsed double is actually an integer value before returning it as a Double. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [241-252] 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 (d == Math.floor(d) && !Double.isInfinite(d)) { + return (long) d; + } + return d; } return Long.parseLong(s); } Suggestion importance[1-10]: 3 __ Why: The suggestion addresses a potential edge case where "3.0" would be parsed as Double instead of Long. However, this is a minor optimization that may not align with the intended behavior of preserving the original numeric type representation from the endpoint response.	Low
Security	Avoid exposing raw values in errors The boolean parsing error message exposes the raw input value, which could leak sensitive information in logs or error responses. Consider sanitizing or limiting the exposed value in the error message to prevent potential information disclosure. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/rest/RestEndpointRegistry.java [255-270] private static boolean toBoolean(Object value) { if (value instanceof Boolean b) { return b; } String s = String.valueOf(value).trim(); if (s.isEmpty()) { throw new IllegalArgumentException("empty string is not a boolean"); } if (s.equalsIgnoreCase("true")) { return true; } if (s.equalsIgnoreCase("false")) { return false; } - throw new IllegalArgumentException("not a boolean: " + value); + throw new IllegalArgumentException("not a boolean value"); } Suggestion importance[1-10]: 4 __ Why: The suggestion addresses a potential information disclosure issue by removing the raw value from the error message. While the security impact is low (boolean values are unlikely to be sensitive), sanitizing error messages is a good practice for consistency.	Low

[github-actions]

Persistent review updated to latest commit 5ed738d

[github-actions]

Persistent review updated to latest commit 84f5773

[github-actions]

Persistent review updated to latest commit 7637f4d