-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add multi search #288
base: main
Are you sure you want to change the base?
Add multi search #288
Conversation
b57a67c
to
350ad3f
Compare
/** | ||
* @return class-string | ||
*/ | ||
private function getClassNameFromIndex(string $index): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a common thing in the bundle, no? I'm wondering if this could be reused somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I'd leave that for later :)
@brunoocasali I'm not sure if we should have a totally separate |
Why you don't think the If yes, I'm not sure it is the right approach... Since I could have a lot of defined models but want to search in a subset, in this case I would have a lot of useless data being sent over the network. |
That's because in bundle we operate on the class level, so you instantiate a I followed the same approach as for the single index search |
Oh, okay I see it now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment + fixing the styling, and we're good to go!
// $response = $this->engine->multiSearch(array_map(function (SearchQuery $query) use ($prefix, $indices) { | ||
// $this->assertIsSearchable($query->getClassName()); | ||
// | ||
// return $query->toEngineQuery($prefix, $indices); | ||
// }, $queries)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
aggregator support needs to be added I think also, but first I need to understand it how to make it work :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM!
Any up for this ? Really looking for it, would be really useful 👀 What's failing in the C.I ? |
// $response = $this->engine->multiSearch(array_map(function (SearchQuery $query) use ($prefix, $indices) { | ||
// $this->assertIsSearchable($query->getClassName()); | ||
// | ||
// return $query->toEngineQuery($prefix, $indices); | ||
// }, $queries)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// $response = $this->engine->multiSearch(array_map(function (SearchQuery $query) use ($prefix, $indices) { | |
// $this->assertIsSearchable($query->getClassName()); | |
// | |
// return $query->toEngineQuery($prefix, $indices); | |
// }, $queries)); |
@brunoocasali do you have any suggestions how to make this work with aggregators? For example if you define Post index and then an aggregator that is also uses Post then there is an ambiguity where to search. |
Can you remind me about what the "aggregators" should do in the first place? Is it the ability to map two models into the same index? Because in any case, I think we could release without that and figure out that later, WDYT? |
I think so..
Okay maybe :) also I think it's possible to define same class as multiple indexes,so there would be same problem that it would search also in all of those 🤔 |
Do you mean, even without this PR? Because I think this could be a problem indeed, so we would not need to worry about that possibility here, right? |
I think if you feel the code for the feature is good enough you can push forward and then grab some user feedback about the feature, WDYT? I also encouraged the rails maintainer to do a similar approach, instead of waiting to have the best public API in town 😅. |
Yeah ok,just need to find some time to finish it:) |
Hi, do you need help here? Maybe I'll be able to do something. Multisearch is an important functionality for me :) |
Hi, if you want to continue the work of this then go ahead. |
What is the work required? Is it to remove the unnecessary comment and add getters to fix the code style issues? |
To decide properly how it should behave :) |
Pull Request
Related issue
Fixes #246
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Opening this PR to discuss the implementation, because not sure what are the expectations, so not ready to merge :)