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

[FEATURE] Add "suggest" to SearchResponse #7390

Closed
svenhartmann opened this issue Mar 17, 2023 · 13 comments · Fixed by #7894
Closed

[FEATURE] Add "suggest" to SearchResponse #7390

svenhartmann opened this issue Mar 17, 2023 · 13 comments · Fixed by #7894
Labels
8.x Relates to 8.x client version
Milestone

Comments

@svenhartmann
Copy link

svenhartmann commented Mar 17, 2023

It looks like the SearchResponse is missing serialization for a suggestion response yet, e.g. https://www.elastic.co/guide/en/elasticsearch/reference/current/search-suggesters.html#querying

@stevejgordon
Copy link
Contributor

Thanks for raising this @svenhartmann. The suggesters property had some issues with code-gen and is on a list for me to revisit. I'll keep this issue open to track that progress. For the second item, I'll need to investigate. That might be an issue with the spec.

@stevejgordon stevejgordon added the 8.x Relates to 8.x client version label Mar 20, 2023
@stevejgordon stevejgordon changed the title Add "suggest" to SearchResponse / Error during CompletionSuggester (8.x Client) [FEATURE] Add "suggest" to SearchResponse / Error during CompletionSuggester Mar 20, 2023
@stevejgordon
Copy link
Contributor

stevejgordon commented Mar 23, 2023

@svenhartmann For the second item, the second code snippet is the correct way to configure the suggester. We model this in the spec as a specialised container concept, with FieldSuggester being able to contain one of the three possible suggester variants (completion, phrase and term). There is an issue with our spec as prefix (and regex) shouldn't appear on the CompletionSuggester variant. I'm going to split this into two issues so we can track and resolve each independently.

The usage for container types like FieldSuggester is something I'd like to improve in our code gen so that setting the container properties can be achieved with the factory methods. I have an issue #7400 capturing such improvements.

@stevejgordon stevejgordon changed the title [FEATURE] Add "suggest" to SearchResponse / Error during CompletionSuggester [FEATURE] Add "suggest" to SearchResponse Mar 23, 2023
@svenhartmann
Copy link
Author

@stevejgordon Would be nice to have this feature and the range queries within the next release. Is it feasible?

@stevejgordon stevejgordon added this to the Future milestone Apr 13, 2023
@svenhartmann
Copy link
Author

@stevejgordon Any news here?

@stevejgordon
Copy link
Contributor

@svenhartmann I looked at this for 8.1.0, but we still need to do some work in code-gen to handle the spec and make the code usable. I don't have an ETA for when we will pick this up but agree it's a gap we need to fill.

@svenhartmann
Copy link
Author

@stevejgordon Seems that its not in the 8.1.1 release. I started migration of a webshop search of a customer in march and the only features that breaks the upgrade is the missing suggest response.

@stevejgordon
Copy link
Contributor

@svenhartmann I didn't get a chance to focus on this for the latest release, so unfortunately, it is still open. I have moved on from maintaining this library day-to-day which has slowed progress, and we have a new engineer starting today to take ownership. We can't provide any timescales for this item at the moment, but it will be picked up when possible.

@asal-hesehus
Copy link

I am also migrating to the new client and are also blocked by this missing feature.

@TheFireCookie
Copy link
Contributor

Hello, we are also trying to migrate to Elastic.Clients.Elasticsearch 8.1.x (to upgrade also from ES7 to ES8) and we also have this issue with the suggestions that is blocking our upgrade. Could we provide you some help ? If you give me some guidance, I can take a look to fix that issue?

@Robbware
Copy link

Hello, I've also ran into this roadblock for an upgrade on our current project and would appreciate if this could be added!

@timschwallie
Copy link

It looks possible to use
string response = resultQuery.ApiCallDetails.ResponseBodyInBytes.ToUtf8String();
to grab the response in json format.
From there it looks like the "suggest" isn't set up right for an easy conversion to c# objects. (using json2csharp.com). Basically, it sets up each suggestor as parameters rather than an array. Whiff.
Probably need to use dynamic objects.

@TheFireCookie
Copy link
Contributor

It worked in NEST 7.x so I guess we could/should take inspiration from the old code to implement it in the new client.

@timschwallie
Copy link

From what I see, the real issue is the JSON produced from ES. Fix that and a lot of the client code on many platforms becomes much easier to code, maintain, and performs better. Basically a lot of band-aiding going on with client code.
I pretty sure another approach I have in mind would work with current JSON, result in easier to code to maintain and faster, but I need more coffee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to 8.x client version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants