Skip to content

Commit

Permalink
Merge pull request metabase#7882 from metabase/optimize-xray-candidates
Browse files Browse the repository at this point in the history 
Rework xray candidates to use only 4 queries
  • Loading branch information
@salsakran
salsakran authored Jun 15, 2018
2 parents 19f2e75 + 34b5ae9 commit 1216835
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 30 deletions.
65 changes: 44 additions & 21 deletions src/metabase/automagic_dashboards/core.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -536,23 +536,19 @@
(assoc :score score
:dataset_query query))))))))

(s/defn ^:private rule-specificity
[rule :- rules/Rule]
(transduce (map (comp count ancestors)) + (:applies_to rule)))

(s/defn ^:private matching-rules
(defn- matching-rules
"Return matching rules orderd by specificity.
Most specific is defined as entity type specification the longest ancestor
chain."
[rules :- [rules/Rule], {:keys [source entity]}]
[rules {:keys [source entity]}]
(let [table-type (or (:entity_type source) :entity/GenericTable)]
(->> rules
(filter (fn [{:keys [applies_to]}]
(let [[entity-type field-type] applies_to]
(and (isa? table-type entity-type)
(or (nil? field-type)
(field-isa? entity field-type))))))
(sort-by rule-specificity >))))
(sort-by :specificity >))))

(defn- linked-tables
"Return all tables accessable from a given table with the paths to get there.
Expand Down Expand Up @@ -642,11 +638,11 @@
vals
(apply concat)))

(s/defn ^:private make-dashboard
([root, rule :- rules/Rule]
(defn- make-dashboard
([root rule]
(make-dashboard root rule {:tables [(:source root)]
:root root}))
([root, rule :- rules/Rule, context]
([root rule context]
(-> rule
(select-keys [:title :description :transient_title :groups])
(instantiate-metadata context {})
Expand Down Expand Up @@ -863,13 +859,40 @@
[field opts]
(automagic-dashboard (merge (->root field) opts)))

(defn- enhanced-table-stats
[table]
(let [field-types (->> (db/select [Field :special_type] :table_id (u/get-id table))
(map :special_type))]
(assoc table :stats {:num-fields (count field-types)
:list-like? (= (count (remove #{:type/PK} field-types)) 1)
:link-table? (every? #{:type/FK :type/PK} field-types)})))
(defn- enhance-table-stats
[tables]
(when (not-empty tables)
(let [field-count (->> (db/query {:select [:table_id [:%count.* "count"]]
:from [Field]
:where [:in :table_id (map u/get-id tables)]
:group-by [:table_id]})
(into {} (map (fn [{:keys [count table_id]}]
[table_id count]))))
list-like? (->> (when-let [candidates (->> field-count
(filter (comp (partial >= 2) val))
(map key)
not-empty)]
(db/query {:select [:table_id]
:from [Field]
:where [:and [:in :table_id candidates]
[:or [:not= :special_type "type/PK"]
[:= :special_type nil]]]
:group-by [:table_id]
:having [:= :%count.* 1]}))
(into #{} (map :table_id)))
link-table? (->> (db/query {:select [:table_id [:%count.* "count"]]
:from [Field]
:where [:and [:in :table_id (keys field-count)]
[:in :special_type ["type/PK" "type/FK"]]]
:group-by [:table_id]})
(filter (fn [{:keys [table_id count]}]
(= count (field-count table_id))))
(into #{} (map :table_id)))]
(for [table tables]
(let [table-id (u/get-id table)]
(assoc table :stats {:num-fields (field-count table-id 0)
:list-like? (boolean (list-like? table-id))
:link-table? (boolean (link-table? table-id))}))))))

(def ^:private ^:const ^Long max-candidate-tables
"Maximal number of tables per schema shown in `candidate-tables`."
Expand All @@ -888,13 +911,13 @@
([database] (candidate-tables database nil))
([database schema]
(let [rules (rules/get-rules ["table"])]
(->> (apply db/select Table
(->> (apply db/select [Table :id :schema :display_name :entity_type :db_id]
(cond-> [:db_id (u/get-id database)
:visibility_type nil]
schema (concat [:schema schema])))
(filter mi/can-read?)
(map enhanced-table-stats)
(remove (comp (some-fn :link-table? :list-like?) :stats))
enhance-table-stats
(remove (comp (some-fn :link-table? :list-like? (comp zero? :num-fields)) :stats))
(map (fn [table]
(let [root (->root table)
rule (->> root
Expand All @@ -903,7 +926,7 @@
dashboard (make-dashboard root rule)]
{:url (format "%stable/%s" public-endpoint (u/get-id table))
:title (:full-name root)
:score (+ (math/sq (rule-specificity rule))
:score (+ (math/sq (:specificity rule))
(math/log (-> table :stats :num-fields)))
:description (:description dashboard)
:table table
Expand Down
11 changes: 9 additions & 2 deletions src/metabase/automagic_dashboards/rules.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@
(constrained-all
{(s/required-key :title) s/Str
(s/required-key :rule) s/Str
(s/required-key :specificity) s/Int
(s/optional-key :cards) [Card]
(s/optional-key :dimensions) [Dimension]
(s/optional-key :applies_to) AppliesTo
Expand Down Expand Up @@ -278,6 +279,10 @@
(def ^:private ^{:arglists '([f])} file->entity-type
(comp (partial re-find #".+(?=\.yaml$)") str (memfn ^Path getFileName)))

(defn- specificity
[rule]
(transduce (map (comp count ancestors)) + (:applies_to rule)))

(defn- load-rule
[^Path f]
(try
Expand All @@ -286,9 +291,11 @@
.toUri
slurp
yaml/parse-string
(assoc :rule entity-type)
(assoc :rule entity-type
:specificity 0)
(update :applies_to #(or % entity-type))
rules-validator))
rules-validator
(as-> rule (assoc rule :specificity (specificity rule)))))
(catch Exception e
(log/errorf (trs "Error parsing %s:\n%s")
(.getFileName f)
Expand Down
68 changes: 61 additions & 7 deletions test/metabase/automagic_dashboards/core_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@
atom)]
~@body))

(defmacro ^:private with-dashboard-cleanup
[& body]
`(tu/with-model-cleanup ['~'Card '~'Dashboard '~'Collection '~'DashboardCard]
~@body))


;;; ------------------- `->reference` -------------------

(expect
[:field-id 1]
Expand All @@ -45,6 +52,8 @@
(#'magic/->reference :mbql)))


;;; ------------------- Rule matching -------------------

(expect
[:entity/UserTable :entity/GenericTable :entity/*]
(->> (data/id :users)
Expand All @@ -64,6 +73,8 @@
(map (comp first :applies_to))))


;;; ------------------- `automagic-anaysis` -------------------

(defn- collect-urls
[dashboard]
(->> dashboard
Expand All @@ -87,11 +98,6 @@
(assert (valid-urls? dashboard))
true)

(defmacro ^:private with-dashboard-cleanup
[& body]
`(tu/with-model-cleanup ['~'Card '~'Dashboard '~'Collection '~'DashboardCard]
~@body))

(expect
(with-rasta
(with-dashboard-cleanup
Expand Down Expand Up @@ -235,6 +241,8 @@
valid-dashboard?)))))


;;; ------------------- /candidates -------------------

(expect
3
(with-rasta
Expand All @@ -245,12 +253,56 @@
1
(tt/with-temp* [Database [{db-id :id}]
Table [{table-id :id} {:db_id db-id}]
Field [{} {:table_id table-id}]
Field [{} {:table_id table-id}]]
Field [_ {:table_id table-id}]
Field [_ {:table_id table-id}]]
(with-rasta
(with-dashboard-cleanup
(count (candidate-tables (Database db-id)))))))

(expect
4
(tt/with-temp* [Database [{db-id :id}]
Table [{table-id :id} {:db_id db-id}]
Field [_ {:table_id table-id}]
Field [_ {:table_id table-id}]]
(with-rasta
(with-dashboard-cleanup
(let [database (Database db-id)]
(db/with-call-counting [call-count]
(candidate-tables database)
(call-count)))))))

(expect
{:list-like? true
:link-table? false
:num-fields 2}
(tt/with-temp* [Database [{db-id :id}]
Table [{table-id :id} {:db_id db-id}]
Field [_ {:table_id table-id :special_type :type/PK}]
Field [_ {:table_id table-id}]]
(with-rasta
(with-dashboard-cleanup
(-> (#'magic/enhance-table-stats [(Table table-id)])
first
:stats)))))

(expect
{:list-like? false
:link-table? true
:num-fields 3}
(tt/with-temp* [Database [{db-id :id}]
Table [{table-id :id} {:db_id db-id}]
Field [_ {:table_id table-id :special_type :type/PK}]
Field [_ {:table_id table-id :special_type :type/FK}]
Field [_ {:table_id table-id :special_type :type/FK}]]
(with-rasta
(with-dashboard-cleanup
(-> (#'magic/enhance-table-stats [(Table table-id)])
first
:stats)))))


;;; ------------------- Definition overloading -------------------

;; Identity
(expect
Expand Down Expand Up @@ -304,6 +356,8 @@
key))


;;; ------------------- Datetime resolution inference -------------------

(expect
:month
(#'magic/optimal-datetime-resolution
Expand Down

0 comments on commit 1216835

Please sign in to comment.