ESQL: change from quoting from backtick to quote #108395

costin · 2024-05-08T03:43:17Z

For historical reasons, the source declaration inside FROM command is
treated as an identifier, using backticks (``) for escaping the value.
This is inconsistent since the source is not an identifier (field name)
but an index name which has different semantics.
index means a field name index while "index" means a literal with
said value.

In case of FROM, the index name/location is more like a literal (also in
unquoted form) than an identifier (that is a reference to a value).

This PR tweaks the grammar and plugs in the quoted string logic so that
both the single quote (") and triple quote (""") are allowed.

For historical reasons, the source declaration inside FROM command is treated as an identifier, using backticks (`) for escaping the value. This is inconsistent since the source is not an identifier (field name) but an index name which has different semantics. `index` means a field name index while "index" means a literal with said value. In case of FROM, the index name/location is more like a literal (also in unquoted form) than an identifier (that is a reference to a value). This PR tweaks the grammar and plugs in the quoted string logic so that both the single quote (") and triple quote (""") are allowed.

elasticsearchmachine · 2024-05-08T03:43:40Z

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

elasticsearchmachine · 2024-05-08T03:43:41Z

Hi @costin, I've created a changelog YAML for you.

alex-spies

I didn't get to fully review the PR yet, but I noticed it is a breaking language change. We need to use our language version mechanism to avoid breaking existing queries.

alex-spies · 2024-05-08T08:32:58Z

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

        assertIdentifierAsIndexPattern("foo,test,xyz", "from foo,   test,xyz");
        assertIdentifierAsIndexPattern(
            "<logstash-{now/M{yyyy.MM}}>,<logstash-{now/d{yyyy.MM.dd|+12:00}}>",
-            "from <logstash-{now/M{yyyy.MM}}>, `<logstash-{now/d{yyyy.MM.dd|+12:00}}>`"
+            "from <logstash-{now/M{yyyy.MM}}>, \"<logstash-{now/d{yyyy.MM.dd|+12:00}}>\""
        );


I am surprised we don't need to update any csv tests - I guess we should add one or two to ensure this change also works correctly end-to-end.

And use these csv tests in the docs for FROM; currently, I think they contain incorrect syntax (missing backticks/quotes):

It'd also be great to add some tests with backticks now.

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

alex-spies · 2024-05-08T08:40:47Z

Also, we'll have to update the docs for FROM.

alex-spies

Another thing I noticed is that for

FROM `testidx`

we include the backticks in the parsed index pattern/name. I think backticks should not be allowed here.

bpintea · 2024-05-08T13:50:25Z

we include the backticks in the parsed index pattern/name. I think backticks should not be allowed here.

I think they should be allowed (maybe surprisingly).
(Also, PUT "my_index" fails with: "reason": """Invalid index name ["my_index"], must not contain the following characters ['<','*','?','>','|',',','/','\',' ','"']""")

bpintea

On one hand, this feels good since quotes aren't allowed in index names, so it feels natural to use them for quoting. OTOH, index names feel closer to identifiers than string literals and conceptually, would be easier to understand them together, as a language user.

It'd be good to have more tests with backticks now, as well as csv-specs, as noted by Alex.

It'd also be good to get it in 8.14, if we can sync this with other clients (like Kibana).

bpintea · 2024-05-08T13:54:34Z

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

        assertIdentifierAsIndexPattern("foo,test,xyz", "from foo,   test,xyz");
        assertIdentifierAsIndexPattern(
            "<logstash-{now/M{yyyy.MM}}>,<logstash-{now/d{yyyy.MM.dd|+12:00}}>",
-            "from <logstash-{now/M{yyyy.MM}}>, `<logstash-{now/d{yyyy.MM.dd|+12:00}}>`"
+            "from <logstash-{now/M{yyyy.MM}}>, \"<logstash-{now/d{yyyy.MM.dd|+12:00}}>\""
        );


It'd also be great to add some tests with backticks now.

astefan

I think it's ok to have these as strings/constants (vs. identifiers). Until now, the backticks were more like a way of emphasizing that the index names are not regular "strings", but based on your explanation, they in essence are and I agree.

alex-spies

Since quoting was disabled in this PR, this PR does not break bwc anymore, so my change request is moot :)

elasticsearchmachine · 2024-06-04T02:37:33Z

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

costin · 2024-06-04T02:37:40Z

I've added the target for this PR to be 8.15 (no backport to 8.14 necessary).
Since quoting (with backticks, in the old form) was removed in 8.14, this PR is just about adding regular quotes to indices for 8.15 and there's no backwards compatibility to worry about.

…fier

costin · 2024-06-04T23:19:03Z

@astefan @bpintea @alex-spies I've merged main and updated the PR, please take a look. /cc @drewdaemon

costin · 2024-06-04T23:19:28Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

-INDEX_UNQUOTED_IDENTIFIER
-    : INDEX_UNQUOTED_IDENTIFIER_PART+


This is not an identifier rather a string - hence the rename.

costin · 2024-06-04T23:20:44Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

-    : FROM indexIdentifier (COMMA indexIdentifier)* metadata?
+    : FROM indexString (COMMA indexString)* metadata?


I chose indexString instead of indexSource since we use source (to declare source commands).

alex-spies

LGTM, but maybe we could add a couple more tests.

I think this should be followed up by updating the docs for FROM; esp. they should use examples from csv tests (currently hard-coded, manual examples) and document + illustrate the use of quotes.
Since we also generalize the quoting for LOOKUP and METRICS, we should probably add some tests to that, too.

alex-spies · 2024-06-07T18:09:38Z

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

+            """);
+        assertStringAsIndexPattern("foo,test,xyz", "from foo,   test,xyz");
+        assertStringAsIndexPattern(
+            "<logstash-{now/M{yyyy.MM}}>,<logstash-{now/d{yyyy.MM.dd|+12:00}}>",


Suggestions for additional test cases:

Maybe let's also add a simple test case without quotes but with colons : to illustrate that CCQ is fine.

from logs-*,*:logs-*

We could also add a test case with an index name containing and/or starting with a dot ., especially without quotes.

What happens when quotes are not properly closed, or opened? We could add a small randomized test for "FROM " + leftQuote + "some-idx" + rightQuote, where the left and right quotes are a random, mismatched selection from ", """ and the empty string (unquoted).

Other pathological cases:

what if a quoted string contains an un-escaped quote in the middle? E.g. FROM """someidx"""name""" or FROM "someidx"name".

What if two quoted strings are next to each other? FROM "someidx""name"

alex-spies · 2024-06-07T18:36:51Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

+// in 8.14 ` were not allowed
+// this has been relaxed in 8.15 since " is used for quoting
+fragment UNQUOTED_SOURCE_PART
+    : ~["=|,[\]/ \t\r\n]
    | '/' ~[*/] // allow single / but not followed by another / or * which would start a comment


I noticed that this is maybe a bit too permissive. For instance, adding the following to the statement parser tests passes just fine:

assertStringAsIndexPattern("foo/=", "from foo/=");

This example also works for the other symbols that are forbidden in line 40: prepending / allows all of them except /, in particular even ".

stratoula · 2024-06-10T08:56:39Z

@costin I have a question. In 8.14 we removed the backticks support. What does it mean? That some indices were not working and with the current PR we are fixing them?

costin added >bug :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI v8.14.1 v8.15.0 labels May 8, 2024

costin requested review from astefan, bpintea, alex-spies and fang-xing-esql May 8, 2024 03:43

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

alex-spies requested changes May 8, 2024

View reviewed changes

alex-spies self-requested a review May 8, 2024 08:40

alex-spies reviewed May 8, 2024

View reviewed changes

alex-spies self-requested a review May 8, 2024 10:59

bpintea approved these changes May 8, 2024

View reviewed changes

astefan approved these changes May 8, 2024

View reviewed changes

astefan mentioned this pull request May 9, 2024

[ES|QL] Support Named and Positional Parameters in EsqlQueryRequest #108421

Open

alex-spies approved these changes May 10, 2024

View reviewed changes

costin removed the v8.14.1 label Jun 4, 2024

costin added 2 commits June 4, 2024 09:25

Merge branch 'main' into esql/from-source-as-string-instead-of-identi…

1bcb7dd

…fier

Update grammar

9d97583

costin force-pushed the esql/from-source-as-string-instead-of-identifier branch from c3dd636 to 9d97583 Compare June 4, 2024 21:37

Add more tests

27a94d9

Add a few more tests

2aa9606

costin requested review from astefan, alex-spies and bpintea June 4, 2024 23:18

costin commented Jun 4, 2024

View reviewed changes

Add extra test

35f2328

alex-spies approved these changes Jun 7, 2024

View reviewed changes

stratoula mentioned this pull request Jun 10, 2024

[ES|QL] We should not escape indices with backticks elastic/kibana#185701

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: change from quoting from backtick to quote #108395

ESQL: change from quoting from backtick to quote #108395

costin commented May 8, 2024 •

edited by astefan

elasticsearchmachine commented May 8, 2024

elasticsearchmachine commented May 8, 2024

alex-spies left a comment

alex-spies May 8, 2024

alex-spies May 8, 2024

bpintea May 8, 2024

alex-spies commented May 8, 2024

alex-spies left a comment

bpintea commented May 8, 2024

bpintea left a comment

bpintea May 8, 2024

astefan left a comment

alex-spies left a comment

elasticsearchmachine commented Jun 4, 2024

costin commented Jun 4, 2024 •

edited

costin commented Jun 4, 2024

costin Jun 4, 2024

costin Jun 4, 2024

alex-spies left a comment •

edited

alex-spies Jun 7, 2024

alex-spies Jun 7, 2024

stratoula commented Jun 10, 2024

		: FROM indexIdentifier (COMMA indexIdentifier)* metadata?
		: FROM indexString (COMMA indexString)* metadata?

		INDEX_UNQUOTED_IDENTIFIER
		: INDEX_UNQUOTED_IDENTIFIER_PART+

ESQL: change from quoting from backtick to quote #108395

Are you sure you want to change the base?

ESQL: change from quoting from backtick to quote #108395

Conversation

costin commented May 8, 2024 • edited by astefan

elasticsearchmachine commented May 8, 2024

elasticsearchmachine commented May 8, 2024

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies commented May 8, 2024

alex-spies left a comment

Choose a reason for hiding this comment

bpintea commented May 8, 2024

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jun 4, 2024

costin commented Jun 4, 2024 • edited

costin commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratoula commented Jun 10, 2024

costin commented May 8, 2024 •

edited by astefan

costin commented Jun 4, 2024 •

edited

alex-spies left a comment •

edited