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

Meilisearch return inconsistent results compared to database/algolia driver when using take() + query() + paginate() #742

Closed
crynobone opened this issue May 30, 2023 · 8 comments · Fixed by #743

Comments

@crynobone
Copy link
Member

crynobone commented May 30, 2023

Scout Version

10.2.2

Scout Driver

Meilisearch

Laravel Version

10.12.0

PHP Version

8.2.6

Database Driver & Version

MySQL 8.0

SDK Version

1.1.1

Meilisearch CLI Version

No response

Description

Given the following use-case:

$queryCallback = fn ($query) => $query->where('created_at', '<', now()->subDays(1));

App\Models\User::search('example')->query($queryCallback)->take(200)->paginate(25);

It should search for "example" using Scout driver and take only 200 sample size, and from that 200 sample size query using Eloquent with only records created before 24 hours ago and earlier, paginate the result using 25 records per page.

Steps To Reproduce

You can refer to the reproducing repository: nova-issues/issue-5574@b666ea2

  1. Run composer install
  2. Run cp .env.example .env
  3. Run php artisan key:generate
  4. Update Algolia credential on .env file.
  5. Run ./vendor/bin/sail up -d
  6. Run ./vendor/bin/sail artisan migrate:fresh --seed
  7. Run ./vendor/bin/sail artisan scout:import App\\Models\\UserWithAlgolia
  8. Run ./vendor/bin/sail artisan scout:import App\\Models\\UserWithMeilisearch
  9. Access / route from the browser.

image

@driesvints
Copy link
Member

There are several issues about this like #725 but I think the gist is that you can't use the take like this with the Meilisearch driver. I'm gonna page in @mmachatschek to see if he can provide a more detailed answer.

Also: are you sure you've rebuilt your search index after upgrade to Scout Meilisearch v1? Since some settings now use different keys.

I tried to send in a PR to the docs about this but probably badly phrased it: laravel/docs#8704

@crynobone
Copy link
Member Author

crynobone commented May 30, 2023

If that's the case then it not possible to use Scout + Meilisearch on Laravel Nova, it require this combination to work correctly.

are you sure you've rebuilt your search index after upgrade to Scout Meilisearch v1? Since some settings now use different keys.

The reproducing repository was only generated today and all data imported to scout drivers should be using the latest index.

@mmachatschek
Copy link
Contributor

mmachatschek commented May 30, 2023

@crynobone @driesvints

with the query example, on meilisearch all results have the same relevancy, therefore the default meilisearch sorting will take effect. in this case. all results are ordered asc to the id (I guess this is due to the nature of adding documents to the index in ascending order)

Algolia returns the results with a different order. this is why you seemingly get a result with algolia but not with meilisearch:

Before adding a hard order on algolia:

image

after adding a sorting to algolia:

image
image

so from my point of view, the behaviour is kind of the same from the implementation side but the default relevancy and sorting of the data is different between the engines, using the same sorting mechanism would lead to the same results

@crynobone
Copy link
Member Author

with the query example, on meilisearch all results have the same relevancy, therefore the default meilisearch sorting will take effect. in this case. all results are ordered asc to the id (I guess this is due to the nature of adding documents to the index in ascending order)

But still, the total items in the index is only 26 records, and using ->take(200) should already account for this.

Algolia returns the results with a different order. this is why you seemingly get a result with algolia but not with meilisearch

I increased the total record to 200 and algolia still able to get the correct record:

image

image

@mmachatschek
Copy link
Contributor

@crynobone still this happens because Algolia returns a different order of results than meilisearch, thats why you have a difference in the search results you are seeing.

If we would flip the seeder on your example repo, so that the Test User is created prior to the other 199 items you would get results with meilisearch as well.

But still, the total items in the index is only 26 records, and using ->take(200) should already account for this.

The take() in this case doesn't really do anything because ->paginate() is used and therefore only 25 items are loaded from the search engine

the ->query will be applied after the results are returned by the search engine. If you want to prefilter the results. you would need to do ->search('example', fn() => {// customize the query to the engine}) on the engine side.

This is also stated on the docs page https://laravel.com/docs/10.x/scout#:~:text=Since%20this%20callback,for%20%22filtering%22%20results

A solution to your problem could be:

  1. loading all items from the search engine User::search('example')->take(10000)->query(xxx)->get()
  2. transform the items into a eloquent query
  3. paginate the eloquent query

@crynobone
Copy link
Member Author

The take() in this case doesn't really do anything because ->paginate() is used and therefore only 25 items are loaded from the search engine

In Algolia (and the way we made the Laravel Nova + Scout + Algolia) integration before are:

App\Models\User::search('example')->query($queryCallback)->take(200)->paginate(25);

  1. Search from the index with the keyword "example" with a hard limit of 200 records.
  2. Get all the IDs from index found from step 1 as key
    public function map(Builder $builder, $results, $model)
    {
    if (count($results['hits']) === 0) {
    return $model->newCollection();
    }
    $objectIds = collect($results['hits'])->pluck('objectID')->values()->all();
    $objectIdPositions = array_flip($objectIds);
    return $model->getScoutModelsByIds(
    $builder, $objectIds
    )->filter(function ($model) use ($objectIds) {
    return in_array($model->getScoutKey(), $objectIds);
    })->sortBy(function ($model) use ($objectIdPositions) {
    return $objectIdPositions[$model->getScoutKey()];
    })->values();
    }
  3. Use eloquent to generate a new query with whereKey() matching the IDs from index and apply $queryCallback

    scout/src/Searchable.php

    Lines 247 to 263 in 9f64e8e

    public function queryScoutModelsByIds(Builder $builder, array $ids)
    {
    $query = static::usesSoftDelete()
    ? $this->withTrashed() : $this->newQuery();
    if ($builder->queryCallback) {
    call_user_func($builder->queryCallback, $query);
    }
    $whereIn = in_array($this->getKeyType(), ['int', 'integer']) ?
    'whereIntegerInRaw' :
    'whereIn';
    return $query->{$whereIn}(
    $this->qualifyColumn($this->getScoutKeyName()), $ids
    );
    }
  4. Then make pagination from step 3

    scout/src/Builder.php

    Lines 405 to 407 in 9f64e8e

    $results = $this->model->newCollection($engine->map(
    $this, $rawResults = $engine->paginate($this, $perPage, $page), $this->model
    )->all());

@simon-roland
Copy link

  1. Search from the index with the keyword "example" with a hard limit of 200 records.

@crynobone I think this is where the problem occurs. Are you sure that this search is not paginated before the filter is applied?

It does not seem like this issue is limited to Meilisearch, the order of results from the engine used determines the result.

Its like I said in laravel/nova-issues#5574

It seems, that if the filter does not include any of the results from the first page, then no results are shown.

@driesvints
Copy link
Member

Closing this as this is an implementation issue in Nova.

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

Successfully merging a pull request may close this issue.

4 participants