Mirror upstream elastic/elasticsearch#137446 for AI review (snapshot of HEAD tree) #287
Conversation
BASE=da8b9c63ed1b60492fca13454dc362317baa5555 HEAD=646875a68d79136dd1f010dbc2a2fe565f063af1 Branch=main
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the cross-cluster search (CCS) initialization logic to handle multiple index patterns individually rather than concatenating them. The key changes include:
- Refactoring async iteration logic into a reusable
forAllhelper method - Processing each index pattern separately in
initCrossClusterStateto properly handle multiple patterns with potential exclusions - Using
IndexResolution.empty()instead of a valid resolution with empty mappings for unresolved pure CCS expressions - Adding
CrossProjectModeDeciderdependency throughout the stack (though currently unused)
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| EsqlSession.java | Refactored async iteration using new forAll helper, moved CCS initialization into preAnalyzeMainIndices, changed to use IndexResolution.empty() for unresolved indices, added CrossProjectModeDecider field |
| EsqlCCSUtils.java | Modified initCrossClusterState to process index patterns individually and merge index expressions per cluster when duplicates exist |
| TransportEsqlQueryAction.java | Instantiated CrossProjectModeDecider for dependency injection |
| TransportActionServices.java | Added crossProjectModeDecider parameter to record |
| IndexResolution.java | Added empty() factory method for creating empty index resolutions |
| EsqlTestUtils.java | Updated test fixtures with CrossProjectModeDecider |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private final RemoteClusterService remoteClusterService; | ||
| private final BlockFactory blockFactory; | ||
| private final ByteSizeValue intermediateLocalRelationMaxSize; | ||
| private final CrossProjectModeDecider crossProjectModeDecider; |
There was a problem hiding this comment.
The crossProjectModeDecider field is added and initialized but never used in this class. If this field is being added for future use, consider adding a TODO comment explaining the intended usage. Otherwise, if it's not needed yet, it should be removed to avoid dead code.
| private final CrossProjectModeDecider crossProjectModeDecider; |
| private static <T> void forAll( | ||
| Iterator<T> iterator, | ||
| PreAnalysisResult result, | ||
| TriConsumer<T, PreAnalysisResult, ActionListener<PreAnalysisResult>> consumer, | ||
| ActionListener<PreAnalysisResult> listener | ||
| ) { | ||
| if (iterator.hasNext()) { | ||
| consumer.apply(iterator.next(), result, listener.delegateFailureAndWrap((l, r) -> forAll(iterator, r, consumer, l))); | ||
| } else { | ||
| listener.onResponse(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
The forAll helper method lacks documentation. Add a JavaDoc comment explaining that this is a recursive async iterator pattern that processes elements sequentially while threading a result object through each iteration. Include parameter descriptions, especially noting that the consumer receives the current element, accumulated result, and a listener for continuation.
This change wires components necessary to perform conditional CPS index resolution.
Index resolution itself will be added after elastic#136632 is merged.