-
Notifications
You must be signed in to change notification settings - Fork 359
Fixing bug where engine does not respect Algolia order #365
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
Conversation
Fixes #341 by sorting the map() results.
|
I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework. Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc. Thanks! |
|
I assumed the discussion in #341 would be enough to explain the situation. Tests didn't protect against this bug, so I don't know why the patch requires tests. |
|
It should be mentioned that Algolia doesn't do a relevancy sort. This means that unless a sort is defined in Algolia, results come back as last in, first out. This is likely why this hasn't been noticed until now because I would guess that the majority of Algolia users don't define custom sorting. Algolia simply collects objects that match the query and returns them. There's no keyword density sorting that happens under the hood. I assume that this hasn't been noticed because Scout pushes records into Algolia in a method that "matches" how they come out of Algolia (last in, first out). We are working on an AWS CloudSearch Scout driver and noticed this instantly because CloudSearch does do relevancy sorting. |
|
@ChrisThompsonTLDR please always provide a thorough explanation in your first comment and not just link to an issue. Do please try to include a test. Try to submit again with these two points in mind. |
|
I'll pass. I already deleted my fork. |
Push this into an Algolia index, then search for You will see that they come back in the last in, first out method. Now if you have these in a database, and you use Scout to import them, as long as you create them in this order 1, 2, 3, 4, 6, 5, 7, you will see that when they come back in Scout, they will be in 7, 6, 5, 4, 3, 2, 1, but if you search in Algolia, they will return as 7, 5, 6, 4, 3, 2, 1. This is because Scout orders by the model's key on import, making them import as 7, 6, 5, 4, 3, 2, 1. |
|
Hey @ChrisThompsonTLDR, I sent in a PR with the fix along with a test. You can view the commit here: dafbfa7 and the PR here: #369 I hope this makes it clear how a test can help here. If you have any questions about this feel free to hit me up here or on Discord or so! |
Fixes #341 by sorting the map() results.