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

[Fix #795] Change implementation of Order param #798

Merged
merged 2 commits into from
May 20, 2021

Conversation

jiajiawang
Copy link
Contributor

Use Array to store the value of Chewy::Search::Parameters::Order.
To allow multiple sorting options that may have the same key name.
For example script based sorting whose key will always be _script.

Use Array to store the value of Chewy::Search::Parameters::Order.
To allow multiple sorting options that may have the same key name.
For example script based sorting whose key will always be `_script`.
@jiajiawang jiajiawang marked this pull request as draft May 19, 2021 12:22
@jiajiawang jiajiawang marked this pull request as ready for review May 19, 2021 13:13
@rabotyaga
Copy link
Contributor

Hey @jiajiawang 👋 !
This is really nice!
There is one thing, that bothers me. Actually, this is a breaking change. Someone might use chaining of request methods to build complex queries and they might chain order method too. After this change chained queries like SomeIndex.order('name').order('name'=>{'order' => 'desc'}) will start work differently.
Also, there is no much sense in having different sort options for the same field ({:sort=>["name", {"name"=>{"order"=>"desc"}}]}), except the special _script field.

What if we keep the previous behavior for all keys, except the _script one?

@jiajiawang
Copy link
Contributor Author

Hi @rabotyaga 👋 !
Thanks for the quick response.
You right. It is a breaking change.
Though, _script isn't the only problem we have. Similarly there is also _geo_distance.
Even for sorting on one certain field multiple times, there are also legitimate use cases.

For example, a numeric field price whose value is [10, 20, 30, ...] (price for different SKUs).
We may want to do {sort: [{price: { mode: 'min' }}, {price: {mode: 'avg'}}]} - sort products by minium SKU price, then by average SKU price if minimum is the same.

Most importantly I feel we should accept the sort body as long as it can be accepted by ES even though they sometime may don't make sense.

@rabotyaga
Copy link
Contributor

Valid points.
Let's then mark this change as breaking in the CHANGELOG (https://github.com/toptal/chewy/blob/master/CONTRIBUTING.md#changelog-entry-format) and somehow emphasize, that chained order calls with the same keys now work differently.

@jiajiawang
Copy link
Contributor Author

CHNAGELOG updated.

@rabotyaga
Copy link
Contributor

@jiajiawang Thank you very much!

@rabotyaga rabotyaga merged commit 261dc9e into toptal:master May 20, 2021
@jiajiawang jiajiawang deleted the order-array-storage branch July 20, 2021 01:23
cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023
* [Fix toptal#795] **(Breaking)** Change implementation of Order param

Use Array to store the value of Chewy::Search::Parameters::Order.
To allow multiple sorting options that may have the same key name.
For example script based sorting whose key will always be `_script`.
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.

2 participants