resolve remotes with field caps call #133947
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this comment.
Thanks @idegtiarenko, it looks good in general.
I left just a couple of comments regarding some corner cases (maybe a bit paranoid)
| private void doResolveRemotes(List<IndexPattern> indexPatterns, QueryBuilder requestFilter, ActionListener<Set<String>> listener) { | ||
| switch (indexPatterns.size()) { | ||
| case 0 -> listener.onResponse(Set.of()); | ||
| case 1 -> indexResolver.resolveConcreteIndices( |
There was a problem hiding this comment.
If the index pattern also contains a cluster/project prefix, will this still be correct?
eg.
FROM remote1:idx1,remote2:idx2
| STATS max(a) by b
| ENRICH policy on a, b
The enrich can only be calculated on the coordinator (after the STATS reduction), but I guess the enrich index on the coordinator won't be resolved. I'm not sure it works at all today tbh, maybe worth checking.
There was a problem hiding this comment.
If the index pattern also contains a cluster/project prefix, will this still be correct?
Yes, this is a valid situation, where all supplied indices are correct specific indices (not aliases nor patterns) and exist.
In such cases we would resolve to exact same 2 identifiers and would derive 2 remotes from them: remote1 and remote2
| requestFilter, | ||
| listener.map(EsqlCCSUtils::getRemotesOf) | ||
| ); | ||
| default -> listener.onFailure(new MappingException("Queries with multiple indices are not supported")); |
There was a problem hiding this comment.
This is OK for now, but it will change when we start supporting subqueries and proper JOINs.
I'm not sure how it will actually work, maybe the enriches in a subquery will have to consider the remotes from the main indices in the subquery itself.
Anyway, I'd add a // TODO here, so that we don't forget.
There was a problem hiding this comment.
Nit: the error message is a bit misleading, it took me a couple of seconds to realize that we were referring to multiple FROM, not to multiple indices. I know it comes from a similar check in EsqlSession, probably we should change both.
There was a problem hiding this comment.
I copied it from
but that is a good call.
This is OK for now, but it will change when we start supporting subqueries and proper JOINs.
I suspect we would still need a distinction between main/top-level from and all the nested ones.
I will think about a way to improve it (across all usages)
This change updates enriches to resolve remotes using dedicated field caps rather than
executionInfo.getClusters().keySet().This is required in order to be able to resolve flat index expressions.
Related to: ES-12612