Skip to content

Commit

Permalink
[8.14] [ES|QL] implicit casting changes (#182989) (#183011)
Browse files Browse the repository at this point in the history 
# Backport

This will backport the following commits from `main` to `8.14`:
- [[ES|QL] implicit casting changes
(#182989)](#182989)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-05-09T05:51:18Z","message":"[ES|QL]
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
https://github.com/elastic/kibana/issues/177699\r\n\r\n### A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated
or","sha":"c3e1027b2d5da9361251e3af3304098717193ded","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","backport:prev-minor","Feature:ES|QL","v8.14.0","Team:ESQL","v8.15.0"],"title":"[ES|QL]
implicit casting
changes","number":182989,"url":"https://github.com/elastic/kibana/pull/182989","mergeCommit":{"message":"[ES|QL]
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
https://github.com/elastic/kibana/issues/177699\r\n\r\n### A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated
or","sha":"c3e1027b2d5da9361251e3af3304098717193ded"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/182989","number":182989,"mergeCommit":{"message":"[ES|QL]
implicit casting changes (#182989)\n\n## Summary\r\n\r\nThis is a
follow-on
from\r\nhttps://github.com/elastic/elasticsearch/pull/107859.\r\n\r\nTwo
main changes to our client-side validation code.\r\n1. comparison
operators like `==` and `>=` now support implicit casting\r\nfrom
strings for `version`, `ip`, and `boolean` types\r\n\r\n2. `in` and `not
in` operators support implicit casting from strings for\r\n`version`,
`ip`, `boolean`, and `date` constants. To address this\r\nquickly, I
have disabled type-checking for array values (e.g. `in
(\r\nversion_field, \"1.2.3\", \"2.3.1\" )`) because our parameter
typing system\r\ndoes not currently support arrays of mixed types which
it will need to\r\nin order to re-enable checking while allowing string
literals. I have\r\nadded this to
https://github.com/elastic/kibana/issues/177699\r\n\r\n### A note on
testing\r\n\r\nThese implicit casting changes may not be on the latest
Elasticsearch\r\nsnapshot. Instead, check out the `8.14` branch in a
local Elasticsearch\r\nrepo and run `yarn es source
--source-path='path/to/elasticsearch'`\r\n\r\nSee
[these\r\ntests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)\r\nfor
a set of good use cases to try. `to_<type>` functions can be used
if\r\nthe sample data doesn't contain a field of the type you want to
test.\r\n\r\n### A note on string to date casting\r\n\r\nIn
#182856 we started
accepting\r\nstring literals for any function arguments that are
dates.\r\n\r\nBy the same logic, `\"2022\" - 1 day` would work, so our
validator doesn't\r\ncomplain about it. However, it currently fails at
the Elasticsearch\r\nlevel.\r\n\r\nIn a discussion with Fang, we
determined that this is a\r\nvalid use case, so I have
created\r\nhttps://github.com/elastic/elasticsearch/issues/108432 and
determined\r\nnot to tighten the client-side validation since this seems
more like a\r\ncasting \"hole\" that will soon be filled in on the ES
side (though not in\r\n8.14).\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x]
[Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or","sha":"c3e1027b2d5da9361251e3af3304098717193ded"}}]}]
BACKPORT-->

Co-authored-by: Drew Tate <[email protected]>
  • Loading branch information
@kibanamachine @drewdaemon
kibanamachine and drewdaemon authored May 9, 2024
1 parent b032740 commit 6a1a267
Show file tree
Hide file tree
Showing 4 changed files with 2,638 additions and 1,071 deletions.
79 changes: 74 additions & 5 deletions packages/kbn-esql-validation-autocomplete/src/definitions/builtin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,22 @@ function createComparisonDefinition(
],
returnType: 'boolean',
},
...['date', 'number'].flatMap((type) => [
{
params: [
{ name: 'left', type: 'version' },
{ name: 'right', type: 'version' },
],
returnType: 'boolean',
},
// constant strings okay because of implicit casting for
// string to version and ip
//
// boolean casting is handled on the specific comparison function
// that support booleans
//
// date casting is handled in the validation routine since it's a
// general rule. Look in compareLiteralType()
...['ip', 'version'].flatMap((type) => [
{
params: [
{ name: 'left', type },
Expand Down Expand Up @@ -214,6 +229,21 @@ export const builtinFunctions: FunctionDefinition[] = [
],
returnType: 'boolean',
},
// constant strings okay because of implicit casting
{
params: [
{ name: 'left', type: 'boolean' },
{ name: 'right', type: 'string', constantOnly: true },
],
returnType: 'boolean',
},
{
params: [
{ name: 'right', type: 'string', constantOnly: true },
{ name: 'right', type: 'boolean' },
],
returnType: 'boolean',
},
],
},
{
Expand All @@ -232,6 +262,21 @@ export const builtinFunctions: FunctionDefinition[] = [
],
returnType: 'boolean',
},
// constant strings okay because of implicit casting
{
params: [
{ name: 'left', type: 'boolean' },
{ name: 'right', type: 'string', constantOnly: true },
],
returnType: 'boolean',
},
{
params: [
{ name: 'right', type: 'string', constantOnly: true },
{ name: 'right', type: 'boolean' },
],
returnType: 'boolean',
},
],
},
{
Expand Down Expand Up @@ -318,6 +363,15 @@ export const builtinFunctions: FunctionDefinition[] = [
},
{ name: 'not_in', description: '' },
].map<FunctionDefinition>(({ name, description }) => ({
// set all arrays to type "any" for now
// this only applies to the "in" operator
// e.g. "foo" in ( "foo", "bar" )
//
// we did this because the "in" operator now supports
// mixed-type arrays like ( "1.2.3", versionVar )
// because of implicit casting.
//
// we need to revisit with more robust validation
type: 'builtin',
ignoreAsSuggestion: /not/.test(name),
name,
Expand All @@ -327,28 +381,43 @@ export const builtinFunctions: FunctionDefinition[] = [
{
params: [
{ name: 'left', type: 'number' },
{ name: 'right', type: 'number[]' },

{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
{
params: [
{ name: 'left', type: 'string' },
{ name: 'right', type: 'string[]' },
{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
{
params: [
{ name: 'left', type: 'boolean' },
{ name: 'right', type: 'boolean[]' },
{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
{
params: [
{ name: 'left', type: 'date' },
{ name: 'right', type: 'date[]' },
{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
{
params: [
{ name: 'left', type: 'version' },
{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
{
params: [
{ name: 'left', type: 'ip' },
{ name: 'right', type: 'any[]' },
],
returnType: 'boolean',
},
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-esql-validation-autocomplete/src/definitions/functions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ export const evalFunctionsDefinitions: FunctionDefinition[] = [
signatures: [
{
params: [{ name: 'field', type: 'string' }],
returnType: 'string',
returnType: 'version',
examples: [`from index | EVAL version = to_version(stringField)`],
},
],
Expand Down
Loading

0 comments on commit 6a1a267

Please sign in to comment.