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

Improve search performance #1339

Merged
merged 12 commits into from
Feb 7, 2018
8 changes: 3 additions & 5 deletions src/Api/Controller/ListDiscussionsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ class ListDiscussionsController extends AbstractListController
public $include = [
'startUser',
'lastUser',
'relevantPosts',
'relevantPosts.discussion',
'relevantPosts.user'
'mostRelevantPost'
];

/**
Expand Down Expand Up @@ -84,7 +82,7 @@ protected function data(ServerRequestInterface $request, Document $document)
$offset = $this->extractOffset($request);
$load = array_merge($this->extractInclude($request), ['state']);

$results = $this->searcher->search($criteria, $limit, $offset, $load);
$results = $this->searcher->search($criteria, $limit, $offset);

$document->addPaginationLinks(
$this->url->to('api')->route('discussions.index'),
Expand All @@ -94,7 +92,7 @@ protected function data(ServerRequestInterface $request, Document $document)
$results->areMoreResults() ? null : 0
);

$results = $results->getResults();
$results = $results->getResults()->load($load);

if ($relations = array_intersect($load, ['startPost', 'lastPost'])) {
foreach ($results as $discussion) {
Expand Down
4 changes: 2 additions & 2 deletions src/Api/Serializer/BasicDiscussionSerializer.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ protected function posts($discussion)
/**
* @return \Tobscure\JsonApi\Relationship
*/
protected function relevantPosts($discussion)
protected function mostRelevantPost($discussion)
{
return $this->hasMany($discussion, BasicPostSerializer::class);
return $this->hasOne($discussion, PostSerializer::class);
}

/**
Expand Down
10 changes: 10 additions & 0 deletions src/Discussion/Discussion.php
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,16 @@ public function lastUser()
return $this->belongsTo('Flarum\User\User', 'last_user_id');
}

/**
* Define the relationship with the discussion's most relevant post.
*
* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
*/
public function mostRelevantPost()
{
return $this->belongsTo('Flarum\Post\Post', 'most_relevant_post_id');
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will only be used when searching, right? Because most_relevant_post_id will just be the name of a dynamically calculated column? If so, that should be made clear in the docblock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point.

}

/**
* Define the relationship with the discussion's readers.
*
Expand Down
71 changes: 12 additions & 59 deletions src/Discussion/Search/DiscussionSearcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,20 @@

namespace Flarum\Discussion\Search;

use Flarum\Discussion\Discussion;
use Flarum\Discussion\DiscussionRepository;
use Flarum\Discussion\Event\Searching;
use Flarum\Post\PostRepository;
use Flarum\Search\ApplySearchParametersTrait;
use Flarum\Search\GambitManager;
use Flarum\Search\SearchCriteria;
use Flarum\Search\SearchResults;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Contracts\Events\Dispatcher;

/**
* Takes a DiscussionSearchCriteria object, performs a search using gambits,
* and spits out a DiscussionSearchResults object.
*/
class DiscussionSearcher
{
use ApplySearchParametersTrait;

/**
* @var \Flarum\Search\GambitManager
* @var GambitManager
*/
protected $gambits;

Expand All @@ -40,37 +34,33 @@ class DiscussionSearcher
protected $discussions;

/**
* @var PostRepository
* @var Dispatcher
*/
protected $posts;
protected $events;

/**
* @param \Flarum\Search\GambitManager $gambits
* @param GambitManager $gambits
* @param DiscussionRepository $discussions
* @param PostRepository $posts
* @param Dispatcher $events
*/
public function __construct(
GambitManager $gambits,
DiscussionRepository $discussions,
PostRepository $posts
) {
public function __construct(GambitManager $gambits, DiscussionRepository $discussions, Dispatcher $events) {
$this->gambits = $gambits;
$this->discussions = $discussions;
$this->posts = $posts;
$this->events = $events;
}

/**
* @param SearchCriteria $criteria
* @param int|null $limit
* @param int $offset
* @param array $load An array of relationships to load on the results.
*
* @return SearchResults
*/
public function search(SearchCriteria $criteria, $limit = null, $offset = 0, array $load = [])
public function search(SearchCriteria $criteria, $limit = null, $offset = 0)
{
$actor = $criteria->actor;

$query = $this->discussions->query()->whereVisibleTo($actor);
$query = $this->discussions->query()->select('discussions.*')->whereVisibleTo($actor);

// Construct an object which represents this search for discussions.
// Apply gambits to it, sort, and paging criteria. Also give extensions
Expand All @@ -82,8 +72,7 @@ public function search(SearchCriteria $criteria, $limit = null, $offset = 0, arr
$this->applyOffset($search, $offset);
$this->applyLimit($search, $limit + 1);

// TODO: inject dispatcher
event(new Searching($search, $criteria));
$this->events->dispatch(new Searching($search, $criteria));

// Execute the search query and retrieve the results. We get one more
// results than the user asked for, so that we can say if there are more
Expand All @@ -96,42 +85,6 @@ public function search(SearchCriteria $criteria, $limit = null, $offset = 0, arr
$discussions->pop();
}

// The relevant posts relationship isn't a typical Eloquent
// relationship; rather, we need to extract that information from our
// search object. We will delegate that task and prevent Eloquent
// from trying to load it.
if (in_array('relevantPosts', $load)) {
$this->loadRelevantPosts($discussions, $search);

$load = array_diff($load, ['relevantPosts', 'relevantPosts.discussion', 'relevantPosts.user']);
}

Discussion::setStateUser($actor);
$discussions->load($load);

return new SearchResults($discussions, $areMoreResults);
}

/**
* Load relevant posts onto each discussion using information from the
* search.
*
* @param Collection $discussions
* @param DiscussionSearch $search
*/
protected function loadRelevantPosts(Collection $discussions, DiscussionSearch $search)
{
$postIds = [];
foreach ($search->getRelevantPostIds() as $relevantPostIds) {
$postIds = array_merge($postIds, array_slice($relevantPostIds, 0, 2));
}

$posts = $postIds ? $this->posts->findByIds($postIds, $search->getActor())->load('user')->all() : [];

foreach ($discussions as $discussion) {
$discussion->relevantPosts = array_filter($posts, function ($post) use ($discussion) {
return $post->discussion_id == $discussion->id;
});
}
}
}
24 changes: 0 additions & 24 deletions src/Discussion/Search/Fulltext/DriverInterface.php

This file was deleted.

36 changes: 0 additions & 36 deletions src/Discussion/Search/Fulltext/MySqlFulltextDriver.php

This file was deleted.

40 changes: 17 additions & 23 deletions src/Discussion/Search/Gambit/FulltextGambit.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,12 @@
namespace Flarum\Discussion\Search\Gambit;

use Flarum\Discussion\Search\DiscussionSearch;
use Flarum\Discussion\Search\Fulltext\DriverInterface;
use Flarum\Search\AbstractSearch;
use Flarum\Search\GambitInterface;
use LogicException;

class FulltextGambit implements GambitInterface
{
/**
* @var \Flarum\Discussion\Search\Fulltext\DriverInterface
*/
protected $fulltext;

/**
* @param \Flarum\Discussion\Search\Fulltext\DriverInterface $fulltext
*/
public function __construct(DriverInterface $fulltext)
{
$this->fulltext = $fulltext;
}

/**
* {@inheritdoc}
*/
Expand All @@ -41,14 +27,22 @@ public function apply(AbstractSearch $search, $bit)
throw new LogicException('This gambit can only be applied on a DiscussionSearch');
}

$relevantPostIds = $this->fulltext->match($bit);

$discussionIds = array_keys($relevantPostIds);

$search->setRelevantPostIds($relevantPostIds);

$search->getQuery()->whereIn('id', $discussionIds);

$search->setDefaultSort(['id' => $discussionIds]);
// TODO: add a migration for fulltext index on discussions.title
$search->getQuery()
->selectRaw('SUBSTRING_INDEX(GROUP_CONCAT(posts.id ORDER BY MATCH(posts.content) AGAINST (?) DESC), \',\', 1) as most_relevant_post_id', [$bit])
->join('posts', function ($join) use ($bit) {
$join->on('posts.discussion_id', '=', 'discussions.id')
->whereRaw('MATCH(posts.content) AGAINST (? IN BOOLEAN MODE)', [$bit]);

// TODO: need to scope post visibility
})
->whereRaw('MATCH(discussions.title) AGAINST (? IN BOOLEAN MODE)', [$bit])
->orWhereNotNull('posts.id')
->groupBy('discussions.id');

$search->setDefaultSort(function ($query) use ($bit) {
$query->orderByRaw('MATCH(discussions.title) AGAINST (?) desc', [$bit]);
$query->orderByRaw('SUM(MATCH(posts.content) AGAINST (?)) desc', [$bit]);
});
}
}
4 changes: 2 additions & 2 deletions src/Search/AbstractSearch.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ public function getDefaultSort()
* Set the default sort order for the search. This will only be applied if
* a sort order has not been specified in the search criteria.
*
* @param array $defaultSort An array of sort-order pairs, where the column
* @param mixed $defaultSort An array of sort-order pairs, where the column
* is the key, and the order is the value. The order may be 'asc',
* 'desc', or an array of IDs to order by.
* @return mixed
*/
public function setDefaultSort(array $defaultSort)
public function setDefaultSort($defaultSort)
{
$this->defaultSort = $defaultSort;
}
Expand Down
16 changes: 10 additions & 6 deletions src/Search/ApplySearchParametersTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,17 @@ protected function applySort(AbstractSearch $search, array $sort = null)
{
$sort = $sort ?: $search->getDefaultSort();

foreach ($sort as $field => $order) {
if (is_array($order)) {
foreach ($order as $value) {
$search->getQuery()->orderByRaw(snake_case($field).' != ?', [$value]);
if (is_callable($sort)) {
$sort($search->getQuery());
} else {
foreach ($sort as $field => $order) {
if (is_array($order)) {
foreach ($order as $value) {
$search->getQuery()->orderByRaw(snake_case($field).' != ?', [$value]);
}
} else {
$search->getQuery()->orderBy(snake_case($field), $order);
}
} else {
$search->getQuery()->orderBy(snake_case($field), $order);
}
}
}
Expand Down