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

The Phrase suggester should support multiple direct_generators #1963

Closed
sergiuionescu opened this issue May 28, 2021 · 6 comments
Closed

The Phrase suggester should support multiple direct_generators #1963

sergiuionescu opened this issue May 28, 2021 · 6 comments

Comments

@sergiuionescu
Copy link
Contributor

The current Phrase suggester(https://github.com/ruflin/Elastica/blob/master/src/Suggest/Phrase.php#L12) only supports one generator. Subsequently calling \Elastica\Suggest\Phrase::addCandidateGenerator always overwrites the previous generator value.

According to https://www.elastic.co/guide/en/elasticsearch/reference/current/search-suggesters.html#_direct_generators multiple generators can be used. Ex:

{
    "suggest": {
        "text": "obel prize",
        "simple_phrase": {
            "phrase": {
                "field": "title.trigram",
                "size": 1,
                "direct_generator": [
                    {
                        "field": "title.trigram",
                        "suggest_mode": "always"
                    },
                    {
                        "field": "title.reverse",
                        "suggest_mode": "always",
                        "pre_filter": "reverse",
                        "post_filter": "reverse"
                    }
                ]
            }
        }
    }
}

At the moment the workaround we can use is:

\Elastica\Suggest\Phrase::setParam('direct_generator', [
    $trigramGenerator->toArray()['direct_generator'],
    $reverseGenerator->toArray()['direct_generator'],
])

Is it possible to update the Phrase suggester class to support a list of generators?

Thanks!

@ruflin
Copy link
Owner

ruflin commented May 31, 2021

@sergiuionescu Interested to open a PR? 😇

@sergiuionescu
Copy link
Contributor Author

sergiuionescu commented May 31, 2021

@ruflin Please have a look at #1964 .
Please run the github action tests, i haven't been able to spin up the integration env locally.

@ruflin
Copy link
Owner

ruflin commented Jun 1, 2021

Thanks! Test should always be run automatically.

@deguif
Copy link
Collaborator

deguif commented Jun 1, 2021

@ruflin they are not run anymore automatically for new contributors.
I had to approve @sergiuionescu work for running the tests as github recently changed this behaviour to avoid abuses (crypto mining, ...).

@deguif
Copy link
Collaborator

deguif commented Jun 1, 2021

@deguif
Copy link
Collaborator

deguif commented Jun 7, 2021

Closing as #1964 was merged and fixed that.

@deguif deguif closed this as completed Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants