Skip to content
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.

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

Add use_field option to intervals query #40157

Merged
merged 5 commits into from
Mar 20, 2019

Conversation

romseygeek
Copy link
Contributor

This is the equivalent of the field_masking_span query, allowing users to
merge intervals from multiple fields - for example, to search for stemmed tokens
near unstemmed tokens.

@romseygeek romseygeek added >feature :Search/Search Search-related issues that do not fall into other categories v7.2.0 labels Mar 18, 2019
@romseygeek romseygeek self-assigned this Mar 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@romseygeek
Copy link
Contributor Author

This currently only applies to the match rule, but it could be extended to all_of and any_of if we think that makes sense.

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 one comment regarding the validation of the field type but the change looks good to me.
use_field is ok but I also like mask_field or fix_field, no strong preference here just ideas so feel free to ignore if you think that use_field is better ;).

if (ft.tokenized() == false ||
ft.indexOptions().compareTo(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS) < 0) {
throw new IllegalArgumentException("Cannot create IntervalQuery over field ["
+ maskedField + "] with no indexed positions");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed ? Couldn't we check these options in MappedFieldType#intervals since use_field is only applicable on the match rule ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++, we already check for this in TextFieldMapper#intervals so the check is unnecessary here

if (in.getVersion().onOrAfter(Version.V_7_1_0)) {
this.useField = in.readOptionalString();
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you don't need the else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useField is final, so it needs to be set somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the final, thanks

@romseygeek
Copy link
Contributor Author

Thanks for the review @jimczi! I pushed some changes to address your comments.

but I also like mask_field or fix_field

I always get confused by mask_field as I think it's ambiguous as to which way the masking is happening, so I'd rather stick with use_field as I think it's clearest.

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.

LGTM, thanks @romseygeek

I always get confused by mask_field as I think it's ambiguous as to which way the masking is happening, so I'd rather stick with use_field as I think it's clearest.

Agreed and it is well documented so let's stick with use_field.

if (in.getVersion().onOrAfter(Version.V_7_1_0)) {
this.useField = in.readOptionalString();
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the final, thanks

@romseygeek romseygeek merged commit 64a53e4 into elastic:master Mar 20, 2019
@romseygeek romseygeek deleted the interval-fieldmask branch March 20, 2019 16:25
romseygeek added a commit that referenced this pull request Mar 20, 2019
This is the equivalent of the `field_masking_span` query, allowing users to
merge intervals from multiple fields - for example, to search for stemmed tokens
near unstemmed tokens.
romseygeek added a commit that referenced this pull request Mar 21, 2019
Now that the `use_field` option has been backported to the 7.x branch,
we can run BWC tests against all versions from 7.1

Follow up to #40157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Search/Search Search-related issues that do not fall into other categories v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants