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

Initial Elasticsearch support #7

Merged
merged 14 commits into from Sep 2, 2016

Conversation

Projects
None yet
@hernandev
Copy link

hernandev commented Aug 13, 2016

Reopened since squashing would include other poeples commits from the upstream sync.

Initial Elasticsearch support, tests on the way.

Some options may differ and I may make a custom config key for customizing the query parameter, Elastic can be really complex but this setup should cover most scenarios.

Working right now:

  • Searching
  • Searching with where conditions
  • Pagination
  • Unit Testing :)

TODO:

  • For future implementations, for both algolia and elastic, may the performSearch be a replaceable callback,so people can extend and customize it, gonna send a separate PR on it

@hernandev hernandev referenced this pull request Aug 13, 2016

Closed

Elasticsearch Support #6

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

Looks good, only thing is I would suggest making the index configurable. Instead of just 'laravel' it should go off of environment variables / the config file. My app has a different index per customer so that would be super helpful.

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

Also, Taylor will probably suggest that you don't add elasticsearch/elasticsearch to the require block, just to the suggestion block so people using Agolia don't have to download the elasticsearch composer package.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 13, 2016

@tomschlick great, thanks for the idea!

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 13, 2016

@tomschlick unless i'm misteken its on require-dev only

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

Oops! The diff viewer was only showing me the line above it and not the block header. Carry on! :) 👍

Diego Hernandes
initial elastic search support
fix importing and prevent key error

pagination working, needs refactoring
@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

FYI you don't have to keep squashing each time you commit anymore since github allows the team to do that at merge :)

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 13, 2016

Just included pagination, not really beauty since elastic does not gives page information, so some math needs to be done, gonna start refactoring the whole thing now. but looks like the support for it is now complete

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 13, 2016

thanks @tomschlick didn't knew that

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

Regarding the index name: there is a searchableAs() method on the Searchable trait that you should be using to determine the index name.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 13, 2016

@tomschlick i'm using that to determine the 'type' field on elastic search

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 13, 2016

Ah ok. Sorry missed that.

Tom Schlick

On Sat, Aug 13, 2016 at 6:28 PM -0400, "Diego Hernandes" notifications@github.com wrote:

@tomschlick i'm using that to determine the 'type' field on elastic search


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@hernandev hernandev referenced this pull request Aug 13, 2016

Closed

Elasticsearch Support #5

@hernandev hernandev changed the title Initial elastic search support Initial Elasticsearch support Aug 14, 2016

*/
public function update($models)
{
foreach($models as $model) {

This comment has been minimized.

@jmarcher

jmarcher Aug 14, 2016

Contributor

use the each() method form collections

*/
public function delete($models)
{
foreach($models as $model) {

This comment has been minimized.

@jmarcher

jmarcher Aug 14, 2016

Contributor

use the each() method form collections

This comment has been minimized.

@hernandev

hernandev Aug 14, 2016

Author

indeed, sending in a bit

This comment has been minimized.

@hernandev
@@ -30,6 +31,14 @@ public function createAlgoliaDriver()
));
}
public function createElasticsearchDriver()
{
return new Engines\ElasticsearchEngine(Elasticsearch::create()

This comment has been minimized.

@nWidart

nWidart Aug 14, 2016

You could use the ClientBuilder class from Elasticsearch:

ClientBuilder::fromConfig(config('scout.elasticsearch'))

This will allow people to use a more in depth configuration than just the hosts.

This comment has been minimized.

@hernandev

hernandev Aug 14, 2016

Author

that's client builder, aliased

This comment has been minimized.

@nWidart

nWidart Aug 15, 2016

But you're hard coding the hosts config option.
There is no way here for me to set additional options on the ES client.

This comment has been minimized.

@hernandev

hernandev Aug 15, 2016

Author

ok, noticed that option now, could be a great option for custom setups. thanks @nWidart

This comment has been minimized.

@nWidart

nWidart Aug 15, 2016

No problem, good job on the PR! You were faster than me to send it. 😄

@adiachenko

This comment has been minimized.

Copy link
Contributor

adiachenko commented Aug 14, 2016

@hernandev Just to add on the topic of configurable index and why it's useful, here is another valid use case I've seen in some apps:

'index' => 'search_' . app()->getLocale(),
@alexbowers

This comment has been minimized.

Copy link

alexbowers commented Aug 14, 2016

How would this handle AWS which is on version 1.5, and Elastic.co which is on 2.3?

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 14, 2016

However the current elasticsearch/elasticsearch library would handle it. 
If that's not compatible it should be pretty trivial to fork this one and set the library to an earlier compatible version. 

Tom Schlick

On Sun, Aug 14, 2016 at 10:13 AM -0400, "Alex Bowers" notifications@github.com wrote:

How would this handle AWS which is on version 1.5, and Elastic.co which is on 2.3?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 14, 2016

@alexbowers from the php library readme

If you are using Elasticsearch 1.x or 2.x, prefer using the Elasticsearch-PHP 2.0 branch. The 1.0 branch is compatible however.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 14, 2016

@adiachenko Elasticsearch docs says that for performance, each index becomes a different shard, and that elastic would handle better single index with multiple types, that's why laravel is the prefixed index, gonne make index a configurable key so people can custom it if needed at run time as well, but would be better if everyone just overwrite the seachableAs() method to customized the type key instead of index key

@paulocastellano

This comment has been minimized.

Copy link

paulocastellano commented Aug 14, 2016

Great job!

Diego Hernandes added some commits Aug 15, 2016

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 15, 2016

@nWidart you comments gonna be hidden since that code is not active anymore, included a config array on the config, so you can pass specific configuration there instead of the hard coded config it was using before.

Also, for those who requested, index name is a separate config option

Diego Hernandes
Diego Hernandes
Merge pull request #2 from hernandev/revert-1-patch-1
Revert "Update ElasticsearchEngine.php"
@VinceG

This comment has been minimized.

Copy link

VinceG commented Aug 24, 2016

@intech I agree. We have the same issue, i think that if we can make Scout just more flexible in terms of filtering and searching it'll at least make it somewhat useable in the real world applications. As it stands right now most applications just can't use it out of the box. I like the concept and this approach so i'll be definitely helping with this once it's merged.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 24, 2016

@intech @VinceG I know the expectations for more complex and robust options would be nice, but let's try making it functional for general and easy use-cases at first, there are performance, relevance and other topics really important but lets try do do it incrementally instead, agreed?

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 24, 2016

@intech still gonna review and merge your changes for bulk operations on this initial PR

@intech

This comment has been minimized.

Copy link
Contributor

intech commented Aug 24, 2016

@hernandev @VinceG now comes first DSL for search queries - https://github.com/ongr-io/ElasticsearchDSL. And here's an interesting moment with mapping in requests depend on it a lot of them on the types of documents field.

@VinceG

This comment has been minimized.

Copy link

VinceG commented Aug 24, 2016

@intech that's awesome.

@intech

This comment has been minimized.

Copy link
Contributor

intech commented Aug 24, 2016

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Aug 24, 2016

@intech looks awesome indeed

@intech intech referenced this pull request Aug 24, 2016

Closed

Welcome to laravel-scout #20

@sleimanx2

This comment has been minimized.

Copy link

sleimanx2 commented Aug 25, 2016

Hey I've been working on this package for a while now.

There is a good amount of similarity with scout however plastic covers a lot of elastic specific queries and mappings.

We started a conversation here about this topic If you guys think that we can move plastic to become a scount driver please let me know.

I think at least we can use some common features like the result to model mapping.

@VinceG

This comment has been minimized.

Copy link

VinceG commented Aug 25, 2016

@sleimanx2 I love the idea and the advanced functionality. I doubt that'll merge that project into scout as that is ES specific. they need to build these engines abstracted so the functionality remains the same when switching between engines. For example if i use Plastic and then decide to switch to Algolia i'll have to re-write the search code. While Scout tries to solve this problem by abstracting everything, we can "borrow" certain functionality from Plastic but i am not sure if @taylorotwell will merge such a large package into Scout.

$0.02

@intech

This comment has been minimized.

Copy link
Contributor

intech commented Aug 25, 2016

@VinceG In this case, we need to convince @taylorotwell that Elasticsearch 🏭 much more promising and popular than service Algolia 👶 ))

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 25, 2016

I think plastic should be a seperate PR. I say let this one go through with the basic functionality and then create a new driver called plastic and hook in new methods where you need them.

Right now though, we just need to get ES in place so we can actually use it.

@jmarcher

This comment has been minimized.

Copy link
Contributor

jmarcher commented Aug 25, 2016

I think we should keep this simple, if someone needs more functionality they can use Plastic.
Anyway instead of asking Taylor to change the actual driver we could just decorate the actual one

@sleimanx2

This comment has been minimized.

Copy link

sleimanx2 commented Aug 26, 2016

I'm also not sure if it's a good idea to build elastic DSL queries using SQL syntax like where and whereIn. Using DSL terminology makes it easier to understand and debug the query if you're referencing the official docs or have a prior experience with elastic.

@intech

This comment has been minimized.

Copy link
Contributor

intech commented Aug 28, 2016

@VinceG

This comment has been minimized.

Copy link

VinceG commented Aug 28, 2016

@intech i like that one. it uses bulk operations.

@VinceG

This comment has been minimized.

Copy link

VinceG commented Aug 28, 2016

@sleimanx2 True. but at least map some of the basic functionality, it'll make things much easier to transition from one engine to another. i am not saying to abstract the whole thing, just the basic where clauses.

@tomschlick

This comment has been minimized.

Copy link
Contributor

tomschlick commented Aug 30, 2016

Is this ready to go?

IMO the stuff above can be a seperate pull request... I just want the ability to use ES now that scout has been released for over a week.

@taylorotwell taylorotwell merged commit 4b02a3a into laravel:master Sep 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Sep 2, 2016

I've merged this and people can iterate on it from here. I'm not an Elastic search expert so have to just trust the community on this thing.

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Sep 2, 2016

thanks @taylorotwell
I believe people can improve it over time now

@mehi-shokri

This comment has been minimized.

Copy link

mehi-shokri commented Sep 24, 2016

@hernandev I'm sorry can this be used with elasticsearch 5.x cluster?

@jmarcher

This comment has been minimized.

Copy link
Contributor

jmarcher commented Sep 26, 2016

@mehrdaad no, as elasticsearch/elasticsearch 2.2 supports elasticsearch >= 2.0, < 5.0

@hernandev

This comment has been minimized.

Copy link
Author

hernandev commented Sep 26, 2016

@mehrdaad, as @jmarcher said, not currently, since there is no stable release of 5.0 yet. So the stable release of php driver for ElasticSearch don't support it

@mehi-shokri

This comment has been minimized.

Copy link

mehi-shokri commented Sep 26, 2016

@hernandev thanks. One more question, I know scout is mainly to submit a query and get results.
But what is your approach if you desire to get a more complicated query using DSL?
for example geosearch, fuzzy searching, etc.
Right now (although seems silly is that I get it done with elasticsearch php client and leave model syncing to scout.

@mehi-shokri

This comment has been minimized.

Copy link

mehi-shokri commented Oct 4, 2016

@hernandev I see in ElasticSearchEngine, in performSearch method, there is a filters argument. Would you please describe what it does? or provide some examples how it works?

@terion-name

This comment has been minimized.

Copy link

terion-name commented Apr 26, 2017

It was merged, but now is not present in the package. Why?

@jmarcher

This comment has been minimized.

Copy link
Contributor

jmarcher commented Apr 26, 2017

It is now only supported by external driver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment