toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client
MIT License
1.89k stars 368 forks source link

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

Closed jiajiawang closed 3 years ago

jiajiawang commented 3 years ago

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.

rabotyaga commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

CHNAGELOG updated.

rabotyaga commented 3 years ago

@jiajiawang Thank you very much!