Add rewrite param setters to all relevant queries #2242
Conversation
Sharptsa
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds setter methods for the rewrite parameter to the Prefix, Regexp, and Fuzzy query classes and introduces constants for all possible rewrite values, complementing the existing implementation in the Wildcard query class.
- Added setRewrite methods and associated constants to Fuzzy, Prefix, and Regexp queries.
- Updated the CHANGELOG to document these new additions.
Files not reviewed (7)
- src/Query/Fuzzy.php: Language not supported
- src/Query/Prefix.php: Language not supported
- src/Query/Regexp.php: Language not supported
- src/Query/Wildcard.php: Language not supported
- tests/Query/FuzzyTest.php: Language not supported
- tests/Query/PrefixTest.php: Language not supported
- tests/Query/RegexpTest.php: Language not supported
ruflin
left a comment
ruflin
left a comment
There was a problem hiding this comment.
Overall LGTM. I was wondering if we could abstract out parts but then I also come to the conclusion, that it might not worth the effort and we rather have some duplicated code.
| /** | ||
| * Rewrite methods: @see https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-multi-term-rewrite.html. | ||
| */ | ||
| public const REWRITE_CONSTANT_SCORE = 'constant_score'; |
There was a problem hiding this comment.
Are these const always the same. I wonder if we should define these once in AbstractQuery. But then, they don't apply to all queries. What is the pattern of all the queries this applies to? Should we have an abstract query or similar that all these extend?
There was a problem hiding this comment.
These constants are always the same. The only pattern between these, is that they are all Term-level queries that use wildcards in some shape or form.
When writing the PR I was also debating puttings the constants in AbstractQuery but reached the same conclusion.
I guess we could have a new level of abstract query for these. Not sure how to name it though.
| * | ||
| * @see https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-multi-term-rewrite.html | ||
| */ | ||
| public function setRewrite(string $rewriteMode): self |
There was a problem hiding this comment.
These methods are always identical?
There was a problem hiding this comment.
It's the same for Prefix and Regexp, also for Wildcard except the null check on $this->field since it's set in the constructor.
For Fuzzy I used the setFieldOption shortcut since it was already present but it can rewritten the same as the others.
IMHO, these queries could use a refactor in the long run, to harmonize them and have consistent constructors.
Right now, Prefix takes an array, the others a field and value, but some are nullable and some are not.
It would create breaking changes though.
| public const REWRITE_CONSTANT_SCORE = 'constant_score'; | ||
| public const REWRITE_CONSTANT_SCORE_BOOLEAN = 'constant_score_boolean'; | ||
| public const REWRITE_SCORING_BOOLEAN = 'scoring_boolean'; | ||
| public const REWRITE_TOP_TERMS_BLENDED_FREQS_N = 'top_terms_blended_freqs_N'; |
There was a problem hiding this comment.
Interesting that we had some of the const already
|
For the CI runs, looks like we need do update Ubuntu first, we got the following error message:
|
|
I opened a PR with a fix here: #2243 |
|
I just merged #2243 which should fix the CI issue. Can rebase / update the branch? |
- add 'rewrite' param setter to Fuzzy - add 'rewrite' param setter to Prefix - add 'rewrite' param setter to Regexp
Sharptsa
force-pushed
the
feature/add_rewrite_param
branch
from
549fbd4 to
c906f90
Compare
|
This new push slipped through on my end. Any chance you could rebase this on 9.x so we can get this into 9.x and backport it from there to 8.x? |
|
I changed the target of this PR to go against 9.x. We can always backport to 8.x afterwards. |
2 participants
I recently had to play with the
rewriteparams on a few queries and only theWildcardandQueryStringclasses have a setter for this param.This PR adds setters for the
rewriteparams for all relevant the relevant Query classes where it is missing (Prefix,RegexpandFuzzy), based on the Elasticsearch documentation https://www.elastic.co/docs/reference/query-languages/query-dsl/query-dsl-multi-term-rewrite. As well as adding constants for all the possible values for this param.