-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Conversation
Pinging @elastic/es-search |
This currently only applies to the |
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Thanks for the review @jimczi! I pushed some changes to address your comments.
I always get confused by |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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
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
This is the equivalent of the
field_masking_span
query, allowing users tomerge intervals from multiple fields - for example, to search for stemmed tokens
near unstemmed tokens.