Skip to content
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

EQL: Replace [`] with ['] for identifiers #62638

Closed
wants to merge 1 commit into from

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Sep 18, 2020

Since ' is no longer supported for defining string literals,
we replace the backquote [`] with single quote ['] for the definition of
identifiers. The backquote is not a commonly used symbol in languages
and many times, especially with some fonts, can easily be mistaken and
cause confusion.

Closes: #62539

Since `'` is no longer supported for defining string literals,
we replace the backquote [`] with single quote ['] for the definition of
identifiers. The backquote is not a commonly used symbol in languages
and many times, especially with some fonts, can easily be mistaken and
cause confusion.

Relates to elastic#61659
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/EQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 18, 2020
@matriv
Copy link
Contributor Author

matriv commented Sep 18, 2020

The error message for the removal of ' around string literals could live for one release before this change, but since EQL is currently experimental it could be done immediately.

/cc @rw-access
/cc @jrodewig

@@ -191,14 +191,12 @@ PIPE: '|';


ESCAPED_IDENTIFIER
: '`' (~'`')* '`'
: '\'' (~'`')* '\''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't agreed on this.

#62539 (review)

Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't merge until we reach an agreement. Related note:in this PR as well: #62539 (review).

Also EQL in Elasticsearch is experimental, but EQL for Endgame is not. For EQL in Endgame users migrating to Elasticsearch, this is would make this not only a breaking change, but a quiet one, since existing syntax is assigned new purpose.

@matriv
Copy link
Contributor Author

matriv commented Sep 18, 2020

@rw-access I agree with you, since there is no way (grammar-wise) to keep both the error message for the single quotes around strings and their new usage around identifiers. Personally, I'd keep the next release with the prohibition of single quotes and introduce the new usage in the release after that.

@costin costin mentioned this pull request Sep 18, 2020
7 tasks
@matriv
Copy link
Contributor Author

matriv commented Sep 18, 2020

Let me add some more thoughts.

  • The single quote ' is currently used rarely to identify strings
  • If a user issues a query with ' around a string literal, e.g.: process where a = 'foo' then the foo will be parsed as a identifier and:
    • if foo is a valid identifier, an exception will be thrown since we cannot have comparisons with identifiers both to the left and to the right.
    • if foo is an invalid identifier, an exception will be thrown stating this.

This doesn't seem as a dangerous behaviour since it doesn't lead to a correct query with wrong behaviour.

On the other hand, for me it seems quite inconvenient for the user to receive a message mentioning a wrong identifier for something that until now it was considered a string literal.

To conclude, if we think that the percentage of users/queries affected is rather low, and since ES-EQL is experimental, doesn't seem so impactful. Imho though, I have a preference over the more clear 2 step approach (1. error message for single quotes around string, 2. replacement of backquotes with single quote for identifiers).

@matriv
Copy link
Contributor Author

matriv commented Sep 21, 2020

Based on @astefan 's comment here: #62650 (comment)
I'd like to conclude that if we lock ourselves in the situation that we have to wait replacing the backquote with the single quote for identifiers until next major release, I'd rather proceed with this breaking change now, since it's safe enough (no wrong results for deprecated syntax) and end up with a more clear grammar.

@costin
Copy link
Member

costin commented Sep 28, 2020

This PR can be closed without merging (see #62663 (comment)).
The issue of quoting an identifier containing the quoting character needs to be verified and addressed.

@matriv
Copy link
Contributor Author

matriv commented Sep 28, 2020

Closing this, see comments above and on the relevant issue: #62539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants