[Feature] Resolve cross-cluster index schemas through the remote cluster's GetMappings (#4551)

[AjimelecGonzalez]

Description

PPL queries on a remote cluster index (source=remote:logs-*) failed with no such index [logs-*], even though the same DSL query (_search) worked. PPL looked for the index's fields on the local cluster, which can't see remote clusters (opensearch-project/OpenSearch#6573). The only workaround was creating an index with the same name and fields on the local cluster.

With this change, PPL reads the fields of a remote index from the remote cluster itself, the same way it does for a local index. No local copy is needed, and local queries work as before.

What users get

• Queries on remote:index, *:index, and mixed local and remote indices work in PPL and Log Explorer.

• These cases return the same result as _search (the skipped-cluster warning arrives with Warn when a cross-cluster search skips a remote cluster #5851):

  Situation	PPL (this PR)	PPL after Warn when a cross-cluster search skips a remote cluster #5851 merges	_search
  Index missing on the remote (remote:nope)	404 no such index [nope]	Same	404 no such index [nope]
  Remote down, skip_unavailable: false	500 Unable to open any proxy connections to remote cluster [remote]	Same	500, same message
  One of several remotes down (or a local index in the query), skip_unavailable: true	200, rows from the other clusters, no warning	200, rows from the other clusters, and a warning that a cluster was skipped	200, hits from the other clusters, _clusters.skipped: 1

  The skipped-cluster warning comes from Warn when a cross-cluster search skips a remote cluster #5851, which reads _clusters.skipped from the search response. Until Warn when a cross-cluster search skips a remote cluster #5851 merges, the rows are correct but nothing says a cluster was skipped.

• A remote index's fields are cached for 60 seconds per user, so after the first query only the search itself goes to the remote cluster.

How it works: getIndexMappings and getIndexMaxResultWindows in the node client group names by cluster (with RemoteClusterService.groupIndices, as search does), read each remote cluster with its own client, and merge the results. Callers don't change. RemoteClusterService is passed into OpenSearchNodeClient as a supplier: createComponents runs before any TransportService exists, so the plugin returns a provider as a component and TransportPPLQueryAction fills it at startup.

Permissions: the remote role needs the same permissions as PPL on a local index (each tested by removing it):

• indices:admin/shards/search_shards
• indices:data/read/search
• indices:admin/mappings/get
• indices:monitor/settings/get

Two more are optional, indices:admin/resolve/index and indices:data/read/field_caps*. They let Log Explorer time ranges skip remote indices outside the range.

Known differences from _search

Situation	PPL	_search
Every remote in a source down, skip_unavailable: true, no local index	503 Remote cluster [remote] is unavailable and was skipped (skip_unavailable is true) (there are no fields to build the query from)	200, no hits, _clusters.skipped: 1
Pattern matches no index (remote:nope-*)	404 no such index [remote:nope-*], as PPL already does for local indices (#4342)	200, no hits
Unknown cluster name (typo:logs-*)	404 no such index [typo:logs-*] (treated as a local name, as in search)	200, no hits
Remote role lacks mappings/get or settings/get	403 no permissions for [indices:admin/mappings/get] ..., as PPL on a local index	Works (search doesn't need them)
Remote accepts the connection but never answers	504 after 10 seconds, Timeout waiting for task	Keeps waiting (no default timeout)
Remote goes down within 60 seconds of a successful query (fields still cached)	500, the same connection error wrapped by the search step (Failed to fetch data from the index ... Unable to open any proxy connections ...)	500 Unable to open any proxy connections ...

The local and remote clusters should also run the same version, because parts of a query can run on the remote cluster.

Docs: cross_cluster_search.md removes the old requirement for a local copy of each remote index, and explains what happens when a remote cluster is down and which permissions the remote role needs.

Testing

Unit tests:

• RemoteMappingsReaderTest: reading each remote cluster and prefixing its index names, the remote max_result_window, parsing identical mappings once, the per-user cache (including a large schema of many distinct mappings), skipping vs. failing an unreachable cluster, not caching answers with a skipped cluster, every cluster skipped and its message, and missing index, permission, and empty-pattern errors.
• OpenSearchNodeClientTest: local-only names never touch the remote service, local and remote mappings and settings merged, every remote skipped with and without local indices, a repeated name, and a date-math name with : staying local.
• OpenSearchDescribeIndexRequestTest: cluster:index names reach the client unchanged.

Integration tests, on two clusters with security, on both engines (each class has a Calcite variant), using indices that exist only on the remote:

• CrossClusterRemoteSchemaIT: remote-only index rows and aggregation, mixed local and remote indices, missing remote index, unreachable remote with and without skip_unavailable, a skipped remote next to a local index, and the cache not sharing one user's fields with another.
• CrossClusterRemoteSchemaCoverageIT: Log Explorer requests and time bounds over rollover indices, wildcards and *:index, type conflicts, nested fields, grouping by a text field's .keyword, mixed field types, SQL, describe, _explain, and a missing mapping permission.
• CrossClusterSearchIT: the old "fails without a local mapping" test now checks an index missing on the remote.

Related Issues

Resolves #4551
Related: opensearch-project/OpenSearch#6573

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 Reviewer Guide 🔍

(Review updated until commit fd07eff)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The cache key in RemoteMappingsReader includes the user object via Objects.toString(user, "") (line 86). If the user object's toString() method inadvertently includes sensitive information (e.g., credentials, tokens), this could be logged or exposed through cache introspection. Ensure the user object's toString() method does not leak sensitive data, or use a sanitized identifier instead.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The cache key includes the user object directly via Objects.toString(user, ""). If the user object's toString() method does not produce a stable, unique identifier across requests, different requests by the same user could miss the cache or, worse, different users could share cache entries if their toString() representations collide. This could lead to incorrect field-level security enforcement. List<Object> key = List.of(Objects.toString(user, ""), qualifiedNames(indicesByCluster)); Possible Issue The fetch method catches all exceptions and checks isSkipUnavailable to decide whether to skip a cluster. However, if remotes.isSkipUnavailable(clusterAlias) throws an exception (e.g., if the alias is not registered), this could propagate unexpectedly. The code assumes isSkipUnavailable always succeeds, but if the remote cluster configuration is inconsistent, this could fail. } Possible Issue The groupByCluster method calls remoteClusters.get() which can throw IllegalStateException if the service is not yet available. While the provider is filled at startup, if a query arrives before TransportPPLQueryAction runs (e.g., during plugin initialization or in a race condition), this will fail. The code does not handle this scenario. .groupIndices(DEFAULT_INDICES_OPTIONS, indexExpression, index -> false)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to fd07eff

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use immutable key type for cache The cache uses List as a key, which is mutable and can lead to unpredictable behavior if the list is modified after being used as a key. Use an immutable collection type like ImmutableList or create a dedicated immutable key class to ensure cache integrity and prevent potential bugs from key mutation. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteMappingsReader.java [58-66] -private static final Cache<List<Object>, RemoteMetadata> CACHE = +private static final Cache<ImmutableList<Object>, RemoteMetadata> CACHE = CacheBuilder.newBuilder() .concurrencyLevel(1) .maximumWeight(MAX_TOTAL_FIELDS) - .weigher((List<Object> key, RemoteMetadata metadata) -> weigh(metadata)) + .weigher((ImmutableList<Object> key, RemoteMetadata metadata) -> weigh(metadata)) .expireAfterWrite(TTL_SECONDS, TimeUnit.SECONDS) .build(); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that using List<Object> as a cache key could be problematic if the list were mutable. However, the code at line 86 uses List.of() which returns an immutable list, so the concern is partially mitigated. Still, explicitly using ImmutableList would make the immutability contract clearer and more robust.	Medium
	Match cache key type with declaration The cache key construction uses List.of() which returns an immutable list, but the previous cache declaration expects List. Ensure consistency by using ImmutableList.of() or updating the cache key type to match the immutable nature of the key being created. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteMappingsReader.java [85-88] RemoteMetadata read(Object user, Map<String, String[]> indicesByCluster) { - List<Object> key = List.of(Objects.toString(user, ""), qualifiedNames(indicesByCluster)); + ImmutableList<Object> key = ImmutableList.of(Objects.toString(user, ""), qualifiedNames(indicesByCluster)); try { return CACHE.get(key, () -> fetch(indicesByCluster)); Suggestion importance[1-10]: 6 __ Why: This suggestion is related to the first one and correctly points out the inconsistency between using List.of() and the cache declaration. Using ImmutableList.of() would make the code more consistent and explicit about immutability, though List.of() already returns an immutable list in practice.	Low
General	Handle all expected exception types The method catches only AllClustersSkipped exception but RemoteMappingsReader.read() can throw other exceptions like IndexNotFoundException and OpenSearchSecurityException. These uncaught exceptions will propagate unexpectedly. Add explicit handling or document that these exceptions are intentionally allowed to propagate. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [180-192] private RemoteMappingsReader.RemoteMetadata readRemote(IndexGroups groups) { try { return new RemoteMappingsReader(client, remoteClusters.get(), REMOTE_METADATA_TIMEOUT) .read( client.threadPool().getThreadContext().getTransient(SECURITY_USER_INFO), groups.remote()); } catch (RemoteMappingsReader.AllClustersSkipped e) { if (groups.local().length == 0) { throw IndexMappingErrors.allClustersSkipped(e.getMessage(), e.indexNames); } return new RemoteMappingsReader.RemoteMetadata(Map.of(), Map.of()); + } catch (ErrorReport e) { + throw e; } } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the code flow. The IndexNotFoundException and OpenSearchSecurityException are already handled within RemoteMappingsReader.fetch() and converted to ErrorReport exceptions before being thrown. The readRemote() method only needs to catch AllClustersSkipped specifically. Adding a catch for ErrorReport that just re-throws it adds no value.	Low

---

Previous suggestions

Suggestions up to commit 4b345ec

Category	Suggestion	Impact
Possible issue	Ensure cache key immutability The cache key uses List which includes mutable lists. If the list contents are modified after being used as a key, cache lookups will fail. Consider using immutable collections or defensive copies to prevent cache corruption. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteMappingsReader.java [64-69] private static final Cache<List<Object>, RemoteMetadata> CACHE = CacheBuilder.newBuilder() .maximumWeight(MAX_TOTAL_FIELDS) .weigher((List<Object> key, RemoteMetadata metadata) -> weigh(metadata)) .expireAfterWrite(TTL_SECONDS, TimeUnit.SECONDS) .build(); +// In the read method, ensure immutability: +List<Object> key = List.of(Objects.toString(user, ""), List.copyOf(Arrays.asList(sorted(indexExpression)))); + Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue with cache key mutability. The read method already uses List.of() which creates immutable lists, but the suggestion to use List.copyOf() for the inner list would provide additional safety against modifications to the sorted array.	Medium
General	Preserve exception chain for debugging The exception handling unwraps and re-throws exceptions, but loses the original exception chain when wrapping in IllegalStateException. This makes debugging harder. Preserve the full exception chain by passing the original exception as the cause. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteMappingsReader.java [86-97] } catch (UncheckedExecutionException | ExecutionException e) { if (e.getCause() instanceof NotCached notCached) { CalcitePlanContext.addWarning(skippedWarning(notCached.skipped)); return notCached.metadata; } if (e.getCause() instanceof RuntimeException runtime) { throw runtime; } - throw new IllegalStateException(e.getCause()); + throw new IllegalStateException("Failed to fetch remote metadata", e); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the exception chain is lost when wrapping in IllegalStateException. Adding the original exception as a cause would improve debugging capabilities without changing the control flow.	Low
	Avoid exceptions for control flow Using exceptions for control flow (returning partial results) is an anti-pattern that impacts performance and code clarity. Consider using a wrapper class or Optional to return partial results instead of throwing and catching exceptions. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteMappingsReader.java [199-208] -static final class NotCached extends RuntimeException { - final transient RemoteMetadata metadata; - final transient List<String> skipped; +static final class PartialResult { + final RemoteMetadata metadata; + final List<String> skipped; - NotCached(RemoteMetadata metadata, List<String> skipped) { - super(null, null, false, false); + PartialResult(RemoteMetadata metadata, List<String> skipped) { this.metadata = metadata; this.skipped = skipped; } } +// Modify the cache loader to return Either<RemoteMetadata, PartialResult> +// or use a custom result wrapper instead of throwing NotCached + Suggestion importance[1-10]: 5 __ Why: While using exceptions for control flow is generally an anti-pattern, in this case it's used to bypass Guava's cache mechanism for partial results that shouldn't be cached. The suggestion is valid but would require significant refactoring of the caching strategy, and the current approach is acceptable given the specific use case.	Low
	Enable stack trace generation for debugging The constructor disables stack trace generation and suppression, which may hinder debugging. Consider enabling stack traces in development environments or providing a factory method that allows conditional stack trace generation based on configuration. core/src/main/java/org/opensearch/sql/executor/EmptyResultException.java [21-24] public EmptyResultException(Warning warning) { - super(warning.getMessage(), null, false, false); + super(warning.getMessage()); this.warning = warning; } Suggestion importance[1-10]: 3 __ Why: While enabling stack traces could aid debugging, the current implementation intentionally disables them for performance reasons since this is an expected control flow exception. The suggestion is valid but has low impact since the exception message and warning already provide sufficient context.	Low

Suggestions up to commit 734ced6

Category	Suggestion	Impact
General	Verify parent node existence The logic assumes that sorting by dot count ensures parents are processed before children, but this may fail for fields like a.b and a.b.c.d where the parent a.b.c is missing from fieldTypes. If a deeply nested field appears without all its ancestors, the code may attempt to access a non-existent parent node, causing incorrect mapping structure. opensearch/src/main/java/org/opensearch/sql/opensearch/mapping/FieldCapsIndexMappings.java [84-99] fieldTypes.keySet().stream() .sorted( Comparator.comparingLong((String name) -> name.chars().filter(c -> c == '.').count()) .thenComparing(Comparator.naturalOrder())) .forEach( name -> { String type = fieldTypes.get(name); int dot = name.lastIndexOf('.'); String parent = dot < 0 ? null : name.substring(0, dot); String leaf = name.substring(dot + 1); + + if (parent != null && !CONTAINER_TYPES.contains(fieldTypes.getOrDefault(parent, "object"))) { + Map<String, Object> parentNode = nodes.get(parent); + if (parentNode != null) { + child(parentNode, "fields", leaf).put("type", type); + return; + } + } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a potential issue where a parent node might not exist when processing multi-fields. However, the existing code already handles missing parents by creating them in the loop that processes all parts of the parent path (lines 105-110). The suggested null check would only partially address the concern and doesn't improve upon the existing logic that ensures all ancestor nodes are created.	Low
	Handle thread context access safely The getTransient(SECURITY_USER_INFO) call may return null when security is disabled, but this null value is used directly as a cache key component. While RemoteFieldCapsCache.get() handles null users, verify that the thread context access is safe and won't throw exceptions in non-security environments. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [158] +Object securityUser = null; +try { + securityUser = client.threadPool().getThreadContext().getTransient(SECURITY_USER_INFO); +} catch (Exception e) { + // Security context not available, proceed with null user +} schema = RemoteFieldCapsCache.get( - client.threadPool().getThreadContext().getTransient(SECURITY_USER_INFO), + securityUser, indexExpression, () -> { FieldCapabilitiesResponse response = client .fieldCaps( new FieldCapabilitiesRequest().indices(indexExpression).fields("*")) .actionGet(REMOTE_METADATA_TIMEOUT); return RemoteSchema.of(response.getIndices(), response.get()); }); Suggestion importance[1-10]: 2 __ Why: The suggestion adds defensive exception handling around getTransient(), but there's no evidence this call throws exceptions in non-security environments. The code already handles null users correctly via Objects.toString(user, "") in the cache. Adding a try-catch without a specific exception type or known failure mode is speculative and adds unnecessary complexity.	Low
Possible issue	Fix cache key equality semantics The cache key construction using List.of(Objects.toString(user, ""), List.of(sorted(indexExpression))) creates nested lists that may not provide stable equality semantics. If sorted(indexExpression) returns the same array reference, the inner List.of() will create different list instances with different identity, potentially causing cache misses for identical queries. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteFieldCapsCache.java [65-66] static RemoteSchema get(Object user, String[] indexExpression, Supplier<RemoteSchema> fetch) { - List<Object> key = List.of(Objects.toString(user, ""), List.of(sorted(indexExpression))); + List<Object> key = List.of(Objects.toString(user, ""), Arrays.asList(sorted(indexExpression))); try { RemoteSchema response = CACHE.get( key, () -> { RemoteSchema fresh = fetch.get(); if (fresh.isEmpty()) { throw new NotCached(fresh); } return fresh; }); return response; } catch (UncheckedExecutionException | ExecutionException | ExecutionError e) { Throwable cause = e.getCause(); if (cause instanceof NotCached notCached) { return notCached.response; } if (cause instanceof RuntimeException runtime) { throw runtime; } if (cause instanceof Error error) { throw error; } throw new IllegalStateException(cause); } } Suggestion importance[1-10]: 3 __ Why: The concern about nested list equality is valid, but List.of() creates immutable lists with proper equals() implementation based on content, not identity. The sorted() method returns a new array each time, so List.of(sorted(...)) creates lists that compare equal when their contents are equal. The suggestion to use Arrays.asList() doesn't meaningfully improve the code.	Low

Suggestions up to commit 2a44d92

Category	Suggestion	Impact
General	Handle edge case for field names When dot is -1 (no dot found), name.substring(dot + 1) returns name.substring(0), which is the entire name. However, this logic is correct. The real issue is that if a field name starts with a dot (e.g., ".hidden"), lastIndexOf('.') returns 0, and substring(0, 0) produces an empty parent string, which may cause unexpected behavior. opensearch/src/main/java/org/opensearch/sql/opensearch/mapping/FieldCapsIndexMappings.java [89-93] static Map<String, Object> toProperties(Map<String, String> fieldTypes) { Map<String, Object> root = new LinkedHashMap<>(); Map<String, Map<String, Object>> nodes = new HashMap<>(); fieldTypes.keySet().stream() .sorted( Comparator.comparingLong((String name) -> name.chars().filter(c -> c == '.').count()) .thenComparing(Comparator.naturalOrder())) .forEach( name -> { String type = fieldTypes.get(name); int dot = name.lastIndexOf('.'); - String parent = dot < 0 ? null : name.substring(0, dot); - String leaf = name.substring(dot + 1); + String parent = (dot <= 0) ? null : name.substring(0, dot); + String leaf = (dot < 0) ? name : name.substring(dot + 1); Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential edge case where field names starting with a dot could produce an empty parent string. However, this is a very unlikely scenario in practice since OpenSearch field names typically don't start with dots (metadata fields like _id are filtered out earlier). The fix is technically correct but addresses a minor edge case that may never occur in real usage.	Low

Suggestions up to commit 2cba009

Category	Suggestion	Impact
Possible issue	Prevent cache key collisions The cache key construction using string concatenation is vulnerable to collisions. A user named "alice" with index "x|y" produces the same key as user "alice|x" with index "y". Use a delimiter that cannot appear in usernames or index names, or employ a more robust key structure. opensearch/src/main/java/org/opensearch/sql/opensearch/client/RemoteFieldCapsCache.java [64-65] static RemoteSchema get(Object user, String[] indexExpression, Supplier<RemoteSchema> fetch) { - String key = Objects.toString(user, "") + "|" + String.join(",", sorted(indexExpression)); + String key = Objects.toString(user, "") + "\u0000" + String.join(",", sorted(indexExpression)); try { RemoteSchema response = CACHE.get( key, () -> { RemoteSchema fresh = fetch.get(); if (fresh.isEmpty()) { throw new NotCached(fresh); } return fresh; }); return response; } catch (UncheckedExecutionException | ExecutionException | ExecutionError e) { Throwable cause = e.getCause(); if (cause instanceof NotCached notCached) { return notCached.response; } ... } } Suggestion importance[1-10]: 8 __ Why: The current key construction using "|" as a delimiter can cause collisions when usernames or index names contain the pipe character. Using a null character (\u0000) as a delimiter is a robust solution since it cannot appear in valid strings, preventing potential cache key collisions that could lead to incorrect schema retrieval.	Medium
General	Handle timeout exceptions explicitly The actionGet(REMOTE_METADATA_TIMEOUT) call can throw OpenSearchTimeoutException when the remote cluster is slow. This exception is not caught, causing it to propagate as an IllegalStateException instead of a more specific timeout error. Add explicit handling for timeout exceptions. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [153-179] RemoteSchema schema; try { schema = RemoteFieldCapsCache.get( client.threadPool().getThreadContext().getTransient(SECURITY_USER_INFO), indexExpression, () -> { FieldCapabilitiesResponse response = client .fieldCaps( new FieldCapabilitiesRequest().indices(indexExpression).fields("*")) .actionGet(REMOTE_METADATA_TIMEOUT); return RemoteSchema.of(response.getIndices(), response.get()); }); } catch (IndexNotFoundException e) { throw RemoteIndicesNotFound.indexNotFound(e, String.join(",", indexExpression)); } catch (OpenSearchSecurityException e) { throw RemoteIndicesNotFound.permissionDenied(e, String.join(",", indexExpression)); +} catch (OpenSearchTimeoutException e) { + throw new IllegalStateException( + "Timeout while fetching field capabilities for [" + + String.join(",", indexExpression) + + "]: " + + e.getMessage(), + e); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that OpenSearchTimeoutException should be caught explicitly to provide clearer error messages. However, the test at line 301-309 shows this is already being tested and handled as an IllegalStateException, so the current behavior may be intentional. Adding explicit handling would improve clarity but is not critical.	Medium
	Clarify leaf name extraction logic When dot is -1 (no dot found), name.substring(dot + 1) returns the entire string starting from index 0, which is correct. However, the logic is unclear and could be misread. Use a more explicit approach to extract the leaf name when no parent exists. opensearch/src/main/java/org/opensearch/sql/opensearch/mapping/FieldCapsIndexMappings.java [89-93] static Map<String, Object> toProperties(Map<String, String> fieldTypes) { Map<String, Object> root = new LinkedHashMap<>(); Map<String, Map<String, Object>> nodes = new HashMap<>(); fieldTypes.keySet().stream() .sorted( Comparator.comparingLong((String name) -> name.chars().filter(c -> c == '.').count()) .thenComparing(Comparator.naturalOrder())) .forEach( name -> { String type = fieldTypes.get(name); int dot = name.lastIndexOf('.'); String parent = dot < 0 ? null : name.substring(0, dot); - String leaf = name.substring(dot + 1); + String leaf = dot < 0 ? name : name.substring(dot + 1); Suggestion importance[1-10]: 4 __ Why: While the suggested change makes the code slightly more explicit, the current implementation is functionally correct and the logic is standard for substring operations. The improvement in readability is marginal and does not address any bug or significant maintainability issue.	Low

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 2a44d92.

Path	Line	Severity	Description
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterRemoteSchemaIT.java	49	low	Hardcoded password 'correcthorsebatterystaple' is used to create test users on both local and remote clusters via the security API. While this is a test file and the password is a well-known example, hardcoded credentials in integration tests risk persisting in shared/staging environments if test teardown fails.
integ-test/src/test/java/org/opensearch/sql/security/CrossClusterRemoteSchemaCoverageIT.java	50	low	Same hardcoded password 'correcthorsebatterystaple' repeated in a second test class, creating users on both clusters. Duplicate hardcoded credential increases the surface area of the issue if test environments are long-lived or shared.

The table above displays the top 10 most important findings.

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

---

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]

Persistent review updated to latest commit 2a44d92

[github-actions]

Persistent review updated to latest commit 734ced6

[codecov]

Codecov Report

❌ Patch coverage is 95.56962% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.78%. Comparing base (866dafc) to head (fd07eff).

Files with missing lines	Patch %	Lines
...ch/sql/opensearch/client/RemoteMappingsReader.java	96.80%	1 Missing and 2 partials ⚠️
...ch/sql/opensearch/client/OpenSearchNodeClient.java	95.00%	1 Missing and 1 partial ⚠️
...pensearch/client/RemoteClusterServiceProvider.java	71.42%	1 Missing and 1 partial ⚠️

❌ Your project check has failed because the head coverage (63.78%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5844      +/-   ##
============================================
+ Coverage     63.66%   63.78%   +0.11%     
- Complexity     9022     9063      +41     
============================================
  Files           959      962       +3     
  Lines         40836    40974     +138     
  Branches       4635     4653      +18     
============================================
+ Hits          26000    26136     +136     
+ Misses        13964    13962       -2     
- Partials        872      876       +4     

Flag	Coverage Δ
sql-engine	63.78% <95.56%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 4b345ec

[penghuo]

Does TimeBound ":" is removed from index names before call indexNames? cc: @ahkcs

[ahkcs]

Yes. Since #5852 the bounds are a single <field,start,end> suffix on the last source, and OpenSearchStorageEngine.getTable strips it with TimeBounds.decode before the name reaches the client.

[AjimelecGonzalez]

Thanks @ahkcs for confirming. TimeBounds.decode in getTable strips the suffix before the name reaches the client, so only real cluster:index names are grouped.

[ahkcs]

A skipped remote in a subsearch empties the whole query. With dead unreachable and
skip_unavailable=true:

• source=bank,dead:logs-* | head 3 → 3 rows + warning
• source=bank | head 3 | append [ source=dead:logs-* ] → 200, no columns, no rows
• source=bank | head 3 | lookup dead:lk account_number → same

This check runs per relation, but QueryService replaces the whole response with an empty one.

[AjimelecGonzalez]

EmptyResultException is removed. Thanks for the catch.

When every remote in a source is skipped and no local index is named, schema resolution now fails with 503 Remote cluster [dead] is unavailable and was skipped (skip_unavailable is true), so append [ source=dead:logs-* ] and lookup dead:lk … return that error instead of an empty response. A skipped remote next to a local index (source=bank,dead:logs-*) still returns the local rows, covered by testSkippedRemoteWithLocalIndexReturnsTheLocalRows.

[ahkcs]

Set.of throws on duplicates, and groupIndices keeps them.
source=bank,bank,remote:bank | stats count() returns 14 on main and 400 duplicate element: bank here.
Use Set.copyOf or a HashSet.

[AjimelecGonzalez]

Fixed by the regrouping: names now come straight from groupIndices, with no Set, so source=bank,bank,remote:bank works as on main. A unit test covers a repeated local name next to a remote one (get_index_mappings_accepts_a_repeated_local_name_with_remote_names).

[ahkcs]

Entries weighing more than ~25,000 (Indices × 1,000 fields) are never cached: Guava splits maximumWeight across 4 segments and evicts any entry heavier than one segment right after loading it. In a unit test with N distinct 1,000-field mappings read twice, 24 indices take 1 fetch and 26 take 2. Set
concurrencyLevel(1).

[AjimelecGonzalez]

Done, thanks. concurrencyLevel(1), plus largeSchemaWithManyDistinctMappingsIsCached (30 distinct 1,000-field mappings read twice, one fetch). The test fails without the fix.

[github-actions]

Persistent review updated to latest commit fd07eff