Skip to content

Add range match policy type for enrich#65781

Closed
probakowski wants to merge 15 commits intoelastic:masterfrom
probakowski:enrich-range
Closed

Add range match policy type for enrich#65781
probakowski wants to merge 15 commits intoelastic:masterfrom
probakowski:enrich-range

Conversation

@probakowski
Copy link
Copy Markdown
Contributor

This change adds new policy types to support all range types as enrich fields. It also adds new format field in enrich policy definition that lets you specify date format that will be used during creation of enrich index

Closes #59037

@probakowski probakowski added :Distributed/Ingest Node Execution or management of Ingest Pipelines v7.11.0 v8.0.0 labels Dec 2, 2020
@probakowski probakowski marked this pull request as ready for review December 2, 2020 23:21
@elasticmachine elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Dec 2, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks good and the change is smaller than I thought it would be.
Can you also add docs for this new enrich policy type?
Maybe also add a rest test to CommonEnrichRestTestCase?

}

if (EnrichPolicy.RANGE_MATCH_TYPES.contains(policyType)) {
// range match uses the same processor as simple match
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extend the comment here by including that a term query on a range field is translated into a range query with the values as lower and upper value. If multiple values are provided then each value is translates into a range query and that is then combined into a boolean query with should clauses, which means that on of the provided values need to fall into the range of the range field.

Copy link
Copy Markdown
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had just a few comments. +1 on adding some docs, but otherwise looks pretty good.

private static final ParseField INDICES = new ParseField("indices");
private static final ParseField MATCH_FIELD = new ParseField("match_field");
private static final ParseField ENRICH_FIELDS = new ParseField("enrich_fields");
private static final ParseField FORMAT = new ParseField("elasticsearch_version");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use a different field name in the constructor


private XContentBuilder resolveEnrichMapping(final EnrichPolicy policy) {
@SuppressWarnings("unchecked")
private XContentBuilder resolveEnrichMapping(final EnrichPolicy policy, GetIndexResponse getIndexResponse) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is getIndexResponse used in this method?

@jakelandis
Copy link
Copy Markdown
Contributor

@probakowski - can this be closed in favor of #76110 ?

@danhermann danhermann added v8.1.0 and removed v7.16.0 labels Oct 27, 2021
@mjmbischoff
Copy link
Copy Markdown
Contributor

@probakowski - can this be closed in favor of #76110 ?

The tests got reused, and I think all functionality is covered, please reopen if these assumptions are false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Ingest Node Execution or management of Ingest Pipelines Team:Data Management (obsolete) DO NOT USE. This team no longer exists. v8.0.0-rc1 v8.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support ip_range in Enrichment

10 participants