Search - clean up case sensitivity option in term-based queries #63923
Conversation
…ueries to an enum. Closes elastic#63893
|
Pinging @elastic/es-search (:Search/Search) |
| /** | ||
| * use the field's setting for handling matching | ||
| */ | ||
| FIELD_DEFAULT(new ParseField("field_default")), |
There was a problem hiding this comment.
I am wondering why we organize case_sensitivity options as parseFields, why not just strings?
as field_default and insensitive are not field names, but rather field values?
There was a problem hiding this comment.
Thanks, Mayya. I was just following the pattern from collect_mode in aggs which uses an enum. This all may be academic because we might be going with a simple boolean, pending discussion (addressed in alternative PR #63926 )
There was a problem hiding this comment.
@markharwood Thanks Mark, this PR LGTM, I left some comments.
Indeed, with enumeration is a more understandable API.
| if (caseInsensitive != DEFAULT_CASE_INSENSITIVITY) { | ||
| builder.field(CASE_INSENSITIVE_FIELD.getPreferredName(), caseInsensitive); | ||
| if (caseSensitivityMode != CaseSensitivityMode.FIELD_DEFAULT) { | ||
| builder.field(CaseSensitivityMode.KEY.getPreferredName(), caseSensitivityMode.parseField().getPreferredName().toString()); |
There was a problem hiding this comment.
Seems that in getPreferredName().toString() ,toString() part is redundant.
Here and in other queryBuilers as well
|
Thanks for the review, Mayya but closing in favour of #63926 |
Case sensitivity options are an uncomfortable fit with a boolean setting - changing to enum.
Closes #63893