Skip to content

Commit

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

This is a follow-on from
elastic/elasticsearch#107859.

Two main changes to our client-side validation code.
1. comparison operators like `==` and `>=` now support implicit casting
from strings for `version`, `ip`, and `boolean` types

2. `in` and `not in` operators support implicit casting from strings for
`version`, `ip`, `boolean`, and `date` constants. To address this
quickly, I have disabled type-checking for array values (e.g. `in (
version_field, "1.2.3", "2.3.1" )`) because our parameter typing system
does not currently support arrays of mixed types which it will need to
in order to re-enable checking while allowing string literals. I have
added this to #177699

### A note on testing

These implicit casting changes may not be on the latest Elasticsearch
snapshot. Instead, check out the `8.14` branch in a local Elasticsearch
repo and run `yarn es source --source-path='path/to/elasticsearch'`

See [these
tests](https://github.com/elastic/kibana/pull/182989/files#diff-89c4af0faedcf80d51cfb19ae397a5898c7293055b19284a94cb3f6a7cd4d071R1727-R1763)
for a set of good use cases to try. `to_<type>` functions can be used if
the sample data doesn't contain a field of the type you want to test.

### A note on string to date casting

In #182856 we started accepting
string literals for any function arguments that are dates.

By the same logic, `"2022" - 1 day` would work, so our validator doesn't
complain about it. However, it currently fails at the Elasticsearch
level.

In a discussion with Fang, we determined that this is a
valid use case, so I have created
elastic/elasticsearch#108432 and determined
not to tighten the client-side validation since this seems more like a
casting "hole" that will soon be filled in on the ES side (though not in
8.14).



### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or
  • Loading branch information
drewdaemon committed May 9, 2024
1 parent ccf0f97 commit c3e1027
Show file tree
Hide file tree
Showing 4 changed files with 2,638 additions and 1,071 deletions.
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
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

0 comments on commit c3e1027

Please sign in to comment.