Add result grouping support

Thanks for reporting this issue. Your patch seems to be simple and correct. But obviously we don't have any tests coverage for the grouping feature at the moment. Therefor I suggest to extend the patch with a test to fix that.

Patch applied and works as expected.

I can't apply the last patch. Can you review it, please?

I did a patch redoing the last modifications and removing an accidental "use" order at document begin. I don't know the correct way to create Test for this case :_( Is the first time that I use search API and Solr.

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -1365,7 +1369,12 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +      if(is_array($doc)) {
   

   Can you describe, when will $doc be an array?

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2448,6 +2457,7 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +    $solarium_query->getGrouping();
   

   What does this line trigger exactly?
   Adding a comment would be nice.

Really I don't know. I only have replicated the last patch and fixing a bug inside without analize the code.

I think that " if(is_array($doc)) {" line is not necesary and I don't undestand the getGrouping order. I'm going to wait to @stdio to explain it and if he doesn't answer I review the code and create a new patch.

@stdio Shit, no, this change is a typo :S

I was playing with the code, debugging and testing it and I suppose that I changed unintentionally before create the patch.

The change that I did intentionally is remove a duplicate use element at document begin (And the reroll because the patch for any reason can't be applied).

Guys, I really like to get the grouping stuff fixed and committed and really appreciate your work!

But since I'm nit 100% sure about the expected search results of such a grouping, I can't judge if the patch is correct.
But I'm convinced that a test won't be very difficult and fill my gap of understanding ;-)

Please have a look at SearchApiSolrTest::testSearchResultSorts(). This might be a good pattern.
The base class of the test allready contains 5 entities. This particular test adds 2 additional entities with extra long texts.
Then the test fires different queries with different sorts and checks the order of the entity IDs in the result set.
I assume that testing the grouping of entity IDs will be very close to this.

Ok, patch done fixing the typo. I think that the code run as expected. I'm using it in a real environment with real data and complex queries and it seems ok.

I will try to do the test but I can't promise that my test will be correct! :-)

I tried to create the test but I cannot run it. Thanks to your comment I could understand the code inside testSearchResultSorts and I think that I can developer another similar that it adding the group options and the asserts, but I need to run the tests before and it seems imposible.

The error it is:

1) Drupal\Tests\search_api_solr\Kernel\SearchApiSolrTest::testExtract
Drupal\search_api_solr\SearchApiSolrException: Solr endpoint http://localhost:8983/solr/d8/ not found.

I was searching on the code and I found the hidden module search_api_solr_test in which the d8 server is created but I can't install and I don't know the way to do it.

When I can execute the test that are already developed I can try to create a new one. Any idea? Or manual to read? Thanks!

@estoyausente: It seems that your Solr server isn't configured for http://localhost:8983/solr/d8/
The easiest way to get it running properly configured for the tests is to use docker-compose. The required config is included in the module. Have a look at the first part of https://youtu.be/NkLBTiRiQiI

I added a new method to test the grouping feature. I'm not sure what other assert can I do, can somebody review the test? I can try improve it but I don't know exactly the correct way.

Regards and thanks for helping.

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2637,8 +2646,9 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +      $type = $field->getType();      // Only single-valued fields are supported.
   
   +++ b/tests/src/Kernel/SearchApiSolrTest.php
   @@ -802,6 +802,32 @@ class SearchApiSolrTest extends BackendTestBase {
   +        'fields' => ['type']
   ...
   +      $data =  $results->getExtraData('search_api_solr_response');
   ...
   +      $this->assertEquals(2, $data['grouped']['ss_type']['ngroups'], 'Get the number of groups after groping.');
   ...
   +    } else {
   ...
   +  ¶
   

   some coding standard violations ;-)

2. +++ b/tests/src/Kernel/SearchApiSolrTest.php
   @@ -802,6 +802,32 @@ class SearchApiSolrTest extends BackendTestBase {
   +      $this->assertEquals(2, $data['grouped']['ss_type']['ngroups'], 'Get the number of groups after groping.');
   

   I'm missing a test that actually asserts a specific result. My current understanding of the grouping feature is, that there must be a resultset of 5 documents "ordered" by type.

Ok! thanks for reviewing. I will try to fix the code standard violations and add a new test checking if the resultset have the correct docs.

great. just something like

$this->assertResults([2, 4, 1, 5, 3], $results, 'Grouped by type descending.');

Hi!

I just added a new assert:

      $this->assertResults([1, 4], $results, 'Grouping by type');

The result set only have 2 elements because the grouping is correct (the system group the docs and return 1 element per group, the first in this case). You can get the different documents of each group ig you go to the group property of the result ($data['grouped'], you can see it in the patch where I check if the groups have the correct number of elements).

I fixed the coding standard error too. Mi code sniffer don't show any other error, please let me know if you find more coding standard problems.

The result set only have 2 elements because the grouping is correct (the system group the docs and return 1 element per group, the first in this case). You can get the different documents of each group ig you go to the group property of the result ($data['grouped'], you can see it in the patch where I check if the groups have the correct number of elements).

OK, I see. Now I see why I was that confused by this feature and commented it out for the first 8.x release.
But is this the best behavior?
AFAIK the feature isn't used so far as it is still commented out without this patch. So we still have the chance to improve it.
If I understand you guys correctly you're using custom code to access the grouped results, don't you?
And there's no support from Search API itself for grouping at the moment.
I don't like that you therefore have to deal with the raw result to fetch the grouped documents.

What's your opinion?
Do you have custom code that might be suitable to be integrated in the module?

OK, I see. Now I see why I was that confused by this feature and commented it out for the first 8.x release.
But is this the best behavior?

I'm not sure. It the first time that I use Solr (or any search engine system) and I thought that it is the usual behavior but really is... a little bit strange. In my use case this behavior is enough because I only need the number of element of each group but I assume that it is not the usual case.

AFAIK the feature isn't used so far as it is still commented out without this patch. So we still have the chance to improve it.
If I understand you guys correctly you're using custom code to access the grouped results, don't you?

Exactly, as I said, in my use case I don't have to get the documents inside the group because the group id and the element count is enough for me but yes, is a custom code. The information that I need is set as a extra data element in the result and i take it from there, something like this (this is a simplify code only as a example):

protected function getValuesFromGroup(\Drupal\search_api\Query\ResultSetInterface &$response, $field) {
    $values = [];
    $solrResponse = $response->getExtraData('search_api_solr_response');
    $matches = $solrResponse['grouped']['its_' . $field]['matches'];
    $groups = $solrResponse['grouped']['its_' . $field]['groups'];
    foreach($groups as $group) {
      $values[$group['groupValue']] = $matches;
    }
   return $values;
}

And there's no support from Search API itself for grouping at the moment.
I don't like that you therefore have to deal with the raw result to fetch the grouped documents.

Agree, it's a little bit dirty

What's your opinion?
Do you have custom code that might be suitable to be integrated in the module?

Right now I'm working directly in the result and getting from here the elements but maybe we can define a better solution and integrate it in search_api_solr. I can try to help but my experience with solr is very little. I'm started to work one month ago by first time. As you say, it's the best moment to think about how the groups are returned by solr. I was taking a look over how other backends implements this feature but I sadly didn't find anything.

I managed to get this working with a modification to the patch https://www.drupal.org/files/issues/Issue-2900410-by-stdio-estoyausente-... (see patch attached, applies to 8.x-1.1)

In a custom module, implement something like this:

/**
 * Implements hook_search_api_solr_query_alter().
 */
function MY_MODULE_search_api_solr_query_alter(\Solarium\Core\Query\QueryInterface $solarium_query, \Drupal\search_api\Query\QueryInterface $query) {
  // Alter the Solarium query.
  $solarium_query->addParam('group', TRUE);
  $solarium_query->addParam('group.field', 'its_nid');
  // Add grouping to the query options so the module knows to group the results.
  $grouping = [
    'use_grouping' => TRUE,
    'group_sort' => ['its_nid' => \Drupal\search_api\Query\QueryInterface::SORT_DESC],
    'fields' => ['its_nid'],
  ];
  $query->setOption('search_api_grouping', $grouping);
}

The attached patch only applies to 8.x-1.x (so you have my apologies) but I wanted to put it here to point out that since Solarium doesn't provide a `numFound` value for results grouped by field, it breaks pagers (for example, on Views). The previous patch I attached set the number of results as the visible number of groups, which is probably not an acceptable solution but is better than nothing.

This leads into my next point. Perhaps there is a discussion or a bit of thought needed regarding the invocation of the `search_api_solr_query` alter hook (NB, this should probably be an event subscriber in Drupal 8). If you're altering the query to group the results, then the necessary grouping parameters aren't set because the hook fires after the query is built. The pertinent part of the code is as follows:

if (!empty($grouping_options['use_grouping'])) {
  $this->setGrouping($solarium_query, $query, $grouping_options, $index_fields, $field_names);
}

If the aforementioned hook is implemented to set grouping, then `$grouping_options['use_grouping']` is not set because the invocation isn't set until further down. The attached patch is a workaround for this and the described pager issue with the following hook implementation:

/**
 * Implements hook_search_api_solr_query_alter().
 */
function my_module_search_api_solr_query_alter(SolariumQueryInterface $solarium_query, QueryInterface $query) {
    // Alter the Solarium query.
    $solarium_query->addParam('group', TRUE);
    $solarium_query->addParam('group.field', 'its_nid');

    // Add grouping to the query options so the module knows to group the results.
    $grouping = [
      'use_grouping' => TRUE,
      'group_sort' => ['its_nid' => $query::SORT_DESC],
      'fields' => ['its_nid'],
      'group.ngroups' => TRUE,
    ];

    // Set the groupings and add the ngroups param.
    $query->setOption('search_api_grouping', $grouping);
    $solarium_query->addParam('group.ngroups', TRUE);

  }

}

We should not work around solarium issues. Since I became the solarium 4.x we should improve that library first.
@jamiehollern: Can you open such an Issue for solarium?

@mkalkbrenner I'd be happy to do so, but I don't think it's a bug in Solarium. I think that's just how Solr is intended to work. If you look in the data from a raw Solr query that uses field grouping, the data for numFound isn't returned. So if it's not a bug, we need to make sure that instead the group.ngroups parameter is set at all times (unless overridden intentionally elsewhere) so that we can set the number of groups as the number of possible results.

Patch from #30 wasn't working for me - getting empty results. It worked when I translated the field name via

$field = $field_names[$field];

around line 1402 and 1416.

Here's the patch. Also changed " to ' and minor refactoring so $response is only used when it's defined.

It applied cleanly on drupal/search_api_solr:1.2

Excuse the previous patch, I was editing it manually and forgot to change the line numbers. Use this instead.

In attachment patch with search_api_solr 8.x-2.0.

But now my facets count is wrong
Someone who can help me?

/**
 * Implements hook_search_api_solr_query_alter().
 */
function hook_search_api_solr_query_alter(\Solarium\Core\Query\QueryInterface $solarium_query, \Drupal\search_api\Query\QueryInterface $query) {
  // Alter the Solarium query.
  $solarium_query->addParam('group', TRUE);
  $solarium_query->addParam('group.field', 'its_nid');

  // Add grouping to the query options so the module knows to group the results.
  $grouping = [
    'use_grouping' => TRUE,
    'group_sort' => ['its_nid' => $query::SORT_DESC],
    'fields' => ['nid'],
    'group.ngroups' => TRUE,
  ];

  // Set the groupings and add the ngroups param.
  $query->setOption('search_api_grouping', $grouping);
  $solarium_query->addParam('group.ngroups', TRUE);
}

Patch #36 works fine for me. Currently we're not using Facets for results count.

+++ b/tests/src/Kernel/SearchApiSolrTest.php
@@ -767,4 +767,34 @@ class SearchApiSolrTest extends SolrBackendTestBase {
+    if($this->solrAvailable) {
...
+    else {
+      $this->assertTrue(TRUE, 'Error: The Solr instance could not be found. Please enable a multi-core one on http://localhost:8983/solr/d8');
+    }

This check has to be removed now.

The latest patch didn't touch some erroneous of the non-active code.
I rewrote the patch and leveraged solarium properly. Please test it in order to get it committed soon!

I pretty sure that the setting of the grouping parameters is correct now. But I still have two concerns:

The wired grouping options are the ones defined by https://www.drupal.org/project/search_api_grouping . Have a look at the bottom of
https://cgit.drupalcode.org/search_api_grouping/tree/includes/processor_...

There's no activity to port this module to Drupal 8.x and I'm of the opinion that the options should be revised.

The second thing is the extraction of the result. I think we must return something that can be displayed by views and not just by custom code.

No Feedback?

Hi, I was in need of this and I successfully implemented the #39 patch on a project with commerce products and facets.
I also had to use the hook_search_api_solr_query_alter() to set the grouping in the query, that's why I agree with you that it would be great if it could be integrated with Views without using custom code

As a colleague to @jamiehollern, who provided the patch at post #30, I can confirm having upgraded from search_api_solr from 1.1 to 2.2, the patch at #39 is working as expected.

For anyone else upgrading from the 1.x branch (with the patch from #30) to the 2.x. branch (with the patch from #39), it's worth noting there is a small change in the solr query alter when comparing posts #30 and #36 that the 'fields' within the $grouping array needs to change from ['its_nid'] to ['nid']. Without this subtle change, the output is no results returned.

Reroll against 8.x-2.4.

Perfectly working for me using Views grouping field (unformatted list parameters) and the following custom code (here grouping by Content Type and limiting result set per 5 for each group, sorting per created date) :

use Drupal\search_api\Query\QueryInterface;
use Solarium\Core\Query\QueryInterface as SolariumQueryInterface;

/**
 * Implements hook_search_api_solr_query_alter().
 */
function drupal8_tests_search_api_solr_query_alter(SolariumQueryInterface $solarium_query, QueryInterface $query) {
  if ($query->getSearchId(FALSE) == 'views_page:recherche__page_1' ) {
    // Alter the Solarium query.
    $solarium_query->addParam('group', TRUE);
    $solarium_query->addParam('group.field', 'ss_type');
    $solarium_query->addParam('group.limit', 5);
    $solarium_query->addParam('group.sort', 'ds_created asc');

    // Add grouping to the query options so the module knows to group the results.
    $grouping = [
      'use_grouping' => TRUE,
      'fields' => ['type'],
      'group.ngroups' => TRUE,
    ];

    // Set the groupings and add the ngroups param.
    $query->setOption('search_api_grouping', $grouping);
    $solarium_query->addParam('group.ngroups', TRUE);
  }
}

As said above it will be awesome to have limit and sort available in views directly.

re-roll against 3.x

I committed the code to 3.x.
But there'll be todos for multilingual support we should handle in a separate issue.
And it would be nice if someone wants to start building native support for views.

Thanks for your help!