Skip to content

Make intervals queries fully pluggable through field mappers. #71429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 20, 2021
Merged

Make intervals queries fully pluggable through field mappers. #71429

merged 1 commit into from
Apr 20, 2021

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 7, 2021

MappedFieldType only allows configuring match and prefix queries today.
This change makes it possible to configure how to create wildcard and fuzzy
queries as well.

This will allow making the upcoming match_only_text field fully support
intervals queries.

@jpountz
Make intervals queries fully pluggable through field mappers.
bd1fcf5 
`MappedFieldType` only allows configuring `match` and `prefix` queries today.
This change makes it possible to configure how to create `wildcard` and `fuzzy`
queries as well.

This will allow making the upcoming `match_only_text` field fully support
intervals queries.
@jpountz jpountz added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.13.0 labels Apr 7, 2021
@jpountz jpountz requested a review from romseygeek April 7, 2021 16:56
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Apr 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

jimczi
jimczi approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left a minor comment, LGTM otherwise

server/src/main/java/org/elasticsearch/index/query/IntervalsSourceProvider.java
if (useField != null) {
source = Intervals.fixField(useField, source);
}
return source;
}

private void checkPositions(MappedFieldType type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the check to still throw a comprehensive exception when positions are not available on the field ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check has moved to TextFieldType so that things would still work with MatchOnlyTextFieldMapper which doesn't index positions but computes them on the fly. Does it address your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right I skipped that part. Thanks, that addresses my question.

@romseygeek
Copy link
Contributor

I wonder if rather than having four intervals methods on MappedFieldType, we could merge things down to a single interval method that takes a String term and an enum { TERM, PREFIX, FUZZY, WILDCARD } and then move the IntervalBuilder calls back up into IntervalsSourceProvider?

We can probably just remove all the checkPositions() calls as well, tbh, as there will be a check within IntervalQuery itself so I'm not sure this additional verification is useful.

@jpountz
Copy link
Contributor Author

jpountz commented Apr 8, 2021

I wonder if rather than having four intervals methods on MappedFieldType, we could merge things down to a single interval method that takes a String term and an enum { TERM, PREFIX, FUZZY, WILDCARD }

fuzzy makes it a bit tricky because it takes additional parameters, and I can imagine other rules taking new parameters in the future as well?

We can probably just remove all the checkPositions() calls as well, tbh, as there will be a check within IntervalQuery itself so I'm not sure this additional verification is useful.

No strong feelings but I like that we're failing early at query parsing time this way, while we'd otherwise fail when creating the scorer. In the prefix case when we delegate to the prefix field type, it also helps have the message about the right field name (ie. not the field name of the sub prefix field).

romseygeek
romseygeek approved these changes Apr 19, 2021
View reviewed changes
Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jpountz

@jpountz jpountz merged commit 25750a3 into elastic:master Apr 20, 2021
@jpountz jpountz deleted the plug_intervals_mapped_field_type branch April 20, 2021 16:10
@jpountz jpountz added v7.14.0 and removed v7.13.0 labels Apr 20, 2021
@jpountz jpountz mentioned this pull request Apr 21, 2021
Merged
jpountz added a commit that referenced this pull request Apr 21, 2021
@jpountz
Make intervals queries fully pluggable through field mappers. (#71429)
1c1ed53 
`MappedFieldType` only allows configuring `match` and `prefix` queries today.
This change makes it possible to configure how to create `wildcard` and `fuzzy`
queries as well.

This will allow making the upcoming `match_only_text` field fully support
intervals queries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimczi jimczi jimczi approved these changes

@romseygeek romseygeek romseygeek approved these changes

Assignees
No one assigned
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.14.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jpountz @elasticmachine @romseygeek @jimczi @jakelandis