ESQL: Multiple patterns for grok command

[flash1293]

Closes #132486

This PR adds the ability to specify multiple grok patterns as part of a single grok command. Consistent with the grok processor for ingest pipelines, they are tried in order, the first matching one is actually applied:

POST _query
{
  "query": """
    ROW col1="123 This is a test" | GROK col1 "%{UUID:def}", "%{WORD:xxx}"
  """
}

returns

       col1       |      def      |      xxx
------------------+---------------+---------------
123 This is a test|null           |123

It's not allowed to have different types for the same semantic names in different patterns:

POST _query
{
  "query": """
    ROW col1="123 This is a test" | GROK col1 "%{UUID:def}", "%{INT:def}"
  """
}

returns

{"error":{"root_cause":[{"type":"parsing_exception","reason":"line 1:33: Invalid GROK pattern [(?:%{UUID:def})|(?:%{INT:def})]: the attribute [def] is defined multiple times with different types"}],"type":"parsing_exception","reason":"line 1:33: Invalid GROK pattern [(?:%{UUID:def})|(?:%{INT:def})]: the attribute [def] is defined multiple times with different types"},"status":400}

This can be considered syntactic sugar over a more complex manual pattern.

[flash1293]

Hmm, this fails BWC tests with a mixed cluster, which is expected since this is introducing a new syntax. I'm not sure how we handle this case, could you advise?

[luigidellaquila]

Thanks @flash1293, LGTM

I left just a couple of minor observations

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[luigidellaquila]

Hmm, this fails BWC tests with a mixed cluster, which is expected since this is introducing a new syntax. I'm not sure how we handle this case, could you advise?

Sorry, I missed this comment.
You'll have to add a new capability in EsqlCapabilities and then add required_capability: your_new_capability (lowercase) to the CSV test.

[elasticsearchmachine]

Hi @flash1293, I've created a changelog YAML for you.

[ivancea]

A bit of an edge case, but what would happen if some pattern is invalid? For example:

1. %{WORD:word}\
2. a)
3. %{WORD:word}

The final result would be: (?:%{WORD:word}\)|(?:a))|(?:%{WORD:word})

Which lead to unexpected patterns.

This is user-made, so I don't think this is very problematic, but this looks like a grok injection and the method combinePatterns() would actually be lying here.

So, solutions:

• Can we verify and warn on wrong patterns? Is that something we do in some other case?
• Can we sanitize or remove invalid patterns?

[ivancea]

Whatever the resolution, we'll need tests for this, both in Grok and in ESQL, depending on what we do

[flash1293]

That's a fun one - tested with ingest pipelines and this indeed works here:

POST /_ingest/pipeline/_simulate
{
  "docs": [
    {
      "_source": {
        "foo": "Test)x"
      }
    }
  ],
  "pipeline": {
    "processors": [
      {
        "grok": {
          "field": "foo",
          "patterns": [
            "%{WORD:word}\\",
            "x)"
          ]
        }
      }
    ]
  }
}

should fail because %{WORD:word}\\ and x) are not valid patterns, but it does pass, no warnings or similar.

I don't feel strongly about this case, we could pass the individual patterns down further and then validate this in the Grok lib, but:

• It seems like a lot of work for an edge case
• It's existing behavior in ingest pipelines
• We would need to compile each regex individually, which sounds like it would add a bunch of additional computation we are avoiding now

I would lean towards leaving the current behavior, wdyt?

[ivancea]

As the code was centralized, I guess the pipeline uses (and already used) the same logic right?

Being pragmatic, validating multiple patterns should error (probably?), which means that validating a single pattern should error too. So technically this is existing behavior, and fixing would be a breaking change (Or a bug fix).
So yeah, let's just make an issue explaining this case (For ESQL at least), and continue with this

[flash1293]

As the code was centralized, I guess the pipeline uses (and already used) the same logic right?

Exactly!

So technically this is existing behavior, and fixing would be a breaking change

I never get tired of quoting Hyrums Law 🙂

[flash1293]

Filed #136750