Skip to content

Commit

Permalink
Misc drill thru fixes 🔧
Browse files Browse the repository at this point in the history
  • Loading branch information
@camsaul
camsaul committed Sep 28, 2023
1 parent 4302573 commit b5cf03a
Show file tree
Hide file tree
Showing 10 changed files with 266 additions and 136 deletions.
132 changes: 66 additions & 66 deletions ...st/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,79 +175,79 @@
:venues (dissoc (venues-price-mbql-gtap-def) :query)}
:attributes {"user" 5, "price" 1}}
(testing "Should add a filter for attributes-only GTAP"
(is (= (mt/query checkins
{:type :query
:query {:source-query {:source-table $$checkins
:fields [$id !default.$date $user_id $venue_id]
:filter [:and
[:>= !default.date [:absolute-datetime #t "2014-01-02T00:00Z[UTC]" :default]]
[:=
$user_id
[:value 5 {:base_type :type/Integer
:effective_type :type/Integer
:coercion_strategy nil
:semantic_type :type/FK
:database_type "INTEGER"
:name "USER_ID"}]]]
::row-level-restrictions/gtap? true}
:joins [{:source-query
{:source-table $$venues
:fields [$venues.id $venues.name $venues.category_id
$venues.latitude $venues.longitude $venues.price]
:filter [:=
$venues.price
[:value 1 {:base_type :type/Integer
(is (=? (mt/query checkins
{:type :query
:query {:source-query {:source-table $$checkins
:fields [$id !default.$date $user_id $venue_id]
:filter [:and
[:>= !default.date [:absolute-datetime #t "2014-01-02T00:00Z[UTC]" :default]]
[:=
$user_id
[:value 5 {:base_type :type/Integer
:effective_type :type/Integer
:coercion_strategy nil
:semantic_type :type/Category
:semantic_type :type/FK
:database_type "INTEGER"
:name "PRICE"}]]
::row-level-restrictions/gtap? true}
:alias "v"
:strategy :left-join
:condition [:= $venue_id &v.venues.id]}]
:aggregation [[:count]]}

::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
:source :aggregation
:field_ref [:aggregation 0]}]
::qp.perms/perms {:gtaps #{(perms/table-query-path (mt/id :checkins))
(perms/table-query-path (mt/id :venues))}}})
(apply-row-level-permissions
(mt/mbql-query checkins
{:aggregation [[:count]]
:joins [{:source-table $$venues
:alias "v"
:strategy :left-join
:condition [:= $venue_id &v.venues.id]}]}))))))))

(deftest middleware-native-quest-test
:name "USER_ID"}]]]
::row-level-restrictions/gtap? true}
:joins [{:source-query
{:source-table $$venues
:fields [$venues.id $venues.name $venues.category_id
$venues.latitude $venues.longitude $venues.price]
:filter [:=
$venues.price
[:value 1 {:base_type :type/Integer
:effective_type :type/Integer
:coercion_strategy nil
:semantic_type :type/Category
:database_type "INTEGER"
:name "PRICE"}]]
::row-level-restrictions/gtap? true}
:alias "v"
:strategy :left-join
:condition [:= $venue_id &v.venues.id]}]
:aggregation [[:count]]}

::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
:source :aggregation
:field_ref [:aggregation 0]}]
::qp.perms/perms {:gtaps #{(perms/table-query-path (mt/id :checkins))
(perms/table-query-path (mt/id :venues))}}})
(apply-row-level-permissions
(mt/mbql-query checkins
{:aggregation [[:count]]
:joins [{:source-table $$venues
:alias "v"
:strategy :left-join
:condition [:= $venue_id &v.venues.id]}]}))))))))

(deftest middleware-native-query-test
(testing "Make sure the middleware does the correct transformation given the GTAPs we have"
(testing "Should substitute appropriate value in native query"
(met/with-gtaps {:gtaps {:venues (venues-category-native-gtap-def)}
:attributes {"cat" 50}}
(is (= (mt/query nil
{:database (mt/id)
:type :query
:query {:aggregation [[:count]]
:source-query {:native (str "SELECT * FROM \"PUBLIC\".\"VENUES\" "
"WHERE \"PUBLIC\".\"VENUES\".\"CATEGORY_ID\" = 50 "
"ORDER BY \"PUBLIC\".\"VENUES\".\"ID\" ASC")
:params []}}

::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
:source :aggregation
:field_ref [:aggregation 0]}]
::qp.perms/perms {:gtaps #{(perms/adhoc-native-query-path (mt/id))}}})
(apply-row-level-permissions
(mt/mbql-query venues
{:aggregation [[:count]]}))))))))
(is (=? (mt/query nil
{:database (mt/id)
:type :query
:query {:aggregation [[:count]]
:source-query {:native (str "SELECT * FROM \"PUBLIC\".\"VENUES\" "
"WHERE \"PUBLIC\".\"VENUES\".\"CATEGORY_ID\" = 50 "
"ORDER BY \"PUBLIC\".\"VENUES\".\"ID\" ASC")
:params []}}

::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
:source :aggregation
:field_ref [:aggregation 0]}]
::qp.perms/perms {:gtaps #{(perms/adhoc-native-query-path (mt/id))}}})
(apply-row-level-permissions
(mt/mbql-query venues
{:aggregation [[:count]]}))))))))


;;; +----------------------------------------------------------------------------------------------------------------+
Expand Down
14 changes: 13 additions & 1 deletion src/metabase/lib/aggregation.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
[:aggregation options ag-uuid]))

(mu/defn resolve-aggregation :- ::lib.schema.aggregation/aggregation
"Resolve an aggregation with a specific `index`."
"Resolve an aggregation with a specific `ag-uuid`."
[query :- ::lib.schema/query
stage-number :- :int
ag-uuid :- :string]
Expand Down Expand Up @@ -389,3 +389,15 @@
{:aggregation-index ag-index
:query query
:stage-number stage-number})))))

(mu/defn aggregation-at-index :- [:maybe ::lib.schema.aggregation/aggregation]
"Get the aggregation at `index` in a stage of the query if it exists, otherwise `nil`. This is mostly for working
with legacy references like
[:aggregation 0]"
[query :- ::lib.schema/query
stage-number :- :int
index :- ::lib.schema.common/int-greater-than-or-equal-to-zero]
(let [ags (aggregations query stage-number)]
(when (> (clojure.core/count ags) index)
(nth ags index))))
59 changes: 46 additions & 13 deletions src/metabase/lib/drill_thru.cljc
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
(ns metabase.lib.drill-thru
(:require
[metabase.lib.aggregation :as lib.aggregation]
[metabase.lib.drill-thru.column-filter :as lib.drill-thru.column-filter]
[metabase.lib.drill-thru.common :as lib.drill-thru.common]
[metabase.lib.drill-thru.distribution :as lib.drill-thru.distribution]
Expand All @@ -16,6 +17,7 @@
[metabase.lib.drill-thru.zoom :as lib.drill-thru.zoom]
[metabase.lib.drill-thru.zoom-in-timeseries :as lib.drill-thru.zoom-in-timeseries]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
[metabase.util.malli :as mu]))
Expand All @@ -31,6 +33,28 @@

;; TODO: ActionMode, PublicMode, MetabotMode need to be captured in the FE before calling `available-drill-thrus`.

(defn- icky-hack-add-source-uuid-to-aggregation-column-metadata
"This is an evil HACK -- the FE calls [[available-drill-thrus]] with query results metadata as produced
by [[metabase.query-processor.middleware.annotate]], which does not include the `:lib/source-uuid` for aggregation
columns (since it's still using legacy MBQL at this point), which
means [[metabase.lib.aggregation/column-metadata->aggregation-ref]] can't generate references (since it requires
`:lib/source-uuid`)... so we have to add it back in manually. I've added `:aggregation-index` to the annotate
results (see [[metabase.query-processor.middleware.annotate/cols-for-ags-and-breakouts]]), and we can use that to
determine the correct `:lib/source-uuid`.
There's probably a more general place we can be doing this, but it's escaping me, so I guess this will have to do
for now. It doesn't seem like the FE generally uses result metadata in most other places so this is not an issue
elsewhere.
Once we're using MLv2 queries everywhere and stop converting back to legacy we should be able to remove this ICKY
HACK."
[query stage-number {{:keys [aggregation-index], :as column} :column, :as context}]
(or (when (and aggregation-index
(not (:lib/source-uuid column)))
(when-let [ag (lib.aggregation/aggregation-at-index query stage-number aggregation-index)]
(assoc-in context [:column :lib/source-uuid] (lib.options/uuid ag))))
context))

(defmethod lib.metadata.calculation/display-info-method ::drill-thru
[query stage-number drill-thru]
(lib.drill-thru.common/drill-thru-info-method query stage-number drill-thru))
Expand All @@ -47,19 +71,28 @@
([query :- ::lib.schema/query
stage-number :- :int
context :- ::lib.schema.drill-thru/context]
(keep #(% query stage-number context)
;; TODO: Missing drills: automatic insights, format.
[lib.drill-thru.distribution/distribution-drill
lib.drill-thru.column-filter/column-filter-drill
lib.drill-thru.foreign-key/foreign-key-drill
lib.drill-thru.object-details/object-detail-drill
lib.drill-thru.pivot/pivot-drill
lib.drill-thru.quick-filter/quick-filter-drill
lib.drill-thru.sort/sort-drill
lib.drill-thru.summarize-column/summarize-column-drill
lib.drill-thru.summarize-column-by-time/summarize-column-by-time-drill
lib.drill-thru.underlying-records/underlying-records-drill
lib.drill-thru.zoom-in-timeseries/zoom-in-timeseries-drill])))
(let [context (icky-hack-add-source-uuid-to-aggregation-column-metadata query stage-number context)]
(try
(into []
(keep #(% query stage-number context))
;; TODO: Missing drills: automatic insights, format.
[lib.drill-thru.distribution/distribution-drill
lib.drill-thru.column-filter/column-filter-drill
lib.drill-thru.foreign-key/foreign-key-drill
lib.drill-thru.object-details/object-detail-drill
lib.drill-thru.pivot/pivot-drill
lib.drill-thru.quick-filter/quick-filter-drill
lib.drill-thru.sort/sort-drill
lib.drill-thru.summarize-column/summarize-column-drill
lib.drill-thru.summarize-column-by-time/summarize-column-by-time-drill
lib.drill-thru.underlying-records/underlying-records-drill
lib.drill-thru.zoom-in-timeseries/zoom-in-timeseries-drill])
(catch #?(:clj Throwable :cljs :default) e
(throw (ex-info (str "Error getting available drill thrus for query: " (ex-message e))
{:query query
:stage-number stage-number
:context context}
e)))))))

(mu/defn drill-thru :- ::lib.schema/query
"`(drill-thru query stage-number drill-thru)`
Expand Down
5 changes: 3 additions & 2 deletions src/metabase/query_processor/middleware/annotate.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,9 @@
:source :breakout))
(for [[i aggregation] (m/indexed aggregations)]
(assoc (col-info-for-aggregation-clause inner-query aggregation)
:source :aggregation
:field_ref [:aggregation i]))))
:source :aggregation
:field_ref [:aggregation i]
:aggregation_index i))))

(mu/defn cols-for-mbql-query
"Return results metadata about the expected columns in an 'inner' MBQL query."
Expand Down
11 changes: 11 additions & 0 deletions test/metabase/lib/aggregation_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
[clojure.test :refer [are deftest is testing]]
[medley.core :as m]
[metabase.lib.aggregation :as lib.aggregation]
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.query :as lib.query]
Expand Down Expand Up @@ -791,3 +792,13 @@
lib/available-aggregation-operators
(m/find-first #(= (:short %) :sum))
lib/aggregation-operator-columns))))))

(deftest ^:parallel aggregation-at-index-test
(let [query (-> lib.tu/venues-query
(lib/aggregate (lib/count))
(lib/aggregate (lib/count)))]
(are [index expected] (=? expected
(lib.aggregation/aggregation-at-index query -1 index))
0 [:count {}]
1 [:count {}]
2 nil)))
Loading

0 comments on commit b5cf03a

Please sign in to comment.