Skip to content
Snippets Groups Projects
Unverified Commit d4f5fb56 authored by Tim Macdonald's avatar Tim Macdonald Committed by GitHub
Browse files

Search: Favor pinned items (#14944) 

* Refactor various bits of search

* Prioritize pinned items in search results

[Fixes #14936]
parent e12da299
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/search.clj
+ 1
− 2
View file @ d4f5fb56
......@@ -317,8 +317,7 @@
(filter some?))]
(->> results
(transduce xf scoring/accumulate-top-results)
;; Pluck out the result; discard the score
(map second)))))
(map :result)))))
;;; +----------------------------------------------------------------------------------------------------------------+
......
src/metabase/search/config.clj
+ 4
− 0
View file @ d4f5fb56
......@@ -29,6 +29,10 @@
class-or-instance
(class class-or-instance)))
(def ^:const displayed-columns
"All of the result components that by default are displayed by the frontend."
#{:name :display_name :collection_name})
(defmulti searchable-columns-for-model
"The columns that will be searched for the query."
{:arglists '([model])}
......
src/metabase/search/scoring.clj
+ 50
− 31
View file @ d4f5fb56
......@@ -75,7 +75,7 @@
:text (str/join " "
(map :text matches-or-misses-map))}))))
(defn- score-with
(defn- text-score-with
[scoring-fns query-tokens search-result]
(let [scores (for [column (search-config/searchable-columns-for-model (search-config/model-name->class (:model search-result)))
:let [matched-text (-> search-result
......@@ -86,12 +86,13 @@
match-tokens
scoring-fns))]
:when (> score 0)]
{:score score
:match matched-text
:match-context (match-context query-tokens match-tokens)
:column column})]
{:text-score score
:match matched-text
:match-context-thunk #(match-context query-tokens match-tokens)
:column column
:result search-result})]
(when (seq scores)
(apply max-key :score scores))))
(apply max-key :text-score scores))))
(defn- consecutivity-scorer
[query-tokens match-tokens]
......@@ -127,30 +128,52 @@
[(str/lower-case (name model)) i])
search-config/searchable-models)))
(defn- score-with-match
(defn- text-score-with-match
[query-string result]
(when (seq query-string)
(score-with match-based-scorers
(tokenize (normalize query-string))
result)))
(text-score-with match-based-scorers
(tokenize (normalize query-string))
result)))
(defn- pinned-score
[{pos :collection_position}]
(if (or (nil? pos)
(zero? pos))
0
(/ -1 pos)))
(defn- compare-score-and-result
"Compare [score result] pairs. Must return -1, 0, or 1. The score is assumed to be a vector, and will be compared in
order."
[[score-1 _result-1] [score-2 _result-2]]
"Compare maps of scores and results. Must return -1, 0, or 1. The score is assumed to be a vector, and will be
compared in order."
[{score-1 :score} {score-2 :score}]
(compare score-1 score-2))
(defn- serialize
"Massage the raw result from the DB into something more useful for the client"
[{:keys [collection_id collection_name name display_name] :as row} {:keys [score match column match-context] :as hit}]
(-> row
(assoc
:name (if (or (= column :name) (nil? display_name)) name display_name)
:matched_column column
:matched_text match
:context (when-not (#{:name :display_name :collection_name} column) match-context) ;; TODO pull this out somewhere more responsible
:score score
:collection {:id collection_id :name collection_name})))
"Massage the raw result from the DB and match data into something more useful for the client"
[{:keys [result column match-context-thunk]}]
(let [{:keys [name display_name
collection_id collection_name]} result]
(-> result
(assoc
:name (if (or (= column :name)
(nil? display_name))
name
display_name)
:context (when-not (search-config/displayed-columns column)
(match-context-thunk))
:collection {:id collection_id
:name collection_name})
(dissoc
:collection_id
:collection_name
:display_name))))
(defn- combined-score
[{:keys [text-score result]}]
[(pinned-score result)
(- text-score)
(model->sort-position (:model result))
(:name result)])
(defn accumulate-top-results
"Accumulator that saves the top n (defined by `search-config/max-filtered-results`) sent to it"
......@@ -172,12 +195,8 @@
(.offer item)))))
(defn score-and-result
"Returns a pair of [score, result] or nil. The score is a vector of comparable things in priority order. The result
has `:matched_column` and `matched_text` injected in"
"Returns a map with the `:score` and `:result`—or nil. The score is a vector of comparable things in priority order."
[query-string result]
(let [hit (score-with-match query-string result)]
(and hit
[[(- (:score hit))
(model->sort-position (:model result))
(:name result)],
(serialize result hit)])))
(when-let [hit (text-score-with-match query-string result)]
{:score (combined-score hit)
:result (serialize hit)}))
test/metabase/api/search_test.clj
+ 15
− 26
View file @ d4f5fb56
......@@ -15,20 +15,17 @@
(def ^:private default-search-row
{:id true
:description nil
:display_name nil
:archived false
:collection_id false
:collection {:id false :name nil}
:collection_position nil
:collection_name nil
:context nil
:favorite nil
:table_id false
:database_id false
:dataset_query nil
:table_schema nil
:table_name nil
:table_description nil
:matched_column "display_name"
:matched_text nil})
:table_description nil})
(defn- table-search-results
"Segments and Metrics come back with information about their Tables as of 0.33.0. The `model-defaults` for Segment and
......@@ -46,11 +43,11 @@
(letfn [(make-result [name & kvs]
(merge
default-search-row
{:name name :matched_text name :matched_column "name"}
{:name name}
(apply array-map kvs)))]
(sorted-results
[(make-result "dashboard test dashboard", :model "dashboard", :favorite false)
(make-result "collection test collection", :model "collection", :collection_id true, :collection_name true)
(make-result "collection test collection", :model "collection", :collection {:id true, :name true})
(make-result "card test card", :model "card", :favorite false, :dataset_query "{}")
(make-result "pulse test pulse", :model "pulse", :archived nil)
(merge
......@@ -68,10 +65,6 @@
:when (false? (:archived result))]
(assoc result :archived true)))
(defn- update-matched-text
[result]
(assoc result :matched_text (:name result)))
(defn- on-search-types [model-set f coll]
(for [search-item coll]
(if (contains? model-set (:model search-item))
......@@ -80,7 +73,7 @@
(defn- default-results-with-collection []
(on-search-types #{"dashboard" "pulse" "card"}
#(assoc % :collection_id true, :collection_name true)
#(assoc % :collection {:id true, :name true})
(default-search-results)))
(defn- do-with-search-items [search-string in-root-collection? f]
......@@ -129,8 +122,7 @@
:when (contains? #{keep-database-id nil} (:database_id result))]
(-> result
mt/boolean-ids-and-timestamps
(update :collection_name #(some-> % string?))
(dissoc :score :context :collection))))))))
(update-in [:collection :name] #(some-> % string?)))))))))
(deftest basic-test
(testing "Basic search, should find 1 of each entity type, all items in the root collection"
......@@ -176,8 +168,8 @@
(into
(default-results-with-collection)
(map #(merge default-search-row % (table-search-results))
[{:name "metric test2 metric", :matched_text "metric test2 metric", :description "Lookin' for a blueberry", :model "metric", :matched_column "name"}
{:name "segment test2 segment", :matched_text "segment test2 segment", :description "Lookin' for a blueberry", :model "segment", :matched_column "name"}])))
[{:name "metric test2 metric", :description "Lookin' for a blueberry", :model "metric"}
{:name "segment test2 segment", :description "Lookin' for a blueberry", :model "segment"}])))
(search-request :rasta :q "test"))))))))
(testing (str "Users with root collection permissions should be able to search root collection data long with "
......@@ -192,10 +184,9 @@
(is (= (sorted-results
(into
(default-results-with-collection)
(for [row (map update-matched-text (default-search-results))
(for [row (default-search-results)
:when (not= "collection" (:model row))]
(update-matched-text
(update row :name #(str/replace % "test" "test2"))))))
(update row :name #(str/replace % "test" "test2")))))
(search-request :rasta :q "test"))))))))
(testing "Users with access to multiple collections should see results from all collections they have access to"
......@@ -208,7 +199,7 @@
(is (= (sorted-results
(into
(default-results-with-collection)
(map (fn [row] (update-matched-text (update row :name #(str/replace % "test" "test2"))))
(map (fn [row] (update row :name #(str/replace % "test" "test2")))
(default-results-with-collection))))
(search-request :rasta :q "test")))))))
......@@ -223,8 +214,8 @@
(into
(default-results-with-collection)
(map #(merge default-search-row % (table-search-results))
[{:name "metric test2 metric", :matched_text "metric test2 metric", :description "Lookin' for a blueberry", :model "metric", :matched_column "name"}
{:name "segment test2 segment", :matched_text "segment test2 segment", :description "Lookin' for a blueberry", :model "segment", :matched_column "name"}])))
[{:name "metric test2 metric", :description "Lookin' for a blueberry", :model "metric"}
{:name "segment test2 segment", :description "Lookin' for a blueberry", :model "segment"}])))
(search-request :rasta :q "test"))))))))
(testing "Metrics on tables for which the user does not have access to should not show up in results"
......@@ -309,9 +300,7 @@
(merge
default-search-row
{:name table-name
:display_name table-name
:table_name table-name
:matched_text table-name
:table_id true
:archived nil
:model "table"
......@@ -333,7 +322,7 @@
(let [lancelot "Lancelot's Favorite Furniture"]
(mt/with-temp Table [table {:name "Round Table" :display_name lancelot}]
(do-test-users [user [:crowberto :rasta]]
(is (= [(assoc (default-table-search-row "Round Table") :display_name lancelot :matched_text lancelot :name lancelot)]
(is (= [(assoc (default-table-search-row "Round Table") :name lancelot)]
(search-request user :q "Lancelot")))))))
(testing "When searching with ?archived=true, normal Tables should not show up in the results"
(let [table-name (mt/random-name)]
......
test/metabase/search/scoring_test.clj
+ 23
− 4
View file @ d4f5fb56
......@@ -25,8 +25,8 @@
(defn scorer->score
[scorer]
(comp :score
(partial #'search/score-with [scorer])))
(comp :text-score
(partial #'search/text-score-with [scorer])))
(deftest consecutivity-scorer-test
(let [score (scorer->score #'search/consecutivity-scorer)]
......@@ -106,11 +106,17 @@
(deftest accumulate-top-results-test
(let [xf (map identity)]
(testing "a non-full queue behaves normally"
(let [items (map (fn [i] [[2 2 i] (str "item " i)]) (range 10))]
(let [items (map (fn [i]
{:score [2 2 i]
:result (str "item " i)})
(range 10))]
(is (= items
(transduce xf search/accumulate-top-results items)))))
(testing "a full queue only saves the top items"
(let [sorted-items (map (fn [i] [[1 2 3 i] (str "item " i)]) (range (+ 10 search-config/max-filtered-results)))]
(let [sorted-items (map (fn [i]
{:score [1 2 3 i]
:result (str "item " i)})
(range (+ 10 search-config/max-filtered-results)))]
(is (= (drop 10 sorted-items)
(transduce xf search/accumulate-top-results (shuffle sorted-items))))))))
......@@ -152,3 +158,16 @@
this social bird lives in small flocks in lowland rainforests in countries such as costa rica
it flies short distances between trees toucans rest in holes in trees
here is some more filler))))))))
(deftest pinned-score-test
(let [score #'search/pinned-score
item (fn [collection-position] {:collection_position collection-position})]
(testing "it provides a sortable score"
(is (= [1 2 3 0 nil]
(->> [(item 0)
(item nil)
(item 3)
(item 1)
(item 2)]
(sort-by score)
(map :collection_position)))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment