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

Escape non-alphanumeric fields for EQL #51443

Closed
rw-access opened this issue Jan 24, 2020 · 6 comments
Closed

Escape non-alphanumeric fields for EQL #51443

rw-access opened this issue Jan 24, 2020 · 6 comments
Assignees
Labels
:Analytics/EQL EQL querying

Comments

@rw-access
Copy link
Contributor

We need to add an escape sequence for fields that don't follow the typical IDENTIFIER regex: [_A-Za-z0-9][_A-Za-z0-9]*

One thought, and currently in the #49658 PR is to use back ticks ` around special fields. I believe this is how MS SQL works, and we currently check for that syntax within the SQL plugin but mark it as unsupported:

@Override
public void exitBackQuotedIdentifier(SqlBaseParser.BackQuotedIdentifierContext context) {
Token token = context.BACKQUOTED_IDENTIFIER().getSymbol();
throw new ParsingException(
"backquoted identifiers not supported; please use double quotes instead",
null,
token.getLine(),
token.getCharPositionInLine());
}

We should decide on an intuitive syntax that doesn't compete with single and double quotes.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/EQL)

@costin
Copy link
Member

costin commented Feb 5, 2020

I've added the discuss label so we can tackle it on our next meeting.

@rw-access
Copy link
Contributor Author

We decided in the 2020/03/19 weekly sync to stick with backticks. Closing.

@colings86
Copy link
Contributor

@rw-access I'm not sure you meant to close this? We should keep the issue open until we have made and merged the change/enhancement/feature.

@rw-access
Copy link
Contributor Author

rw-access commented Mar 24, 2020

@colings86 we implemented and added tests for the backticks in #49658, but afterwards weren't 100% sure if that was the desired approach. we opened this issue to discuss and track what we decided. since we decided to keep the ` escaping `, I closed it

@colings86
Copy link
Contributor

Ah ok, that makes sense. Sorry for the confusion

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

No branches or pull requests

5 participants