[ES|QL] add validation for VALUES and MV_SORT

[drewdaemon]

Summary

Adds validation support for VALUES and MV_SORT.

Validation

Autocomplete

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

Pinging @elastic/kibana-esql (Team:ESQL)

[drewdaemon]

@stratoula I think this is ready for review

[dej611]

can we call it values to align with the CommandDefinition thing?

[drewdaemon]

Could you clarify a bit? I'm thinking options better describes what this is. "value" implies what the user actually chooses, whereas "options" says to me a set of values they are allowed to choose.

[stratoula]

Shouldn't we have here the parser and lexer changes too? 🤔

[drewdaemon]

@stratoula those were taken care of in #179850

[stratoula]

Gotcha! Thanx, I will review tomorrow!

[stratoula]

/ci

[stratoula]

It fails on a duplicate i18n keyword but it works fine so I am approving to unblock!

[dej611]

I've left few comments on code to revisit.

[drewdaemon]

They're on my radar @dej611 :)

[drewdaemon]

@elasticmachine merge upstream

[stratoula]

I like the idea of suggesting asc , desc. Looks nice. When I am clicking one of these though it is not added as a string having as a result the validator to complain.

[drewdaemon]

When I am clicking one of these though it is not added as a string having as a result the validator to complain.

Good catch... this was working but it looks like I broke it. The tests are yelling about it too :D

[stratoula]

Autocomplete works great. @dej611 can you check code wise and if you are ok let's merge

[dej611]

The structure seems better now. I would ask for a minor tweak as in code comments.

[dej611]

I'm not against to exporting for internal usage (which I think should be marked) rather for public one.
As long as they are not publicly exported I'm ok.

[dej611]

As a follow up of this PR I would probably reuse this for chrono_literals and time_literals as well

[dej611]

I would probably make this go into the omniscent isEqualType as:

// currently only supporting literalOptions with string literals
      actualArg.literalType === 'string' &&
      argDef.literalOptions &&
      !isEqualType(actualArg, argDef, references, parentCommand)

then the isEqualType signature will change as:

export function isEqualType(
  item: ESQLSingleAstItem,
  argDef: SignatureArgType | FunctionArg,
  references: ReferenceMaps,
  parentCommand?: string,
  nameHit?: string
) {
  const argType = 'innerType' in argDef && argDef.innerType ? argDef.innerType : argDef.type;
  if (argType === 'any') {
    return true;
  }
  if (item.type === 'literal') {
    return compareLiteralType(
      argType,
      item,
      'literalOptions' in argDef ? argDef.literalOptions : undefined
    );
  }
...

and the compareLiteralType function becomes:

function compareLiteralType(argTypes: string, item: ESQLLiteral, possibleOptions?: string[]) {
  if (item.literalType !== 'string') {
    return argTypes === item.literalType;
  }
  if (argTypes === 'chrono_literal') {
    return chronoLiterals.some(({ name }) => name === item.text);
  }
  if (possibleOptions) {
    return possibleOptions.includes(unwrapStringLiteralQuotes(item.text.toLowerCase()));
  }
  return argTypes === item.literalType;
}

(I think the assumption that the literalOptions are already lower case can be made).

[drewdaemon]

I started down this path but I'm not sure isEqualType is the best fit. We want a different message if the type is correct but it doesn't match one of the literalOptions. All isEqualType returns is true or false.

If I were to incorporate this check into isEqualType, I would end up reporting that the type is incorrect when, in fact, it just doesn't match one of the supported options.

[drewdaemon]

@dej611 I've addressed your comments. Can you take another look?

[drewdaemon]

@elasticmachine merge upstream

[drewdaemon]

@elasticmachine merge upstream

[dej611]

Approve to unblock.

Can you create a follow up PR with more tests over mv_sort with capitalized asc and desc args?

[drewdaemon]

Good idea @dej611 . Done in 98c6b5f

[drewdaemon]

@elasticmachine merge upstream

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 4a1e176

Failed CI Steps

• FTR Configs #29

Test Failures

• [job] [logs] FTR Configs #29 / transform /internal/transform/schedule_now_transforms bulk schedule "after each" hook for "should schedule multiple transforms by transformIds, even if one of the transformIds is invalid"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/esql-ast	66	63	-3

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
@kbn/esql-ast	2	6	+4

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
kbnUiSharedDeps-srcJs	2.9MB	2.9MB	+1.8KB

Unknown metric groups

API count

id	before	after	diff
@kbn/esql-ast	66	63	-3
@kbn/esql-validation-autocomplete	193	194	+1
total			-2

History

• 💔 Build #202580 failed 3d8ab9c
• 💚 Build #202326 succeeded 243bab5
• 💔 Build #202269 failed e17547e
• 💔 Build #202246 failed 7de22de
• 💚 Build #202130 succeeded d0d5caa

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[drewdaemon]

💛 Build succeeded, but was flaky

I'll take it!