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

[6.0] Add a searchableWith property to eager load some relations on batch indexing (scout:import) #342

Closed
wants to merge 1 commit into from

Conversation

tarekbazine
Copy link

referring to this issues.

todo :

  • tests
  • update laravel scout docs (adding this property)

i really want to help for the tests but i did not know where to start, feedbacks will be appreciated.

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this proposal. I like the idea, but your implementation contains some issues.

Loading a relation using the with on the makeAllSearchable method changes the default result of the toSearchableArray method. As example, the result of the scout:import would be a record with:

array:10 [
  "id" => 57
  "user_id" => 40
  "image_url" => "https://lorempixel.com/640/480/?35139"
  "title" => "Configure models"
  "user" => array:6 [ // <--- here...
    "id" => 40
    "name" => "Rhett Kozey"
    "email" => "qhegmann@example.net"
    "email_verified_at" => "2019-01-07 17:21:14"
    "created_at" => "2019-01-07 17:21:15"
    "updated_at" => "2019-01-07 17:21:15"
  ]
]

Horever, the result of toSearchableArray using the normal auto indexing would be the same record without the user key:

array:10 [
  "id" => 57
  "user_id" => 40
  "image_url" => "https://lorempixel.com/640/480/?35139"
  "title" => "Configure models"
]

@nunomaduro
Copy link
Member

But let's wait until @taylorotwell or @driesvints jump on this.

@driesvints driesvints changed the title add a searchableWith property to eager load some relations on batch indexing (scout:import) [6.0] Add a searchableWith property to eager load some relations on batch indexing (scout:import) Jan 8, 2019
@driesvints
Copy link
Member

Hmm, think it's best that we hold off on this implementation. @nunomaduro can you jump in on the issue to figure out a solution first before we accept a PR? Thanks!

@driesvints driesvints closed this Jan 8, 2019
@fitztrev
Copy link
Contributor

@nunomaduro @driesvints - I don't understand the problem with this PR. What you describe is the behavior I would expect to happen.

class Post extends Model
{
    protected $with = ['user'];
}

class Post extends Model
{
    protected $searchableWith = ['user'];
}

The property $searchableWith behaves the same as $with, but allows you to load those relationships only in the context of indexing records. So if your toSearchableArray contains data from a relationship, you can eager load and make the scout:import command much faster and more efficient.

This PR fixes #329 and algolia/scout-extended#90 for me.

@ultrono
Copy link

ultrono commented Mar 5, 2020

@fitztrev I think @nunomaduro takes issue with the fact that unless the toSearchableArrray() method is defined on the model, the relationship is also indexed.

Using the code from this PR, to index 16,500 records, with a single relationship (a booking model, with a 'customer' relationship) massively speeds up indexing.

With eager loading the process takes 4 mins 24 seconds. WIthout eager loading the time increases 11 mins 09 seconds.

I'm using the TNTSearch driver.

EDIT 07/03/20:

I've just come across matchish/laravel-scout-elasticsearch#31 (comment), which suggests creating a separate model, with global scope applied (although I assume I could just populate "with" property on the model. I tend index records on my new model. This is farr from ideal, but without support to eager loads relationships there isn;t particularly any option I'm aware of.

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

Successfully merging this pull request may close these issues.

None yet

5 participants