-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add automated script to sync operators and add support for MATCH operators #205565
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/kbn-esql-validation-autocomplete/src/definitions/types.ts
# Conflicts: # src/platform/packages/shared/kbn-esql-validation-autocomplete/src/definitions/generated/operators.ts
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
/ci |
/ci |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
cc @qn895 |
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.
Thanx Quynh! I didnt test it but I have some questions to understand better the code. Also why do I see changes in the validation tests?
@@ -112,14 +112,17 @@ export function removeQuoteForSuggestedSources(suggestions: SuggestionRawDefinit | |||
})); | |||
} | |||
|
|||
const leftHandSideParamName = new Set(['left', 'field', 'lhs']); |
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.
Haven't we mapped in the script above as left? Why do we need them here?
@@ -117,56 +123,58 @@ export const isReturnType = (str: string | FunctionParameterType): str is Functi | |||
str !== 'unsupported' && | |||
(dataTypes.includes(str as SupportedDataType) || str === 'unknown' || str === 'any'); | |||
|
|||
export interface Signature { |
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.
Nice!
@@ -1060,7 +1068,7 @@ | |||
{ | |||
"query": "row 1 years + 1 year", | |||
"error": [ | |||
"Argument of [+] must be [date], found value [1 years] type [duration]" | |||
"Argument of [+] must be [date], found value [1 year] type [duration]" |
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.
So now we start the validation from the end and not from the beginning?
extraSignatures?: Signature[]; | ||
} | ||
> = { | ||
add: { |
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.
So every time a new operator is being added we need to add an entry here? Can this info be given by ES? Have we talked with them?
@@ -384,7 +384,7 @@ export const comparisonFunctions: FunctionDefinition[] = [ | |||
}, | |||
].map((op): FunctionDefinition => createComparisonDefinition(op)); | |||
|
|||
const likeFunctions: FunctionDefinition[] = [ | |||
const notLikeFunctions: FunctionDefinition[] = [ |
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 dont understand this change, the not_like is not being returned from ES?
This PR adds automated script to sync operators and add support for MATCH operators
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breaking
label should be applied in these situations.release_note:*
label is applied per the guidelinesIdentify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.