[Backport 3.9] Resolve cross-cluster index schemas through the remote cluster's GetMappings (#5844)

[AjimelecGonzalez]

Description

Backport of #5844 to 3.9: PPL reads the mapping and settings of cluster:index names from each remote cluster, so cross-cluster queries such as source=remote:logs-* work without a local copy of the index.

The automatic backport failed on two conflicts, resolved by hand:

• SQLPlugin.java and TransportPPLQueryAction.java: 3.9 doesn't have async query support ([FEATURE] Async PPL query submission and result retrieval #5832), so only the RemoteClusterServiceProvider parameter and wiring from [Feature] Resolve cross-cluster index schemas through the remote cluster's GetMappings (#4551) #5844 are added. The other 15 files cherry-pick unchanged and match main.

Tested on 3.9: spotlessCheck, all unit tests, the full integTestWithSecurity (including the 98 cross-cluster tests), and the full integTest pass.

Related Issues

Backport of #5844

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 🔍

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 cache key includes the user as a string via Objects.toString(user, ""). If user is null when security is off, the key becomes an empty string. If two different requests both have null user but different index patterns, they share the same first element in the key, which could lead to cache collisions or unexpected behavior if the cache logic relies on unique user identification. List<Object> key = List.of(Objects.toString(user, ""), qualifiedNames(indicesByCluster)); Possible Issue In the fetch method, when a remote cluster is skipped, the code adds the cluster alias to the skipped list but continues processing other clusters. If all clusters are skipped and mappings.isEmpty() is true, an AllClustersSkipped exception is thrown. However, if some mappings were retrieved before a cluster was skipped, the partial result is thrown as NotCached. This logic assumes that once any mapping is found, the result is partial if any cluster is skipped. If the first cluster succeeds and the second is skipped, the result is marked as partial and not cached, even if the first cluster's data is complete. This could lead to unnecessary cache misses. private RemoteMetadata fetch(Map<String, String[]> indicesByCluster) { String indexNames = String.join(",", qualifiedNames(indicesByCluster)); Map<String, IndexMapping> mappings = new LinkedHashMap<>(); Map<String, Integer> windows = new LinkedHashMap<>(); // Rollover indices usually share one mapping: parse it once and share the result, so cost and // memory follow the number of distinct mappings rather than the number of indices. Map<MappingMetadata, IndexMapping> parsed = new HashMap<>(); List<String> skipped = new ArrayList<>(); indicesByCluster.forEach( (clusterAlias, indices) -> { try { Client remote = client.getRemoteClusterClient(clusterAlias); GetMappingsResponse mappingsResponse = remote.admin().indices().prepareGetMappings(indices).setLocal(true).get(timeout); GetSettingsResponse settingsResponse = remote .admin() .indices() .getSettings( new GetSettingsRequest().indices(indices).local(true).includeDefaults(true)) .actionGet(timeout); mappingsResponse .mappings() .forEach( (index, mapping) -> mappings.put( RemoteClusterAware.buildRemoteIndexName(clusterAlias, index), parsed.computeIfAbsent(mapping, IndexMapping::new))); settingsResponse .getIndexToSettings() .forEach( (index, settings) -> windows.put( RemoteClusterAware.buildRemoteIndexName(clusterAlias, index), settings.getAsInt( IndexSettings.MAX_RESULT_WINDOW_SETTING.getKey(), IndexSettings.MAX_RESULT_WINDOW_SETTING.getDefault( Settings.EMPTY)))); } catch (Exception e) { Throwable cause = ExceptionsHelper.unwrapCause(e); // A missing index or a denied read is the remote's answer, not an outage. if (cause instanceof IndexNotFoundException notFound) { throw IndexMappingErrors.indexNotFound(notFound, indexNames); } if (cause instanceof OpenSearchSecurityException denied) { throw IndexMappingErrors.permissionDenied(denied, indexNames); } if (!remotes.isSkipUnavailable(clusterAlias)) { throw e instanceof RuntimeException runtime ? runtime : new IllegalStateException(e); } skipped.add(clusterAlias); } }); if (mappings.isEmpty()) { if (!skipped.isEmpty()) { throw new AllClustersSkipped(skippedMessage(skipped), indexNames); } throw IndexMappingErrors.indexNotFound(new IndexNotFoundException(indexNames), indexNames); } if (!skipped.isEmpty()) { // Partial answer during an outage: serve it, but don't cache it. The search that follows // reports the skipped cluster. throw new NotCached(new RemoteMetadata(mappings, windows)); } return new RemoteMetadata(mappings, windows); Possible Issue The groupByCluster method checks if any index name contains a colon to decide whether to call remoteClusters.get(). If no colon is found, it returns early with all names as local. However, if a colon is present but the prefix is not a registered remote cluster, the name is treated as local by groupIndices. The early return optimization is correct, but if remoteClusters.get() is called and throws an exception (e.g., if the service is not yet available), the error message may be confusing. The code should ensure that remoteClusters.get() is only called when necessary and that the provider is always available when remote names are present. private IndexGroups groupByCluster(String... indexExpression) { if (Arrays.stream(indexExpression) .noneMatch(name -> name.indexOf(RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR) >= 0)) { return new IndexGroups(indexExpression, Map.of()); } Map<String, String[]> byCluster = new LinkedHashMap<>(); remoteClusters .get() .groupIndices(DEFAULT_INDICES_OPTIONS, indexExpression, index -> false) .forEach((clusterAlias, indices) -> byCluster.put(clusterAlias, indices.indices())); String[] local = byCluster.remove(RemoteClusterService.LOCAL_CLUSTER_GROUP_KEY); return new IndexGroups(local == null ? new String[0] : local, byCluster); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use immutable cache key type The cache uses List as a key, which includes mutable user info. If the user object is modified after being used as a cache key, cache lookups will fail. Consider using immutable key components or creating a defensive copy of the user info to prevent cache misses due to external modifications. 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<CacheKey, RemoteMetadata> CACHE = CacheBuilder.newBuilder() .concurrencyLevel(1) .maximumWeight(MAX_TOTAL_FIELDS) - .weigher((List<Object> key, RemoteMetadata metadata) -> weigh(metadata)) + .weigher((CacheKey key, RemoteMetadata metadata) -> weigh(metadata)) .expireAfterWrite(TTL_SECONDS, TimeUnit.SECONDS) .build(); +private record CacheKey(String user, List<String> indices) {} + Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that using List<Object> as a cache key could be problematic if the user object is mutable. However, the code already converts the user to a string with Objects.toString(user, "") in the read method (line 86), which makes it effectively immutable. The suggestion would improve code clarity but doesn't fix a critical issue.	Low
	Clarify empty metadata return intent When all remote clusters are skipped but local indices exist, the method returns empty mappings/windows. However, the caller merges these empty maps with local results, which is correct. Consider adding a comment explaining this behavior to clarify that returning empty maps is intentional and safe. 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); } + // All remotes skipped but local indices exist: return empty remote metadata + // so the query proceeds with local indices only return new RemoteMappingsReader.RemoteMetadata(Map.of(), Map.of()); } } Suggestion importance[1-10]: 4 __ Why: Adding a comment to clarify the intentional behavior of returning empty metadata when all remotes are skipped but local indices exist would improve code readability and maintainability. This is a minor documentation improvement that helps future developers understand the logic.	Low

[codecov]

Codecov Report

❌ Patch coverage is 95.56962% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.10%. Comparing base (d62098c) to head (0908acc).

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.10%) 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              @@
##                3.9    #5869      +/-   ##
============================================
+ Coverage     62.98%   63.10%   +0.12%     
- Complexity     8824     8865      +41     
============================================
  Files           939      942       +3     
  Lines         40156    40294     +138     
  Branches       4514     4532      +18     
============================================
+ Hits          25291    25427     +136     
+ Misses        14058    14056       -2     
- Partials        807      811       +4     

Flag	Coverage Δ
sql-engine	63.10% <95.56%> (+0.12%)	⬆️

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.