-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Resolve indices using original index pattern #134218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e0f5989
9e56546
e172c13
62cb8e9
d653020
21ccd63
24b7a4c
b1e75f4
3029bfb
58c8230
ee777a4
06bfcfa
3c9a40b
cf31115
e9a0e29
880f169
444217d
e5254ec
685830f
adaa593
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -695,15 +695,14 @@ private void preAnalyzeMainIndices( | |
ThreadPool.Names.SYSTEM_READ | ||
); | ||
if (preAnalysis.indexPattern() != null) { | ||
String indexExpressionToResolve = EsqlCCSUtils.createIndexExpressionFromAvailableClusters(executionInfo); | ||
if (indexExpressionToResolve.isEmpty()) { | ||
// if this was a pure remote CCS request (no local indices) and all remotes are offline, return an empty IndexResolution | ||
if (executionInfo.clusterAliases().isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're back to the same question of whether this is right or not when we call it the second time. Is this intentional to have it here, or it's just sneaked in from merging the previous patch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to #135353 (comment)
There are several aspects to this question:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All considered, IMHO this seems reasonable.
ES-12978 is for CPS, but I guess we have the same problem with CCS now.
++ |
||
// return empty resolution if the expression is pure CCS and resolved no remote clusters (like no-such-cluster*:index) | ||
listener.onResponse( | ||
result.withIndices(IndexResolution.valid(new EsIndex(preAnalysis.indexPattern().indexPattern(), Map.of(), Map.of()))) | ||
); | ||
} else { | ||
indexResolver.resolveAsMergedMapping( | ||
indexExpressionToResolve, | ||
preAnalysis.indexPattern().indexPattern(), | ||
result.fieldNames, | ||
// Maybe if no indices are returned, retry without index mode and provide a clearer error message. | ||
switch (preAnalysis.indexMode()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,12 +42,9 @@ | |
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.regex.Matcher; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
|
@@ -986,16 +983,7 @@ public void testAlias() throws Exception { | |
Request request = esqlRequest("FROM " + index + " | KEEP emp_id | SORT emp_id | LIMIT 100"); | ||
ResponseException error = expectThrows(ResponseException.class, () -> performRequestWithRemoteSearchUser(request)); | ||
assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(400)); | ||
String expectedIndexExpressionInError = index.replace("*", "my_remote_cluster"); | ||
Pattern p = Pattern.compile("Unknown index \\[([^\\]]+)\\]"); | ||
Matcher m = p.matcher(error.getMessage()); | ||
assertTrue("Pattern matcher to parse error message did not find matching string: " + error.getMessage(), m.find()); | ||
String unknownIndexExpressionInErrorMessage = m.group(1); | ||
Set<String> actualUnknownIndexes = org.elasticsearch.common.Strings.commaDelimitedListToSet( | ||
unknownIndexExpressionInErrorMessage | ||
); | ||
Set<String> expectedUnknownIndexes = org.elasticsearch.common.Strings.commaDelimitedListToSet(expectedIndexExpressionInError); | ||
assertThat(actualUnknownIndexes, equalTo(expectedUnknownIndexes)); | ||
assertThat(error.getMessage(), containsString("Unknown index [" + index + "]")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Match original query pattern, not semi resolved pattern |
||
} | ||
|
||
for (var index : List.of( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in 3 below cases the error message index now matches the original input (notice that the order was different).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ordering stable - i.e. is it going to always match original order, no matter how many patterns and clusters we have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, error uses
indexPattern
. We are supplying original user input into it now.