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

Boost Suggestions by Context #544

Conversation

peteclark-ft
Copy link
Contributor

@peteclark-ft peteclark-ft commented Jun 14, 2017

Elasticsearch supports boosting by certain suggester categories - see here.

This PR adds support for the boost field, while maintaining backwards compatibility.

N.B. The order of contexts is not important to the query, and so I didn't maintain their order in the json output. I didn't want to change the way you did tests (by parsing the json and comparing them), or add any new dependencies, so I simply gave the output two acceptable options. Hope that's ok.

@olivere
Copy link
Owner

olivere commented Jun 16, 2017

Hey! Thanks for supporting elastic. I really need to read the documentation on suggesters first (I'm not an expert in this field), but the PR's looking good as far as I see it.

Re the tests: The output of the Go JSON serializer should be deterministic, so there should be no need for two expectations. Was it nondeterministic in your tests?

@peteclark-ft
Copy link
Contributor Author

Hi @olivere - weirdly I ran my tests locally and they passed, but they didn't pass in travis; so I assumed maps weren't ordered, and gave both options.. But you're right, the map key order is lexicographically sorted, so it should always output the same thing.

I've amended the tests back!

@olivere
Copy link
Owner

olivere commented Jun 16, 2017

Hmm... okay. The TestSuggesterCategoryQueryWithoutBoost test fails sporadically as SuggesterCategoryQuery.values is a map. The keys in a map are randomized when used in a range loop, so the test may or may not fail. I will change the test accordingly, and make it work in both cases.

@peteclark-ft
Copy link
Contributor Author

@olivere - ahh of course; I knew I wasn't going mad! We could maintain an additional array for ordering?

@olivere
Copy link
Owner

olivere commented Jun 16, 2017

@peteclark-ft I think I'll just make the test check for both options, as you already did. ES shouldn't care about the order, so no need to preserve the order.

@olivere olivere merged commit 619cd51 into olivere:release-branch.v5 Jun 16, 2017
@peteclark-ft
Copy link
Contributor Author

@olivere Thanks for merging! Good luck in the future!

@olivere
Copy link
Owner

olivere commented Jun 16, 2017

Your PR is going to make it into ES 5.0.40 soon. Thanks for your contributions.

@peteclark-ft peteclark-ft deleted the feature/context-suggester-boosting branch June 16, 2017 10:47
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

Successfully merging this pull request may close these issues.

None yet

2 participants