I was trying to configure a search_results display and noticed that there was no snippet field to be found. After digging in DS code for a bit, I noticed that it "mimics" the node search execute and wasn't setting a snippet (like the original node_search_execute()).

(PS... Would it be possible to include the additional modules provided by DS in your issue components? Just a preference really :))

Thanks! Mark

CommentFileSizeAuthor
#11 1537634-ds_search-snippet-11.patch2.99 KBmarkhalliwell
#4 1537634-ds_search-snippet.patch2.52 KBmarkhalliwell
#2 1537634-ds_search-snippet.patch2.51 KBmarkhalliwell
#1 1537634-ds_search-snippet.patch2.5 KBmarkhalliwell

Comments

markhalliwell’s picture

markhalliwell
Primary language English
commented
StatusFileSize
new 1537634-ds_search-snippet.patch2.5 KB

Attaching patch.

markhalliwell’s picture

markhalliwell
Primary language English
commented
StatusFileSize
new 1537634-ds_search-snippet.patch2.51 KB

Attaching new patch. Changes "Snippet" to "Search snippet" in the field UI.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Active » Needs review
markhalliwell’s picture

markhalliwell
Primary language English
commented
StatusFileSize
new 1537634-ds_search-snippet.patch2.52 KB

Tonight isn't my night lol sorry for all the comments. Found one thing, the build of the node view should probably be "search_index". This will allow the appropriate gathering of content that's actually searchable.

swentel’s picture

swentel commented
Category: bug » feature
Priority: Critical » Normal
Status: Needs review » Needs work 
+++ b/modules/ds_search/ds_search.moduleundefined
@@ -453,6 +455,13 @@ function node_ds_search_execute($keys = NULL, $conditions = NULL) {
+    $build = node_view($node, 'search_index');

This should be search_result. Node modules does this as well.

There's a bigger second problem here though. We're actually going to call node_view twice now, because the view callback is again called in ds_search_search_page(). It should avoid that at all cost. Maybe we should pass along the $build and check for it in ds_search_search_page() to avoid the performance hit.

markhalliwell’s picture

markhalliwell
Primary language English
commented

This should be search_result. Node modules does this as well.

Node module also isn't customizing the search results display by adding a "Search snippet" field. I originally tried using search_result, but it only returned the title of the node (I only have the node title and search snippet for my search results display). So, I setup a search index display as well. This way the search results snippet (for DS) can use the search index display as it's source and create a valid snippet against fields that aren't shown on the search results display.

There's a bigger second problem here though. We're actually going to call node_view twice now, because the view callback is again called in ds_search_search_page(). It should avoid that at all cost. Maybe we should pass along the $build and check for it in ds_search_search_page() to avoid the performance hit.

I looked in ds_search_search_page(), but didn't see this mentioned callback in the function or any sub-functions. The only other node_view() that I see is for Apache Solr multisite support. Was this removed from the 7.x-2.x branch?

swentel’s picture

swentel commented

Hmm, that's actually a good thought, never saw it like that, so yeah, search_index would be valid there. However, it still leaves us two node_view() callbacks, which we really need now since we're rendering different view modes. In ds_search_search_page() you'll find ds_entity_view_fallback(). That function either uses entity or falls back to the original view function.

Now, personally, and at work, we never use core search anymore, however, there's people out there that probably do, so we really need to make sure those 2 node_view() callbacks aren't a big performance hit here.

markhalliwell’s picture

markhalliwell
Primary language English
commented

Seems to only be a 15ms (mean) increase, doesn't appear to be that bad? I'm only starting to get familiar with benchmarking.

Without calling node_view($node, 'search_index') and not displaying the original search snippet:

Concurrency Level:      1
Time taken for tests:   172.218 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Non-2xx responses:      500
Total transferred:      8689000 bytes
HTML transferred:       8456000 bytes
Requests per second:    2.90 [#/sec] (mean)
Time per request:       344.436 [ms] (mean)
Time per request:       344.436 [ms] (mean, across all concurrent requests)
Transfer rate:          49.27 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       25  282 130.3    332     758
Processing:     0   63 125.5      0     396
Waiting:        0   58 115.3      0     368
Total:        305  344  31.3    341     758

With calling node_view($node, 'search_index') and displaying the original node search snippet:

Concurrency Level:      1
Time taken for tests:   179.390 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Non-2xx responses:      500
Total transferred:      8689000 bytes
HTML transferred:       8456000 bytes
Requests per second:    2.79 [#/sec] (mean)
Time per request:       358.781 [ms] (mean)
Time per request:       358.781 [ms] (mean, across all concurrent requests)
Transfer rate:          47.30 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       25  277 143.9    347     852
Processing:     0   82 146.9      0     990
Waiting:        0   75 136.1      0     963
Total:        313  359  43.0    361    1021
swentel’s picture

swentel commented

That's indeed not that bad. Hmm ok, maybe just one thing: could we make search snippet for node search optional maybe ? With a checkbox on the ds search configuration screen in the node fieldset. I think it could use a description on how to actually configure then the view modes so you get the best results. Because, ironically, most people use DS to completely remove that default search result output, and I don't want them to get any overhead from the snippet calculation if they're not going to use the snippet field at all. If you could get that in the patch, I feel comfortable committing the patch :)

markhalliwell’s picture

markhalliwell
Primary language English
commented

Sweet. I think it'd be far better to determine whether or not the search snippet is actually being used. This could function as the "on/off" switch so it doesn't call the node_view() again. Let me dig around some more to come up with a solution.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1537634-ds_search-snippet-11.patch2.99 KB

I believe we have a winner! I was actually able to get the patch down to -1ms (basically no impact what so ever). I was also able to not render the snippet unless the field is actually present in the display.

Without patch:

Concurrency Level:      1
Time taken for tests:   173.156 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Non-2xx responses:      500
Total transferred:      8689000 bytes
HTML transferred:       8456000 bytes
Requests per second:    2.89 [#/sec] (mean)
Time per request:       346.312 [ms] (mean)
Time per request:       346.312 [ms] (mean, across all concurrent requests)
Transfer rate:          49.00 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       25  266 138.3    339     406
Processing:     0   80 138.3      0     377
Waiting:        0   73 127.1      0     350
Total:        307  346  18.6    347     407

With patch:

Concurrency Level:      1
Time taken for tests:   172.479 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Non-2xx responses:      500
Total transferred:      8689000 bytes
HTML transferred:       8456000 bytes
Requests per second:    2.90 [#/sec] (mean)
Time per request:       344.958 [ms] (mean)
Time per request:       344.958 [ms] (mean, across all concurrent requests)
Transfer rate:          49.20 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       25  269 136.5    325     607
Processing:     0   76 140.7      0    1098
Waiting:        0   70 130.1      0    1073
Total:        304  345  44.7    338    1123
swentel’s picture

swentel commented
Status: Needs review » Fixed

Looks good, committed and pushed, thanks!

Status: Fixed » Closed (fixed)

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

BarisW’s picture

BarisW commented

Just to make sure, does this only work for Apache SOLR? I don't see the field in Drupal core search.

markhalliwell’s picture

markhalliwell
Primary language English
commented

This patch actually adds the core search module's snippet and removes the conditional SOLR if clause so it will either use SOLR (if it is installed and configured properly) or the default core search snippet.

This patch is for the 7.x-2.x branch only. My guess is that if you're not seeing this field, then you are probably using the stable 7.x-1.x branch which only has support for the SOLR snippet. I don't believe this patch will make its way back to 1.x, at least not at the moment.

swentel’s picture

swentel commented
Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Closed (fixed) » Active

Will commit this to 7.x-1.x branch as well

swentel’s picture

swentel commented
Status: Active » Closed (fixed)

Just committed!

swentel’s picture

swentel commented
Issue summary: View changes

Updated issue.