Skip to content

Commit b5cf03a

Browse files
camsaulcamsaul
committed
Misc drill thru fixes 🔧
1 parent 4302573 commit b5cf03a

File tree

10 files changed

+266
-136
lines changed

10 files changed

+266
-136
lines changed

‎enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj

Lines changed: 66 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -175,79 +175,79 @@
175175
:venues (dissoc (venues-price-mbql-gtap-def) :query)}
176176
:attributes {"user" 5, "price" 1}}
177177
(testing "Should add a filter for attributes-only GTAP"
178-
(is (= (mt/query checkins
179-
{:type :query
180-
:query {:source-query {:source-table $$checkins
181-
:fields [$id !default.$date $user_id $venue_id]
182-
:filter [:and
183-
[:>= !default.date [:absolute-datetime #t "2014-01-02T00:00Z[UTC]" :default]]
184-
[:=
185-
$user_id
186-
[:value 5 {:base_type :type/Integer
187-
:effective_type :type/Integer
188-
:coercion_strategy nil
189-
:semantic_type :type/FK
190-
:database_type "INTEGER"
191-
:name "USER_ID"}]]]
192-
::row-level-restrictions/gtap? true}
193-
:joins [{:source-query
194-
{:source-table $$venues
195-
:fields [$venues.id $venues.name $venues.category_id
196-
$venues.latitude $venues.longitude $venues.price]
197-
:filter [:=
198-
$venues.price
199-
[:value 1 {:base_type :type/Integer
178+
(is (=? (mt/query checkins
179+
{:type :query
180+
:query {:source-query {:source-table $$checkins
181+
:fields [$id !default.$date $user_id $venue_id]
182+
:filter [:and
183+
[:>= !default.date [:absolute-datetime #t "2014-01-02T00:00Z[UTC]" :default]]
184+
[:=
185+
$user_id
186+
[:value 5 {:base_type :type/Integer
200187
:effective_type :type/Integer
201188
:coercion_strategy nil
202-
:semantic_type :type/Category
189+
:semantic_type :type/FK
203190
:database_type "INTEGER"
204-
:name "PRICE"}]]
205-
::row-level-restrictions/gtap? true}
206-
:alias "v"
207-
:strategy :left-join
208-
:condition [:= $venue_id &v.venues.id]}]
209-
:aggregation [[:count]]}
210-
211-
::row-level-restrictions/original-metadata [{:base_type :type/Integer
212-
:semantic_type :type/Quantity
213-
:name "count"
214-
:display_name "Count"
215-
:source :aggregation
216-
:field_ref [:aggregation 0]}]
217-
::qp.perms/perms {:gtaps #{(perms/table-query-path (mt/id :checkins))
218-
(perms/table-query-path (mt/id :venues))}}})
219-
(apply-row-level-permissions
220-
(mt/mbql-query checkins
221-
{:aggregation [[:count]]
222-
:joins [{:source-table $$venues
223-
:alias "v"
224-
:strategy :left-join
225-
:condition [:= $venue_id &v.venues.id]}]}))))))))
226-
227-
(deftest middleware-native-quest-test
191+
:name "USER_ID"}]]]
192+
::row-level-restrictions/gtap? true}
193+
:joins [{:source-query
194+
{:source-table $$venues
195+
:fields [$venues.id $venues.name $venues.category_id
196+
$venues.latitude $venues.longitude $venues.price]
197+
:filter [:=
198+
$venues.price
199+
[:value 1 {:base_type :type/Integer
200+
:effective_type :type/Integer
201+
:coercion_strategy nil
202+
:semantic_type :type/Category
203+
:database_type "INTEGER"
204+
:name "PRICE"}]]
205+
::row-level-restrictions/gtap? true}
206+
:alias "v"
207+
:strategy :left-join
208+
:condition [:= $venue_id &v.venues.id]}]
209+
:aggregation [[:count]]}
210+
211+
::row-level-restrictions/original-metadata [{:base_type :type/Integer
212+
:semantic_type :type/Quantity
213+
:name "count"
214+
:display_name "Count"
215+
:source :aggregation
216+
:field_ref [:aggregation 0]}]
217+
::qp.perms/perms {:gtaps #{(perms/table-query-path (mt/id :checkins))
218+
(perms/table-query-path (mt/id :venues))}}})
219+
(apply-row-level-permissions
220+
(mt/mbql-query checkins
221+
{:aggregation [[:count]]
222+
:joins [{:source-table $$venues
223+
:alias "v"
224+
:strategy :left-join
225+
:condition [:= $venue_id &v.venues.id]}]}))))))))
226+
227+
(deftest middleware-native-query-test
228228
(testing "Make sure the middleware does the correct transformation given the GTAPs we have"
229229
(testing "Should substitute appropriate value in native query"
230230
(met/with-gtaps {:gtaps {:venues (venues-category-native-gtap-def)}
231231
:attributes {"cat" 50}}
232-
(is (= (mt/query nil
233-
{:database (mt/id)
234-
:type :query
235-
:query {:aggregation [[:count]]
236-
:source-query {:native (str "SELECT * FROM \"PUBLIC\".\"VENUES\" "
237-
"WHERE \"PUBLIC\".\"VENUES\".\"CATEGORY_ID\" = 50 "
238-
"ORDER BY \"PUBLIC\".\"VENUES\".\"ID\" ASC")
239-
:params []}}
240-
241-
::row-level-restrictions/original-metadata [{:base_type :type/Integer
242-
:semantic_type :type/Quantity
243-
:name "count"
244-
:display_name "Count"
245-
:source :aggregation
246-
:field_ref [:aggregation 0]}]
247-
::qp.perms/perms {:gtaps #{(perms/adhoc-native-query-path (mt/id))}}})
248-
(apply-row-level-permissions
249-
(mt/mbql-query venues
250-
{:aggregation [[:count]]}))))))))
232+
(is (=? (mt/query nil
233+
{:database (mt/id)
234+
:type :query
235+
:query {:aggregation [[:count]]
236+
:source-query {:native (str "SELECT * FROM \"PUBLIC\".\"VENUES\" "
237+
"WHERE \"PUBLIC\".\"VENUES\".\"CATEGORY_ID\" = 50 "
238+
"ORDER BY \"PUBLIC\".\"VENUES\".\"ID\" ASC")
239+
:params []}}
240+
241+
::row-level-restrictions/original-metadata [{:base_type :type/Integer
242+
:semantic_type :type/Quantity
243+
:name "count"
244+
:display_name "Count"
245+
:source :aggregation
246+
:field_ref [:aggregation 0]}]
247+
::qp.perms/perms {:gtaps #{(perms/adhoc-native-query-path (mt/id))}}})
248+
(apply-row-level-permissions
249+
(mt/mbql-query venues
250+
{:aggregation [[:count]]}))))))))
251251

252252

253253
;;; +----------------------------------------------------------------------------------------------------------------+

‎src/metabase/lib/aggregation.cljc

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
[:aggregation options ag-uuid]))
3131

3232
(mu/defn resolve-aggregation :- ::lib.schema.aggregation/aggregation
33-
"Resolve an aggregation with a specific `index`."
33+
"Resolve an aggregation with a specific `ag-uuid`."
3434
[query :- ::lib.schema/query
3535
stage-number :- :int
3636
ag-uuid :- :string]
@@ -389,3 +389,15 @@
389389
{:aggregation-index ag-index
390390
:query query
391391
:stage-number stage-number})))))
392+
393+
(mu/defn aggregation-at-index :- [:maybe ::lib.schema.aggregation/aggregation]
394+
"Get the aggregation at `index` in a stage of the query if it exists, otherwise `nil`. This is mostly for working
395+
with legacy references like
396+
397+
[:aggregation 0]"
398+
[query :- ::lib.schema/query
399+
stage-number :- :int
400+
index :- ::lib.schema.common/int-greater-than-or-equal-to-zero]
401+
(let [ags (aggregations query stage-number)]
402+
(when (> (clojure.core/count ags) index)
403+
(nth ags index))))

‎src/metabase/lib/drill_thru.cljc

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
(ns metabase.lib.drill-thru
22
(:require
3+
[metabase.lib.aggregation :as lib.aggregation]
34
[metabase.lib.drill-thru.column-filter :as lib.drill-thru.column-filter]
45
[metabase.lib.drill-thru.common :as lib.drill-thru.common]
56
[metabase.lib.drill-thru.distribution :as lib.drill-thru.distribution]
@@ -16,6 +17,7 @@
1617
[metabase.lib.drill-thru.zoom :as lib.drill-thru.zoom]
1718
[metabase.lib.drill-thru.zoom-in-timeseries :as lib.drill-thru.zoom-in-timeseries]
1819
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
20+
[metabase.lib.options :as lib.options]
1921
[metabase.lib.schema :as lib.schema]
2022
[metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
2123
[metabase.util.malli :as mu]))
@@ -31,6 +33,28 @@
3133

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

36+
(defn- icky-hack-add-source-uuid-to-aggregation-column-metadata
37+
"This is an evil HACK -- the FE calls [[available-drill-thrus]] with query results metadata as produced
38+
by [[metabase.query-processor.middleware.annotate]], which does not include the `:lib/source-uuid` for aggregation
39+
columns (since it's still using legacy MBQL at this point), which
40+
means [[metabase.lib.aggregation/column-metadata->aggregation-ref]] can't generate references (since it requires
41+
`:lib/source-uuid`)... so we have to add it back in manually. I've added `:aggregation-index` to the annotate
42+
results (see [[metabase.query-processor.middleware.annotate/cols-for-ags-and-breakouts]]), and we can use that to
43+
determine the correct `:lib/source-uuid`.
44+
45+
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
46+
for now. It doesn't seem like the FE generally uses result metadata in most other places so this is not an issue
47+
elsewhere.
48+
49+
Once we're using MLv2 queries everywhere and stop converting back to legacy we should be able to remove this ICKY
50+
HACK."
51+
[query stage-number {{:keys [aggregation-index], :as column} :column, :as context}]
52+
(or (when (and aggregation-index
53+
(not (:lib/source-uuid column)))
54+
(when-let [ag (lib.aggregation/aggregation-at-index query stage-number aggregation-index)]
55+
(assoc-in context [:column :lib/source-uuid] (lib.options/uuid ag))))
56+
context))
57+
3458
(defmethod lib.metadata.calculation/display-info-method ::drill-thru
3559
[query stage-number drill-thru]
3660
(lib.drill-thru.common/drill-thru-info-method query stage-number drill-thru))
@@ -47,19 +71,28 @@
4771
([query :- ::lib.schema/query
4872
stage-number :- :int
4973
context :- ::lib.schema.drill-thru/context]
50-
(keep #(% query stage-number context)
51-
;; TODO: Missing drills: automatic insights, format.
52-
[lib.drill-thru.distribution/distribution-drill
53-
lib.drill-thru.column-filter/column-filter-drill
54-
lib.drill-thru.foreign-key/foreign-key-drill
55-
lib.drill-thru.object-details/object-detail-drill
56-
lib.drill-thru.pivot/pivot-drill
57-
lib.drill-thru.quick-filter/quick-filter-drill
58-
lib.drill-thru.sort/sort-drill
59-
lib.drill-thru.summarize-column/summarize-column-drill
60-
lib.drill-thru.summarize-column-by-time/summarize-column-by-time-drill
61-
lib.drill-thru.underlying-records/underlying-records-drill
62-
lib.drill-thru.zoom-in-timeseries/zoom-in-timeseries-drill])))
74+
(let [context (icky-hack-add-source-uuid-to-aggregation-column-metadata query stage-number context)]
75+
(try
76+
(into []
77+
(keep #(% query stage-number context))
78+
;; TODO: Missing drills: automatic insights, format.
79+
[lib.drill-thru.distribution/distribution-drill
80+
lib.drill-thru.column-filter/column-filter-drill
81+
lib.drill-thru.foreign-key/foreign-key-drill
82+
lib.drill-thru.object-details/object-detail-drill
83+
lib.drill-thru.pivot/pivot-drill
84+
lib.drill-thru.quick-filter/quick-filter-drill
85+
lib.drill-thru.sort/sort-drill
86+
lib.drill-thru.summarize-column/summarize-column-drill
87+
lib.drill-thru.summarize-column-by-time/summarize-column-by-time-drill
88+
lib.drill-thru.underlying-records/underlying-records-drill
89+
lib.drill-thru.zoom-in-timeseries/zoom-in-timeseries-drill])
90+
(catch #?(:clj Throwable :cljs :default) e
91+
(throw (ex-info (str "Error getting available drill thrus for query: " (ex-message e))
92+
{:query query
93+
:stage-number stage-number
94+
:context context}
95+
e)))))))
6396

6497
(mu/defn drill-thru :- ::lib.schema/query
6598
"`(drill-thru query stage-number drill-thru)`

‎src/metabase/query_processor/middleware/annotate.clj

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -399,8 +399,9 @@
399399
:source :breakout))
400400
(for [[i aggregation] (m/indexed aggregations)]
401401
(assoc (col-info-for-aggregation-clause inner-query aggregation)
402-
:source :aggregation
403-
:field_ref [:aggregation i]))))
402+
:source :aggregation
403+
:field_ref [:aggregation i]
404+
:aggregation_index i))))
404405

405406
(mu/defn cols-for-mbql-query
406407
"Return results metadata about the expected columns in an 'inner' MBQL query."

‎test/metabase/lib/aggregation_test.cljc

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
44
[clojure.test :refer [are deftest is testing]]
55
[medley.core :as m]
6+
[metabase.lib.aggregation :as lib.aggregation]
67
[metabase.lib.convert :as lib.convert]
78
[metabase.lib.core :as lib]
89
[metabase.lib.query :as lib.query]
@@ -791,3 +792,13 @@
791792
lib/available-aggregation-operators
792793
(m/find-first #(= (:short %) :sum))
793794
lib/aggregation-operator-columns))))))
795+
796+
(deftest ^:parallel aggregation-at-index-test
797+
(let [query (-> lib.tu/venues-query
798+
(lib/aggregate (lib/count))
799+
(lib/aggregate (lib/count)))]
800+
(are [index expected] (=? expected
801+
(lib.aggregation/aggregation-at-index query -1 index))
802+
0 [:count {}]
803+
1 [:count {}]
804+
2 nil)))

0 commit comments

Comments
 (0)