Allow range types to be used for enrich matching

[mjmbischoff]

Currently we hardcode that the data type for the match field on the enrich index, when the MATCH policy is used, this is (always) keyword. This breaks look ups when source indicies use a rangetype for lookup. For example when a ip_range is used the literal value from the source is indexed as a keyword and therefor doesn't match any of the ip's.

This PR improves the code by checking for the field type on the source indicies and then proceeds to use the range type in the enrich index if this is the type for all source indicies. If the types are different across source indices, we fallback to keyword. For non-range types we also fallback to keyword, emulating the current behavior.

---

• Have you signed the contributor license agreement?
• Have you followed the contributor guidelines?
• If submitting code, have you built your formula locally prior to submission with gradle check?
• If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
• If submitting code, have you checked that your submission is for an OS and architecture that we support?

[jbaiera]

The code looks pretty good here, though I can't seem to remember if we had plans to separate ranges out from the match enrich policy type. Searching for data using indexed ranges feels closer to how geo_match works, but the underlying query DSL is indeed closer to how match would write it.

@martijnvg do you remember if that was the plan or not?

[martijnvg]

Apologies for responding late to this PR.
Like James, I think the code in this PR looks good.

@martijnvg do you remember if that was the plan or not?

Yes, the plan was to introduce a new enrich policy type for ranges.
This has the added benefit that actual range can be queried in addition to literal values.
Przemko started with a pr for this a while ago: #65781 (but this got stalled due to inactivity).
However maybe this enhancement to the match type can exist next with the future range policy type?
What do you think?

[jbaiera]

Since we have two separate policy types (match and geo_match) for different query types, I think I'd lean more toward keeping design parity and introducing a third policy type. While the underlying query DSL still uses a match query, we always intended the match policy type to enrich based on singular values. Adding a range policy type gives us the flexibility to support future growth, while adding range support in the match policy type may muddy the waters going forward should the functionality be duplicated.

Does the policy type pertain more to the query DSL created, or is it more concerned with the field type of the enrich key? I'll throw the team discuss label on here to see if we can reach consensus.

[elasticmachine]

Pinging @elastic/es-core-features (Team:Core/Features)

[martijnvg]

Since we have two separate policy types (match and geo_match) for different query types, I think I'd lean more toward keeping design parity and introducing a third policy type. While the underlying query DSL still uses a match query, we always intended the match policy type to enrich based on singular values. Adding a range policy type gives us the flexibility to support future growth, while adding range support in the match policy type may muddy the waters going forward should the functionality be duplicated.

Muddying the waters when it comes to policy types is a valid concern if this change would get merged. Thinking from that perspective, maybe the match processor should not be changed to use more field types for the match field.

Does the policy type pertain more to the query DSL created, or is it more concerned with the field type of the enrich key?

I think it is both. It controls what field type to use the match field (during policy execution) and the query that is executed against this field (in the processor implementation).

[mjmbischoff]

Apologies for responding late to this PR.

No worries, you should enjoy your PTO.

Przemko started with a pr for this a while ago: #65781 (but this got stalled due to inactivity).

Missed this, will have a closer look at it to see what I've missed.

Added the changes to make it a separate policy but realized I forgot about the configuring part. I'll see if I can pick this up later.

[mjmbischoff]

Looking into #65781 , it seems a format field is introduced for date but no such parameter is described for https://www.elastic.co/guide/en/elasticsearch/reference/7.14/query-dsl-term-query.html in our docs (which seems incomplete as it doesn't describe the range matching either) it is described in the type description for the mapping https://www.elastic.co/guide/en/elasticsearch/reference/7.14/range.html

It is described for the range query but that is for taking a range as the input to match on a value (or possible range), while I'm sure some user can find a reason to match like that I think that would be a different policy (and PR) it does make me wonder if the naming for the policy we introduce here is the right one. And potentially re-evaluate the split?

I should add some tests for dates including exotic date formats that it won't magically pick to check if we need the format configured and if it needs to be user specified or copied from the mapping of the source / enrich index.

[martijnvg]

@mjmbischoff Looks like the format option in #65781 is used for format option of the date field in the enrich index. However perhaps this isn't needed and can the format option be extracted from the mapping of the source index during enrich policy execution.

It is described for the range query but that is for taking a range as the input to match on a value (or possible range), while I'm sure some user can find a reason to match like that I think that would be a different policy (and PR) it does make me wonder if the naming for the policy we introduce here is the right one. And potentially re-evaluate the split?

Right, perhaps ranges should be handled by a different kind of policy, like the one proposed in #65781.

[mjmbischoff]

Going with RANGE_TYPE, allowing RANGE_MATCH_TYPE or better name to be used for matching ranges against values, ranges.

Added tests for date_range which indeed triggered on the format being missing. Similar to types, requiring a singular distinct format specification or otherwise we fail and inform the user. If someone feels they need can argue that they need multiple source indicies with different format fields feel free to open an ER arguing your case. Merging these is non-trivial, because one format might mask the other etc.

[mjmbischoff]

@martijnvg @jbaiera think it's ready, can you give it a check.

[martijnvg]

Looks good @mjmbischoff! I left a few more comments around simplification.

[martijnvg]

@elasticmachine update branch

[martijnvg]

Thanks for the quick update! I left a few more comments.

[martijnvg]

Thanks for iterating on this PR @mjmbischoff.
I think this PR looks good now.
@jbaiera do you think this PR is good to get merged?

[jbaiera]

LGTM, thanks for the changes!

[mjmbischoff]

@martijnvg I did the honors, is it ok if I leave backporting (if applicable) to you?

[martijnvg]

I did the honors

🎉

is it ok if I leave backporting (if applicable) to you?

Yes, I will do the back porting to the 7.x branch.