Global search: optimize -- ~20% improvement (#34353)

dev/src/dev.clj

@@ -39,6 +39,7 @@
    [clojure.string :as str]
    [dev.debug-qp :as debug-qp]
    [dev.model-tracking :as model-tracking]
+   [dev.explain :as dev.explain]
    [honeysql.core :as hsql]
    [malli.dev :as malli-dev]
    [metabase.api.common :as api]
@@ -79,6 +80,8 @@
  [debug-qp
   process-query-debug
   pprint-sql]
+ [dev.explain
+  explain-query]
  [model-tracking
   track!
   untrack!

dev/src/dev/explain.clj

+(ns dev.explain
+  (:require
+   [clojure.string :as str]
+   [honey.sql :as sql]
+   [toucan2.core :as t2]))
+
+(defn explain-query
+  "Explain a sql query or a honeysql query with option to analyze the query."
+  ([queryable]
+   (explain-query queryable false))
+  ([queryable analyze?]
+   (->> (t2/query
+         (str/join
+          " "
+          (remove nil? ["EXPLAIN"
+                        (when analyze? "ANALYZE")
+                        "(" (if (map? queryable) (first (sql/format queryable {:inline true})) queryable) ")"])))
+        (map #(get % (keyword "query plan"))))))

src/metabase/api/search.clj

@@ -139,7 +139,7 @@
     query))
 
 (mu/defn ^:private replace-select :- :map
-  "Replace a select from query that has alias is `target-alias` with the `with` column, throw an error if
+  "Replace a select from query that has alias is `target-alias` with [`with` `target-alias`] column, throw an error if
   can't find the target select.
 
   This works with the assumption that `query` contains a list of select from [[select-clause-for-model]],
@@ -148,15 +148,16 @@
   This function then will replace the dummy column with alias is `target-alias` with the `with` column."
   [query        :- :map
    target-alias :- :keyword
-   with         :- [:or :keyword [:sequential :any]]]
-  (let [selects (:select query)
-        idx     (first (keep-indexed (fn [index item]
-                                       (when (and (coll? item)
-                                                  (= (last item) target-alias))
-                                         index))
-                                     selects))]
+   with         :- :keyword]
+  (let [selects     (:select query)
+        idx         (first (keep-indexed (fn [index item]
+                                           (when (and (coll? item)
+                                                      (= (last item) target-alias))
+                                             index))
+                                         selects))
+        with-select [with target-alias]]
     (if (some? idx)
-      (assoc query :select (m/replace-nth idx with selects))
+      (assoc query :select (m/replace-nth idx with-select selects))
       (throw (ex-info "Failed to replace selector" {:status-code  400
                                                     :target-alias target-alias
                                                     :with         with})))))
@@ -165,13 +166,24 @@
   [query :- :map
    model :- [:enum "card" "dataset" "dashboard" "metric"]]
   (-> query
-       (replace-select :last_editor_id [:r.user_id :last_editor_id])
-       (replace-select :last_edited_at [:r.timestamp :last_edited_at])
+       (replace-select :last_editor_id :r.user_id)
+       (replace-select :last_edited_at :r.timestamp)
        (sql.helpers/left-join [:revision :r]
                               [:and [:= :r.model_id (search.config/column-with-model-alias model :id)]
                                [:= :r.most_recent true]
                                [:= :r.model (search.config/search-model->revision-model model)]])))
 
+(mu/defn ^:private with-moderated-status :- :map
+  [query :- :map
+   model :- [:enum "card" "dataset"]]
+  (-> query
+      (replace-select :moderated_status :mr.status)
+      (sql.helpers/left-join [:moderation_review :mr]
+                             [:and
+                              [:= :mr.moderated_item_type "card"]
+                              [:= :mr.moderated_item_id (search.config/column-with-model-alias model :id)]
+                              [:= :mr.most_recent true]])))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                      Search Queries for each Toucan Model                                      |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -191,7 +203,8 @@
                               [:= :bookmark.user_id api/*current-user-id*]])
       (add-collection-join-and-where-clauses :card.collection_id search-ctx)
       (add-card-db-id-clause (:table-db-id search-ctx))
-      (with-last-editing-info model)))
+      (with-last-editing-info model)
+      (with-moderated-status model)))
 
 (defmethod search-query-for-model "action"
   [model search-ctx]

src/metabase/search/config.clj

@@ -245,7 +245,8 @@
    [:table.description :table_description]])
 
 (defmulti columns-for-model
-  "The columns that will be returned by the query for `model`, excluding `:model`, which is added automatically."
+  "The columns that will be returned by the query for `model`, excluding `:model`, which is added automatically.
+  This is not guaranteed to be the final list of columns, new columns can be added by calling [[api.search/replace-select]]"
   {:arglists '([model])}
   (fn [model] model))
 
@@ -264,17 +265,6 @@
   (conj default-columns :collection_id :collection_position :dataset_query :creator_id
         [:collection.name :collection_name]
         [:collection.authority_level :collection_authority_level]
-        [{:select   [:status]
-          :from     [:moderation_review]
-          :where    [:and
-                     [:= :moderated_item_type "card"]
-                     [:= :moderated_item_id :card.id]
-                     [:= :most_recent true]]
-          ;; order by and limit just in case a bug violates the invariant of only one most_recent. We don't want to
-          ;; error in this query
-          :order-by [[:id :desc]]
-          :limit    1}
-         :moderated_status]
         bookmark-col dashboardcard-count-col))
 
 (defmethod columns-for-model "indexed-entity" [_]

src/metabase/search/filter.clj

@@ -189,6 +189,13 @@
   [query join-type table]
   (->> (get query join-type) (partition 2) (map first) (some #(= % table)) boolean))
 
+(defn search-model->revision-model
+  "Return the apporpriate revision model given a search model."
+  [model]
+  (case model
+    "dataset" (recur "card")
+    (str/capitalize model)))
+
 (doseq [model ["dashboard" "card" "dataset" "metric"]]
   (defmethod build-optional-filter-query [:last-edited-by model]
     [_filter model query editor-ids]

test/metabase/api/search_test.clj

@@ -13,6 +13,7 @@
             PermissionsGroupMembership Pulse PulseCard QueryAction Segment Table]]
    [metabase.models.collection :as collection]
    [metabase.models.model-index :as model-index]
+   [metabase.models.moderation-review :as moderation-review]
    [metabase.models.permissions :as perms]
    [metabase.models.permissions-group :as perms-group]
    [metabase.models.revision :as revision]
@@ -371,6 +372,22 @@
       (is (= dashboard-count-results
              (set (unsorted-search-request-data :rasta :q "dashboard-count")))))))
 
+(deftest moderated-status-test
+  (let [search-term "moderated-status-test"]
+    (mt/with-temp [:model/Card {card-id :id} {:name "moderated-status-test"}]
+      ;; an item could have multiple moderation-review, and it's current status is defined as
+      ;; moderation-review.most_recent, so we creates multiple moderation review here to make sure
+      ;; test result return the most recent status and don't duplicate the result
+      (doseq [status ["verified" nil "verified"]]
+        (moderation-review/create-review! {:moderated_item_id   card-id
+                                           :moderated_item_type "card"
+                                           :moderator_id        (mt/user->id :crowberto)
+                                           :status              status}))
+      (is (=? [{:id               card-id
+                :model            "card"
+                :moderated_status "verified"}]
+              (:data (mt/user-http-request :crowberto :get 200 "search" :q search-term)))))))
+
 (deftest permissions-test
   (testing (str "Ensure that users without perms for the root collection don't get results NOTE: Metrics and segments "
                 "don't have collections, so they'll be returned")