[ES|QL] Skip validating remote cluster index names in parser #114271

fang-xing-esql · 2024-10-08T03:15:07Z

Skip the validation of index names in parser if it is from a remote cluster.

Reduce the scope of #112081 to local indices only.

It is mainly because the behavior of invalid indices names on remote cluster is inconsistent with local cluster, for example these commands below pass even though the index name is invalid or remote cluster does not exists.

FROM *:invalid#index
FROM *fake:invalid#index

It is better to defer the validation of remote cluster indices names to later phase or until the pattern is clear.

…clusion unless the DateMath expression is invalid

elasticsearchmachine · 2024-10-08T03:15:31Z

Hi @fang-xing-esql, I've created a changelog YAML for you.

elasticsearchmachine · 2024-10-08T19:19:22Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-10-08T19:19:22Z

Pinging @elastic/kibana-esql (ES|QL-ui)

bpintea · 2024-10-10T16:17:17Z

Resolves: #114201

Not sure if that needs resolving: the grammar allows FROM with unquoted_index | quoted_index | unquoted_remote_cluster:(unquoted_index | quoted_index). So if smth is quoted only, that has to be an index.

bpintea · 2024-10-10T16:21:13Z

It is better to defer the validation of remote cluster indices names to later phase

But will it not eventually fail anyways? In my understanding the point was to not allow later phases to execute, if we know it's for nothing.
However, if the failure is drastically different and we want to keep consistency, that could be a reason to skip the validation, but is that the case?

astefan · 2024-10-10T20:27:22Z

It is better to defer the validation of remote cluster indices names to later phase

But will it not eventually fail anyways? In my understanding the point was to not allow later phases to execute, if we know it's for nothing. However, if the failure is drastically different and we want to keep consistency, that could be a reason to skip the validation, but is that the case?

The story is more complex than this :-). I am in favor of skipping the validation of anything related to CCS. See this discussion for more context. Depending on skip_available setting we might or might not fail a query to a remote cluster.

astefan

LGTM

bpintea

Lgtm

bpintea · 2024-10-11T12:32:48Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

-        for (String command : List.of("FROM", "METRICS")) {
+        List<String> commands = new ArrayList<>();
+        commands.add("FROM");
+        if (Build.current().isSnapshot()) {


Are the tests run in non-snapshot code? Can stay as is, was just wondering.

It doesn't have the assumeTrue for snapshot, just be to cautious.

bpintea · 2024-10-11T12:32:53Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/IdentifierBuilder.java

@@ -71,6 +75,9 @@ private static void validateIndexPattern(String indexPattern, EsqlBaseParser.Ind
        String[] indices = indexPattern.split(",");
        boolean hasExclusion = false;
        for (String index : indices) {
+            if (isRemoteIndexName(index)) { // skip the validation if there is remote cluster


This check is a bit baffling unless you dig in the grammar to understand that when quoting an index pattern, this can actually contain a remote index pattern, which is accepted downstream. I guess a comment (somewhere) would be nice to the next reader, but can also be merged as is without it.

costin

LGTM - please backport this to 8.16 also.

elasticsearchmachine · 2024-10-16T19:46:35Z

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 114271

…#114271) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

fang-xing-esql · 2024-10-16T21:04:59Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	8.16

Questions ?

Please refer to the Backport tool documentation

…#114271) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

#114960) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

#114959) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

…#114271) * skip validating remote cluster index names in parser

fang-xing-esql and others added 24 commits August 21, 2024 23:22

validate index name in parser

240a4d4

validate index string in parser

f4e2fac

Merge branch 'main' into validate-index-name

d122c23

validate index name in parser

9845f28

fix bug

316f204

Merge branch 'main' into validate-index-name

f675063

Merge branch 'main' into validate-index-name

cb96e18

add more tests

909edbf

Update docs/changelog/112081.yaml

0d4716f

Merge branch 'main' into validate-index-name

9f0ec40

update according to review comments

8cfd07d

more tests for index names with date math

71fce7a

Merge branch 'main' into validate-index-name

0dfee87

Merge branch 'main' into validate-index-name

1717c7e

modify tests according to review comments

afb62e3

Merge branch 'main' into validate-index-name

578ee34

Merge branch 'main' into validate-index-name

31577ef

allow invalid index names if there is index name with wildcard and ex…

9465693

…clusion unless the DateMath expression is invalid

Merge branch 'main' into validate-index-name

9ecf17c

Merge branch 'main' into validate-index-name

a868ab8

update according to review comments

cc0acdf

Merge branch 'main' into validate-index-name

7dc33fa

Merge branch 'main' into validate-index-name

24671c2

skip validating remote cluster index names in parser

0d1fa8c

fang-xing-esql added >bug :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI v8.16.0 v9.0.0 labels Oct 8, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 8, 2024

fang-xing-esql requested review from costin, astefan and bpintea October 8, 2024 19:37

astefan approved these changes Oct 10, 2024

View reviewed changes

bpintea approved these changes Oct 11, 2024

View reviewed changes

brianseeders added v8.17.0 and removed v8.16.0 labels Oct 16, 2024

costin approved these changes Oct 16, 2024

View reviewed changes

fang-xing-esql added v8.16.0 auto-backport Automatically create backport pull requests when merged labels Oct 16, 2024

fang-xing-esql merged commit 64ae0ae into elastic:main Oct 16, 2024
16 checks passed

elasticsearchmachine added the backport pending label Oct 16, 2024

fang-xing-esql mentioned this pull request Oct 16, 2024

[8.x] [ES|QL] Skip validating remote cluster index names in parser (#114271) #114959

Merged

fang-xing-esql mentioned this pull request Oct 16, 2024

[8.16] [ES|QL] Skip validating remote cluster index names in parser (#114271) #114960

Merged

elasticsearchmachine pushed a commit that referenced this pull request Oct 16, 2024

[ES|QL] Skip validating remote cluster index names in parser (#114271) (

f4b9464

#114960) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

elasticsearchmachine pushed a commit that referenced this pull request Oct 16, 2024

[ES|QL] Skip validating remote cluster index names in parser (#114271) (

c77b415

#114959) * skip validating remote cluster index names in parser (cherry picked from commit 64ae0ae)

fang-xing-esql removed the backport pending label Oct 16, 2024

georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024

[ES|QL] Skip validating remote cluster index names in parser (elastic…

1dcaf88

…#114271) * skip validating remote cluster index names in parser

fang-xing-esql deleted the validate-index-name branch October 30, 2024 02:51

jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024

[ES|QL] Skip validating remote cluster index names in parser (elastic…

eeedf76

…#114271) * skip validating remote cluster index names in parser

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Skip validating remote cluster index names in parser #114271

[ES|QL] Skip validating remote cluster index names in parser #114271

fang-xing-esql commented Oct 8, 2024 •

edited

Loading

elasticsearchmachine commented Oct 8, 2024

elasticsearchmachine commented Oct 8, 2024

elasticsearchmachine commented Oct 8, 2024

bpintea commented Oct 10, 2024

bpintea commented Oct 10, 2024

astefan commented Oct 10, 2024

astefan left a comment

bpintea left a comment

bpintea Oct 11, 2024

fang-xing-esql Oct 16, 2024

bpintea Oct 11, 2024

costin left a comment

elasticsearchmachine commented Oct 16, 2024

fang-xing-esql commented Oct 16, 2024

[ES|QL] Skip validating remote cluster index names in parser #114271

[ES|QL] Skip validating remote cluster index names in parser #114271

Conversation

fang-xing-esql commented Oct 8, 2024 • edited Loading

elasticsearchmachine commented Oct 8, 2024

elasticsearchmachine commented Oct 8, 2024

elasticsearchmachine commented Oct 8, 2024

bpintea commented Oct 10, 2024

bpintea commented Oct 10, 2024

astefan commented Oct 10, 2024

astefan left a comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

bpintea Oct 11, 2024

Choose a reason for hiding this comment

fang-xing-esql Oct 16, 2024

Choose a reason for hiding this comment

bpintea Oct 11, 2024

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 16, 2024

💔 Backport failed

fang-xing-esql commented Oct 16, 2024

💚 All backports created successfully

Questions ?

fang-xing-esql commented Oct 8, 2024 •

edited

Loading