[ES|QL] Enable CCS tests for subqueries#137776
Conversation
|
Hi @fang-xing-esql, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
smalyshev
left a comment
There was a problem hiding this comment.
I reviewed part of it, will finish up tomorrow.
| for (int i = 0; i <= input.length() - delimiterLength; i++) { | ||
| char c = input.charAt(i); | ||
|
|
||
| if (c == '(') { |
There was a problem hiding this comment.
I hope we don't have (s inside strings anywhere, otherwise this I think would break.
There was a problem hiding this comment.
We don't have fancy queries like this in CsvTests yet, and we don't check for tokens like from, metadata , |, ), '(' either. This is a very lightweight parsing utility targeting to existing subqueries in CsvTests. If there are more fancy subqueries added to CsvTests in the future, it needs to be enhanced.
| /** | ||
| * Convert index patterns and subqueries in FROM commands to use remote indices. | ||
| */ | ||
| private static String convertSubqueryToRemoteIndices(String testQuery) { |
There was a problem hiding this comment.
What about SET commands that can precede FROM? Are those supported?
There was a problem hiding this comment.
SET is not tested, and specially characters that could be a token used by query parser is not tested here either. We are not implementing a full query parser here. Just to make sure all the queries in CsvTests with the subquery capability works fine in this PR. If more fancy queries are added into CsvTests, we will need to enhance it.
| for (String indexPatternOrSubquery : indexPatternsAndSubqueries) { | ||
| // remove the from keyword if it's there | ||
| indexPatternOrSubquery = indexPatternOrSubquery.strip(); | ||
| if (indexPatternOrSubquery.toLowerCase(Locale.ROOT).startsWith("from ")) { |
There was a problem hiding this comment.
This seems to imply every element of indexPatternsAndSubqueries can start with FROM, but there could be only one FROM and then comma-separated list of expressions... Not sure if it's important for particular queries in the tests, but seems incorrect.
There was a problem hiding this comment.
Each indexPatternOrSubquery could be one of the following:
from+ index patterns:from index1- index patterns:
index2 - subqueries:
(from index3 | where a > 0), subqueries will be processed recursively.
There was a problem hiding this comment.
Wait, ate you saying you can do FROM index1, FROM index2, FROM index3?
There was a problem hiding this comment.
Wait, ate you saying you can do
FROM index1, FROM index2, FROM index3?
I don't think this is a valid CsvTests query, this method might be able to deal with it, however it will not see this pattern in queries from CsvTests...
...ulti-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
Outdated
Show resolved
Hide resolved
...ulti-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
Outdated
Show resolved
Hide resolved
| // take care of removing the subquery during analysis. | ||
| // If all subqueries have invalid index resolution, we should fail in Analyzer's verifier. | ||
| if (r.indexResolution.isEmpty() == false // it is not a row | ||
| && r.indexResolution.size() > 1 // there is a subquery |
There was a problem hiding this comment.
Can subquery be the only clause in FROM? I understand it's kinda weird to do it, but is it possible?
There was a problem hiding this comment.
If a subquery is the only clause in a FROM, it will be merged into the main query by parser, there is a test here.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
Outdated
Show resolved
Hide resolved
| String clusterAlias = entry.getKey(); | ||
| String indexExpression = entry.getValue(); | ||
| EsqlExecutionInfo.Cluster cluster = executionInfo.getCluster(clusterAlias); | ||
| if (indexExpression.equals(cluster.getIndexExpression())) { |
There was a problem hiding this comment.
Not sure I understand this check - if it's the entry under this cluster's alias, why we need to check again?
There was a problem hiding this comment.
clustersWithInvalidIndexResolutions is a map, its key is cluster alias and value is all of the index patterns that are marked as invalid, because they are missing on that cluster.
If all of the indices are missing on a cluster, the cluster is marked as SKIPPED here in EsqlExwecutionInfo. If some of the indices are available, and some are missing, the cluster is not marked as SKIPPED. We do this check here mainly for the situations that a remote cluster is referenced by multiple subqueries.
| required_capability: subquery_in_from_command | ||
|
|
||
| FROM employees, (FROM sample_data | EVAL x = client_ip::keyword ) metadata _index | ||
| FROM employees, (FROM sample_data metadata _index | EVAL x = client_ip::keyword ) metadata _index |
There was a problem hiding this comment.
What about this check:
assumeFalse("can't test with _index metadata", (remoteMetadata == false) && hasIndexMetadata(testCase.query));
wouldn't it interfere with this test?
There was a problem hiding this comment.
It does. I'd like to keep some metadata tests for subqueries here. So I removed metadata option from a few queries in this file and hope to cover more subquery tests in MultiClusterSpecIT.
| @@ -3,7 +3,6 @@ | |||
| // | |||
|
|
|||
| subqueryInFrom | |||
There was a problem hiding this comment.
Question for my education - if we have two sets of metadata fields, like this: FROM index1, (FROM index2 METADATA _index, _id) METADATA _index, _version - what is the resulting field set here? Is it a union, an intersection or something else?
There was a problem hiding this comment.
It is an UnionAll of the results of all subqueries, if there are duplicated records among the subqueries, the duplicates are not removed.
There was a problem hiding this comment.
I am not asking about rows, but about column names - if there's _index in subquery and in the main expression, how does it work? Does main subquery _index only fills the column for actual indices, not subqueries?
There was a problem hiding this comment.
The test has metadata for both the main and subquery, both indices referenced by main and subquery return.
...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterSubqueryIT.java
Show resolved
Hide resolved
...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterSubqueryIT.java
Outdated
Show resolved
Hide resolved
...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterSubqueryIT.java
Show resolved
Hide resolved
...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterSubqueryIT.java
Show resolved
Hide resolved
...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterSubqueryIT.java
Outdated
Show resolved
Hide resolved
| ) { | ||
| // check if all clusters involved have skip_unavailable = true | ||
| boolean allClustersSkipUnavailable = true; | ||
| var groupedIndices = indicesGrouper.groupIndices( |
There was a problem hiding this comment.
We could no longer rely on indicesGrouper as it is not going to be available for CPS queries.
There was a problem hiding this comment.
I am going to open a pr with alternative for it this week. I will keep you updated.
There was a problem hiding this comment.
I have merged #138396
You should be able to access grouped original or concrete indices from EsRelation or EsIndex now.
Please let me know if you need any help with it.
There was a problem hiding this comment.
The reference to IndicesExpressionGrouper when identifying empty subquery is removed.
| } | ||
| // Check if a subquery need to be pruned. If some but not all the subqueries has invalid index resolution, | ||
| // and all the clusters referenced by the subquery that has invalid index resolution have skipUnavailable=true, | ||
| // try to prune it by setting IndexResolution to EMPTY_SUBQUERY. Analyzer.PruneEmptyUnionAllBranch will |
There was a problem hiding this comment.
I believe this should also add a warning with remote and failure.
There was a problem hiding this comment.
I believe this should also add a warning with remote and failure.
The empty subqueries with no valid index pattern found after field caps is logged in debug log in this PR. EsqlExecutionInfo seems to be a good place to add some details(for both planning and execution) at subquery level. It is added as a subtask here.
idegtiarenko
left a comment
There was a problem hiding this comment.
We should not introduce more usages of indicesGrouper since it is not going to be available in CPS.
|
Thank you so much for reviewing @smalyshev @idegtiarenko ! Stas's comments have been addressed(let me know if I missed anything), and I'll wait for Ievgen's PR and then adjust the usage of |
| // miss the results from a branch and return wrong results. Both RUNNING and SUCCESSFUL mean we should continue | ||
| // processing the next compute on remote cluster in the queue. If it is partial, it is fine to skip the subsequent | ||
| // branches as something went wrong already. | ||
| if (clusterStatus != EsqlExecutionInfo.Cluster.Status.RUNNING |
There was a problem hiding this comment.
@smalyshev I'd like to discuss a bit more with you about the change here.
There was a problem hiding this comment.
This change would redefine how statuses work, I am not sure that's the right thing to do. SUCCESSFUL means "we are done with this cluster and all went well", PARTIAL means "we are done with this cluster and we had to skip some data". I don't think we should put cluster into SUCCESSFUL state before the main query is finished. Maybe we need more statuses here?
There was a problem hiding this comment.
Also, I am not sure this:
If it is partial, it is fine to skip the subsequent branches as something went wrong already.
is correct. If one subquery partially failed, but we did not set the cluster to SKIPPED, I am not convinced it's ok to skip the other subqueries. In fact, we may have to revisit when we set clusters to SKIPPED - e.g. if there's a missing index in one of the subqueries, and we have partial results on, it shouldn't skip this cluster in all other subqueries I think. Also, if one index is corrupt, it doesn't mean other subqueries will fail. Again, we may need to consider maybe we need more fine-grained checks here now that we have subqueries.
There was a problem hiding this comment.
Discussed with @smalyshev offline, today the way that the cluster status is updated in EsqlExecutionInfo does not quite work with the branch model of fork/subquery. The SUCCESSFUL status in EsqlExecutionInfo was set at branch level(the same for the metrics saved in EsqlExecutionInfo), not at the whole query level, so we might lose data from branches if there is overlap on the remote cluster between branches.
This needs more discussion, and should be addressed in a separate PR. We will need to find a better way to set cluster status and metrics in EsqlExecutionInfo. I'm going to remove this change from this PR, and make this PR add stable tests for subqueries with CCS only(avoid overlap remote cluster between subqueries(branches) for now).
There was a problem hiding this comment.
This is the PR that ensures subquery/fork return deterministic results.
@idegtiarenko The usage of |
| .anyMatch(ir -> ir.isValid() && ir.get().originalIndices().get(clusterAlias) != null); | ||
| if (clusterHasValidIndex == false) { | ||
| String errorMsg = "no valid indices found in any subquery " + EsqlCCSUtils.inClusterName(clusterAlias); | ||
| LOGGER.debug(errorMsg); |
There was a problem hiding this comment.
Do we need this log statement here? If so, could we convert it to logger formatting instead of unconditional string concatenation?
There was a problem hiding this comment.
Done, this message is attached to the failure/warning message to skipped cluster as well.
| String errorMsg = "no valid indices found in any subquery {}"; | ||
| LOGGER.debug(errorMsg, EsqlCCSUtils.inClusterName(clusterAlias)); |
There was a problem hiding this comment.
| String errorMsg = "no valid indices found in any subquery {}"; | |
| LOGGER.debug(errorMsg, EsqlCCSUtils.inClusterName(clusterAlias)); | |
| LOGGER.debug("no valid indices found in any subquery {}", EsqlCCSUtils.inClusterName(clusterAlias)); |
idegtiarenko
left a comment
There was a problem hiding this comment.
Looks good from index resolution point of view 👍
|
Thank you for reviewing @idegtiarenko @smalyshev ! |
This feature is behind snapshot, and not formally released yet.
This is to support #136035, and it is also to support https://github.com/elastic/esql-planning/issues/88
This PR addresses the following items:
Enabled CCS tests with subqueries in the following tests:
CrossClusterSubqueryITCrossClusterSubqueryUnavailableRemotesITMultiClusterSpecIT, convert queries with subqueries to use remote index patterns, by recognizing multiplefromcommands in the querysubquery.csv-specto allow more queries to run withMultiClusterSpecIT, removed theforktags, and removed metadata from some of the queries.Prune subqueries that do not have a valid index pattern found for them, so that the query can continue as far as there is valid
IndexResolutionfor a subset of the subqueries.EsqlSessionto recognize subquery index patterns that do not have a validIndexResolutionfound, and mark them asEMPTY_SUBQUERYPruneEmptyUnionAllBranchis added inAnalyzerto prune empty subqueries.Item that will be addressed in the next PRs:
CrossClusterSubqueryIT, and it will be addressed in a separate PR.EsqlExecutionInfomay not reflect the situation offorkorsubquerycorrectly, it will be addressed in a separate PR, as we need to clarify the execution time behaviors when something went wrong with onesubquery, how do we process the othersubqueries.** Prerequisite**:
This PR ensures consistent results for
subqueryandforkwith CCS.