search

elastic-rs / elastic

An Elasticsearch REST API client for Rust
Apache License 2.0
252 stars 40 forks source link

Add `sort` to `Hit`. #413

Closed cristicbz closed 4 years ago

cristicbz commented 4 years ago

If you specify sort in an elasticsearch query, you'll get back a field called sort with the value that used for sorting that specific hit---this is sueful if you do any kind of fancy sorting or if passing _source=False but still want to paginate e.g. id-s.

Let me know if there's any kind of convention I've not adhered to, first time contributing to this repo.

On a related note---is there any reason why Hit doesn't have all public fields? I wanted to extract id and sort from my hits, and I currently have to clone to do that, even if I use into_hits. If Id, Index, Type derived Deserialize, they could be used as member types, removing the need for the conversion performed by the corresponding accessors.

Thanks a lot for this library! Using it over the official one for a couple of reason, but a big one is the presence of a SyncClient.

cristicbz commented 4 years ago

@mwilliammyers is there anything I can do to help get this PR merged? It's only a tiny change, hoping I don't need to depend on a git revision

mwilliammyers commented 4 years ago

Sorry for the extremely late response. I merged it in.

We are probably not going to be doing any more releases to crates.io but you can still use a git rev (but from this repo now that I have merged it).

I am curious about your other reasons for using this client over the official one; does the official client lack some features (besides a sync client)? We thought about doing a sync client for the official client but it doesn’t reduce any dependencies (it actually adds some because reqwest’s sync client just wraps tokio) and we figured it would be much harder to support both.

is there any reason why Hit doesn't have all public fields?

I am not sure; that was before my time here :) but I think most of the fields are exposed via getters. Let me check the docs...

Edit: Ahh, it’s missing id(). My documents have their own id field, so I use that. If you use the proc-macros to index documents, they should too because the proc-macros make you specify an id?

cristicbz commented 4 years ago

Thanks for the response!

Sorry to hear that this repo is not going to see any more releases. I think realistically the only reason is the sync client. We don't have any async rust in our stack---there's been a lot of churn in the ecosystem for one, but also doing async adds a lot of viral complexity to an application especially when trying to preserve load shedding, back pressure and mixing computational and io workloads. Ultimately async is an optimisation which in our case would be very much unnecessary---with a few dozen threads our services can handle everything that's thrown at them.

I can understand though that maintaining sync and async is a significant burden. The rust world seems to be very much moving inexorably towards async everything, so we may need to reconsider our position sooner or later---it won't be a small job. I'm still holding out hope that writing sync servers will remain viable in the long run

On Wed, 17 Jun 2020, 22:08 William Myers, notifications@github.com wrote:

Sorry for the extremely late response. I merged it in.

We are probably not going to be doing any more releases to crates.io but you can still use a git rev (but from this repo now that I have merged it).

I am curious about your other reasons for using this client over the official one; does the official client lack some features (besides a sync client)? We thought about doing a sync client for the official client but it doesn’t reduce any dependencies (it actually adds some because reqwest’s sync client just wraps tokio) and we figured it would be much harder to support both.

is there any reason why Hit doesn't have all public fields?

I am not sure; that was before my time here :) but I think most of the fields are exposed via getters. Let me check the docs...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/elastic-rs/elastic/pull/413#issuecomment-645625501, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGDUMELB6OJU2WXFJFRIK3RXEWDDANCNFSM4LRC2C5A .