Reuse grouped indices #137879
Reuse grouped indices #137879
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| Map<String, OriginalIndices> clusterToOriginalIndices = transportService.getRemoteClusterService() | ||
| .groupIndices(SearchRequest.DEFAULT_INDICES_OPTIONS, PlannerUtils.planOriginalIndices(physicalPlan)); | ||
| // Gets the original indices specified in the FROM command of the query. We need the original query to resolve alias filters. | ||
| Map<String, OriginalIndices> clusterToOriginalIndices = getIndices(execInfo, EsqlExecutionInfo.Cluster::getOriginalIndices); |
There was a problem hiding this comment.
It is not possible to group original expressions in flat/CPS since cluster prefixes are not explicit.
EsqlExecutionInfo contains the necessary information after performing FC, we could take it from there.
Additionally this would avoid the need to traverse plane and group indices again and will make this operation a tiny bit cheaper.
| .groupIndices(SearchRequest.DEFAULT_INDICES_OPTIONS, PlannerUtils.planConcreteIndices(physicalPlan).toArray(String[]::new)); | ||
|
|
||
| // Returns the concrete/resolved indices used in the FROM command of the query. | ||
| Map<String, OriginalIndices> clusterToConcreteIndices = getIndices(execInfo, EsqlExecutionInfo.Cluster::getConcreteIndices); |
There was a problem hiding this comment.
Grouping concrete indices should be possible, but it is preferable to store result in EsqlExecutionInfo instead of traversing plan and grouping indices again (especially if we have a pattern resolved to hundreds of indices).
| /** | ||
| * This cluster's concrete/resolved indices. | ||
| */ | ||
| private final String concreteIndices; |
There was a problem hiding this comment.
nit: I wonder if this should be a String[] instead. Probably it depends on how we'll use it in the future.
There was a problem hiding this comment.
I was considering using array, however I am concerned that it is mutable opposed to plain string
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.3.csv # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlExecutionInfo.java
idegtiarenko
force-pushed
the
reuse_groupped_indices
branch
from
3e2d19d to
e61dbd8
Compare
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.3.csv
dnhatn
left a comment
dnhatn
left a comment
There was a problem hiding this comment.
I've left some small comments. Thanks @idegtiarenko.
| out.writeString(indexExpression); | ||
| out.writeString(originalIndices); | ||
| if (out.getTransportVersion().supports(CONCRETE_INDICES_VERSION)) { | ||
| out.writeString(concreteIndices); |
There was a problem hiding this comment.
Do we need to serialize this field? If so, we need to handle nulls from line 526.
| return Maps.transformValues( | ||
| executionInfo.getClusters(), | ||
| cluster -> new OriginalIndices( | ||
| Strings.commaDelimitedListToStringArray(getter.apply(cluster)), |
There was a problem hiding this comment.
This is only called once, but I'm not sure this approach. We concatenate an array of concrete indices into a single string, then split them here. Can we store them as a Set (shared with indexNameWithModes from EsRelation) or as an array, but not as a single string?
4 participants
This change reuse previously grouped indices during query execution phase.