Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nadavoid’s picture

drunken monkey’s picture

Since we now use common configurations for all Drupal Solr modules, the config changes would first have to be approved there.
And also, of course, we'd first need a viable patch in the Search API issue. But in principle it looks good.

drunken monkey’s picture

Status: Needs review » Needs work
thePanz’s picture

Patch attached (see related issue and comments here: #1197538-63: Random sort in Views).

Removed store="false" edits from schema.xml, the field is never used to STORE data: it is used as a "virtual field" just at query time.

drunken monkey’s picture

Here, too: thanks, good work! Apart from needing a feature, as said in the other issue, I see these minor style problems:

  1. +++ b/includes/service.inc
    @@ -851,6 +852,16 @@ class SearchApiSolrService extends SearchApiAbstractService {
    +      // SOLR provides a "virtual" query-time-only field named "random_SEED" that
    +      // can be used to sort randomly the results.
    

    Line length, capitalization ("Solr") and grammar. Also, our schema provides the field, not Solr itself.

  2. +++ b/includes/service.inc
    @@ -851,6 +852,16 @@ class SearchApiSolrService extends SearchApiAbstractService {
    +        $params = $query->getOption('search_api_random', array());
    +        // Random seed: getting the value from parameters or computing a new ome.
    +        $seed = !empty($params['seed']) ? $params['seed'] : rand(1, 200);
    

    I'd use mt_rand() and a higher maximum value.

drunken monkey’s picture

Version: 7.x-1.4 » 7.x-1.x-dev
thePanz’s picture

Updated patch.
Regarding the "feature" implementation, could you guide me to such implementation? (a few guidelines on IRC could be enough ) :)

thePanz’s picture

FileSize
1.58 KB

Adding the "Random sort" as a SearchAPI feature.

thePanz’s picture

Status: Needs work » Needs review
drunken monkey’s picture

Very good, thanks!
As soon as the other issue is resolved, I think we can commit this (after adapting it, if necessary, of course).

Just one question: Why do we want to keep the field name length below 10?

thePanz’s picture

Ok, thanks!

The "keep the field name under 10" is just to avoid the Solr query length to reach the maximum limit when multiple fields and parameters are used (I had some experiences of that in other projects, but not related to Drupal).

I know that it could be a rare case, but I also think that 1k different seeds (1-999) provides good randomness! :)

drunken monkey’s picture

Status: Needs review » Needs work

I know that it could be a rare case, but I also think that 1k different seeds (1-999) provides good randomness! :)

For most sites, surely. But for sites with, e.g., a million nodes, this will mean that some nodes will never be shown – and with enough visits, that might be a realistic problem.
Also, even with a much lower node count, too few different values means that some nodes will be shown much more than others – and when I, e.g., think of advertisements, that also might be a problem.

In conclusion, saving a few characters towards the total query maximum is no sufficient reason to be stingy here. Please use a higher value, or even a call without parameters (it seems that the number will then have at most ten digits, which is also no danger for the query length).

thePanz’s picture

Status: Needs work » Needs review
FileSize
1.47 KB

Sure, let's then use the mt_rand() without parameters to get the full-length random value.
New patch attached.

thePanz’s picture

FileSize
1.46 KB

New patch attached, as aligned with #1197538-80: Random sort in Views

joshtaylor’s picture

Very cool, thePanz - this adds random sorting!

Also nice that you can add something like:
$query->setOption('search_api_random_sort', ['seed' => 42]);

For the seed.

joshtaylor’s picture

Seems I had a crucial bit of code commented out, woops:

SearchApiException: Trying to sort on unknown field search_api_random. in SearchApiQuery->sort() (line 590 of search_api/includes/query.inc).
The website encountered an unexpected error. Please try again later.

drunken monkey’s picture

Status: Needs review » Fixed

@ joshtaylor: Did you also apply the patch #1197538-80: Random sort in Views to the Search API? For me, everything works fine.

Anyways, looks good now, thanks again!
Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

drunken monkey’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Closed (fixed) » Needs review
Related issues: +#2604874: Move query validation logic to backend plugins
FileSize
2.09 KB

Ported to Drupal 8, please test/review (in conjunction with #1197538-89: Random sort in Views)!

  • drunken monkey committed cb82a54 on 8.x-1.x
    Issue #2313591 by drunken monkey: Added support for random sorting.
    
drunken monkey’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.