-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ES|QL] Function AST node subtypes #189268
Conversation
Pinging @elastic/kibana-esql (Team:ESQL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. Just a couple questions
|
||
export type BinaryExpressionArithmeticOperator = '+' | '-' | '*' | '/' | '%'; | ||
export type BinaryExpressionAssignmentOperator = '='; | ||
export type BinaryExpressionComparisonOperator = '==' | '=~' | '!=' | '<' | '<=' | '>' | '>='; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think =~
is a valid operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove it, or leave it, indifferent.
export type BinaryExpressionArithmeticOperator = '+' | '-' | '*' | '/' | '%'; | ||
export type BinaryExpressionAssignmentOperator = '='; | ||
export type BinaryExpressionComparisonOperator = '==' | '=~' | '!=' | '<' | '<=' | '>' | '>='; | ||
export type BinaryExpressionRegexOperator = 'like' | 'not_like' | 'rlike' | 'not_rlike'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the type checker catch it if the grammar changes in such a way that these lists are out-of-sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not. But it is a good point: we have the createFunction
helper, it currently allows any function, I could create createBinaryExpression
, which would help in some cases with checking types.
However, the main reason I added these types was because of the (not_)(r)like
regex expressions. Unlike the other operators, I cannot just print them when pretty-printing, so I wanted those expression captured in the types, so TypeScript would give me the correct autocomplete. When I was adding them, I decided to capture all binary operators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @vadimkibana ! From the Detection Engine team here 👋🏽 Are there any changes we need to make to make sure existing ES|QL rules continue to function as intended? |
💚 Build Succeeded
Metrics [docs]Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
@yctercero This change adds a new |
Summary
Closes #189259
subtype
property for function AST node types. This allows to discriminate between real functions an various expression types.Checklist
Delete any items that are not applicable to this PR.
For maintainers