Skip to content
Snippets Groups Projects
Unverified Commit f3b31a92 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

`quick-filter` drill on an aggregate column should introduce a new stage (#34812)

parent 42bd30c6
Branches
Tags
No related merge requests found
......@@ -27,6 +27,7 @@ import {
} from "metabase-types/api/mocks/presets";
import { createMockColumn } from "metabase-types/api/mocks";
import type {
ComparisonFilter,
DatasetColumn,
RowValue,
StructuredDatasetQuery,
......@@ -1969,70 +1970,70 @@ describe("drillThru", () => {
},
},
// FIXME: filter gets applied on the the same query stage as aggregations, but it should wrap the query (metabase#34346)
// {
// drillType: "drill-thru/quick-filter",
// clickType: "cell",
// columnName: "sum",
// queryType: "aggregated",
// drillArgs: ["="],
// expectedQuery: {
// "source-query": AGGREGATED_ORDERS_DATASET_QUERY.query,
// filter: [
// "=",
// [
// "field",
// "sum",
// {
// "base-type": "type/Float",
// },
// ],
// AGGREGATED_ORDERS_ROW_VALUES.sum,
// ],
// },
// },
// {
// drillType: "drill-thru/quick-filter",
// clickType: "cell",
// columnName: "CREATED_AT",
// queryType: "aggregated",
// drillArgs: ["<"],
// expectedQuery: {
// ...AGGREGATED_ORDERS_DATASET_QUERY.query,
// filter: [
// "<",
// [
// "field",
// ORDERS.CREATED_AT,
// {
// "base-type": "type/DateTime",
// "temporal-unit": "month",
// },
// ],
// AGGREGATED_ORDERS_ROW_VALUES.CREATED_AT,
// ] as ComparisonFilter,
// },
// },
// {
// drillType: "drill-thru/quick-filter",
// clickType: "cell",
// columnName: "max",
// queryType: "aggregated",
// drillArgs: ["≠"],
// expectedQuery: {
// "source-query": AGGREGATED_ORDERS_DATASET_QUERY.query,
// filter: [
// "not-null",
// [
// "field",
// "max",
// {
// "base-type": "type/Float",
// },
// ],
// ],
// },
// },
// filter gets applied on the the same query stage as aggregations, but it should wrap the query (metabase#34346)
{
drillType: "drill-thru/quick-filter",
clickType: "cell",
columnName: "sum",
queryType: "aggregated",
drillArgs: ["="],
expectedQuery: {
"source-query": AGGREGATED_ORDERS_DATASET_QUERY.query,
filter: [
"=",
[
"field",
"sum",
{
"base-type": "type/Float",
},
],
AGGREGATED_ORDERS_ROW_VALUES.sum,
],
},
},
{
drillType: "drill-thru/quick-filter",
clickType: "cell",
columnName: "CREATED_AT",
queryType: "aggregated",
drillArgs: ["<"],
expectedQuery: {
...AGGREGATED_ORDERS_DATASET_QUERY.query,
filter: [
"<",
[
"field",
ORDERS.CREATED_AT,
{
"base-type": "type/DateTime",
"temporal-unit": "month",
},
],
AGGREGATED_ORDERS_ROW_VALUES.CREATED_AT,
] as ComparisonFilter,
},
},
{
drillType: "drill-thru/quick-filter",
clickType: "cell",
columnName: "max",
queryType: "aggregated",
drillArgs: [""],
expectedQuery: {
"source-query": AGGREGATED_ORDERS_DATASET_QUERY.query,
filter: [
"not-null",
[
"field",
"max",
{
"base-type": "type/Float",
},
],
],
},
},
// fk-details should create a query for fk target table (metabase#34383)
{
......
(ns metabase.lib.drill-thru.progression
"Logic for when it's possible to \"drill down\" into some breakout on a query, seeing it at a finer level of detail."
;; (:require
;; [medley.core :as m]
;; [metabase.lib.metadata.calculation :as lib.metadata.calculation]
;; [metabase.lib.options :as lib.options]
;; [metabase.lib.types.isa :as lib.types.isa])
)
;; (defn- field-columns-by-name [query stage-number dimensions]
;; (let [by-name (m/index-by :name (lib.metadata.calculation/visible-columns query stage-number query))]
;; (for [{:keys [column-name]} dimensions
;; :let [column (get by-name column-name)]
;; :when (and column (not= (:lib/source column) :source/expressions))]
;; column)))
;; (def ^:private drill-down-progressions
;; [;; DateTime drill down
;; {:predicates [#(some-> % :unit #{:year :quarter :month :week :day :hour})]
;; :zoom-in #(lib.options/update-options update :unit {:year :quarter
;; :quarter :month
;; :month :week
;; :week :day
;; :day :hour
;; :hour :minute})}
;; ;; Country => State (=> City)
;; {:predicates [lib.types.isa/country?]
;; :zoom-in nil}
;; ;; START HERE I think the transforms can end up applying to different dimensions before and after applying a
;; ;; drill step. (eg. from COUNTRY to STATE, or STATE to lat/long)
;; ]
;; )
;; (defn- progression-matches?
;; "Returns the `:zoom-in` function of a progression if all its `:predicates` pass."
;; [{:keys [predicates zoom-in]} columns]
;; (when (every? #(some % columns) predicates)
;; zoom-in))
;; (defn- matching-progression
;; "A progression applies to a query and dimensions if for each function in `:predicates` there exists some column that
;; satisfies it. (The columns may be different for each predicate, there just needs to be some column that passes each.)"
;; [query stage-number columns]
;; (some #(progression-matches? % columns) drill-down-progressions))
;; (defn- next-breakouts [query stage-number dimensions]
;; (when-let [columns (not-empty (field-columns-by-name query stage-number dimensions))]
;; (prn columns)
;; (when-let [progression (matching-progression query stage-number columns)]
;; progression
;; )
;; ))
......@@ -9,6 +9,7 @@
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.drill-thru :as lib.schema.drill-thru]
[metabase.lib.stage :as lib.stage]
[metabase.lib.types.isa :as lib.types.isa]
[metabase.util.malli :as mu]))
......@@ -49,14 +50,22 @@
stage-number :- :int
{:keys [column value]} :- ::lib.schema.drill-thru/context]
(when (and (lib.drill-thru.common/mbql-stage? query stage-number)
;(editable? query stage-number)
;; (editable? query stage-number)
column
(some? value)
(not (lib.types.isa/primary-key? column))
(not (lib.types.isa/foreign-key? column)))
{:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/quick-filter
:operators (operators-for column value)}))
;; for aggregate columns, we want to introduce a new stage when applying the drill-thru, `:new-stage?` is used to
;; track this. (#34346)
(let [{:keys [column new-stage?]} (if (= (:lib/source column) :source/aggregations)
{:column (assoc column :lib/source :source/previous-stage)
:new-stage? true}
{:column column
:new-stage? false})]
{:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/quick-filter
:operators (operators-for column value)
:new-stage? new-stage?})))
(defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/quick-filter
[_query _stage-number drill-thru]
......@@ -68,10 +77,12 @@
stage-number :- :int
drill :- ::lib.schema.drill-thru/drill-thru.quick-filter
filter-op :- ::lib.schema.common/non-blank-string]
(if-let [quick-filter (m/find-first #(= (:name %) filter-op) (:operators drill))]
(lib.filter/filter query stage-number (:filter quick-filter))
(throw (ex-info (str "No matching filter for operator " filter-op)
{:drill-thru drill
:operator filter-op
:query query
:stage-number stage-number}))))
(let [quick-filter (or (m/find-first #(= (:name %) filter-op) (:operators drill))
(throw (ex-info (str "No matching filter for operator " filter-op)
{:drill-thru drill
:operator filter-op
:query query
:stage-number stage-number})))
query (cond-> query
(:new-stage? drill) lib.stage/append-stage)]
(lib.filter/filter query stage-number (:filter quick-filter))))
......@@ -71,8 +71,11 @@
[:merge
::drill-thru.common
[:map
[:type [:= :drill-thru/quick-filter]]
[:operators [:sequential ::drill-thru.quick-filter.operator]]]])
[:type [:= :drill-thru/quick-filter]]
[:operators [:sequential ::drill-thru.quick-filter.operator]]
;; whether applying this drill thru should introduce a new stage to the query. Filters on aggregate columns should
;; introduce a new stage.
[:new-stage? :boolean]]])
(mr/def ::drill-thru.fk-filter
[:merge
......
(ns metabase.lib.drill-thru.fk-details-test
(:require
[clojure.test :refer [deftest is testing]]
[medley.core :as m]
[metabase.lib.core :as lib]
[clojure.test :refer [deftest testing]]
[metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
[metabase.lib.test-metadata :as meta]
[metabase.util :as u]
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
[metabase.lib.test-metadata :as meta]))
(deftest ^:parallel returns-fk-details-test-1
(lib.drill-thru.tu/test-returns-drill
......@@ -32,63 +26,44 @@
(deftest ^:parallel do-not-return-fk-details-for-non-fk-column-test
(testing "fk-details should not get returned for non-fk column (#34441)"
(let [test-case {:click-type :cell
:query-type :aggregated
:column-name "max"}
{:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
context (lib.drill-thru.tu/test-case-context query row test-case)]
(testing (str "\nQuery = \n" (u/pprint-to-str query)
"\nContext =\n" (u/pprint-to-str context))
(let [drills (into #{}
(map :type)
(lib/available-drill-thrus query context))]
(testing (str "\nAvailable drills =\n" (u/pprint-to-str drills))
(is (not (contains? drills :drill-thru/fk-details)))))))))
(lib.drill-thru.tu/test-drill-not-returned
{:click-type :cell
:query-type :aggregated
:column-name "max"
:drill-type :drill-thru/fk-details})))
(deftest ^:parallel apply-fk-details-test
(testing "fk-details should create a correct query for fk target table (#34383)"
(let [test-case {:click-type :cell
:query-type :unaggregated
:column-name "PRODUCT_ID"}
{:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
context (lib.drill-thru.tu/test-case-context query row test-case)]
(is (get row "PRODUCT_ID"))
(testing (str "\nQuery = \n" (u/pprint-to-str query)
"\nContext =\n" (u/pprint-to-str context))
(let [drill (m/find-first #(= (:type %) :drill-thru/fk-details)
(lib/available-drill-thrus query context))]
(is (=? {:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/fk-details,
:column {:name "PRODUCT_ID"}
:object-id (get row "PRODUCT_ID")
:many-pks? false}
drill))
(is (=? {:stages [{:source-table (meta/id :products)
:filters [[:= {}
[:field {} (meta/id :products :id)]
(get row "PRODUCT_ID")]]}]}
(lib/drill-thru query -1 drill))))))))
(let [column-value (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "PRODUCT_ID"])]
(lib.drill-thru.tu/test-drill-application
{:click-type :cell
:query-type :unaggregated
:column-name "PRODUCT_ID"
:drill-type :drill-thru/fk-details
:expected {:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/fk-details
:column {:name "PRODUCT_ID"}
:object-id column-value
:many-pks? false}
:expected-query {:stages [{:source-table (meta/id :products)
:filters [[:= {}
[:field {} (meta/id :products :id)]
column-value]]}]}}))))
(deftest ^:parallel apply-fk-details-test-2
(testing "fk-details should create a correct query for fk target table (#34383)"
(let [test-case {:click-type :cell
:query-type :unaggregated
:column-name "USER_ID"}
{:keys [query row]} (lib.drill-thru.tu/query-and-row-for-test-case test-case)
context (lib.drill-thru.tu/test-case-context query row test-case)]
(is (get row "USER_ID"))
(testing (str "\nQuery = \n" (u/pprint-to-str query)
"\nContext =\n" (u/pprint-to-str context))
(let [drill (m/find-first #(= (:type %) :drill-thru/fk-details)
(lib/available-drill-thrus query context))]
(is (=? {:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/fk-details,
:column {:name "USER_ID"}
:object-id (get row "USER_ID")
:many-pks? false}
drill))
(is (=? {:stages [{:source-table (meta/id :people)
:filters [[:= {}
[:field {} (meta/id :people :id)]
(get row "USER_ID")]]}]}
(lib/drill-thru query -1 drill))))))))
(let [column-value (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "USER_ID"])]
(lib.drill-thru.tu/test-drill-application
{:click-type :cell
:query-type :unaggregated
:column-name "USER_ID"
:drill-type :drill-thru/fk-details
:expected {:lib/type :metabase.lib.drill-thru/drill-thru
:type :drill-thru/fk-details
:column {:name "USER_ID"}
:object-id column-value
:many-pks? false}
:expected-query {:stages [{:source-table (meta/id :people)
:filters [[:= {}
[:field {} (meta/id :people :id)]
column-value]]}]}}))))
(ns metabase.lib.drill-thru.quick-filter-test
(:require
[clojure.test :refer [deftest testing]]
[metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]))
[metabase.lib.drill-thru.test-util :as lib.drill-thru.tu]
[metabase.lib.test-metadata :as meta]))
(deftest ^:parallel returns-quick-filter-test-1
(lib.drill-thru.tu/test-returns-drill
......@@ -88,3 +89,55 @@
:column-name "max"
:expected {:type :drill-thru/quick-filter, :operators [{:name "="}
{:name "≠"}]}})))
(deftest ^:parallel apply-quick-filter-on-correct-level-test
(testing "quick-filter on an aggregation should introduce an new stage (#34346)"
(lib.drill-thru.tu/test-drill-application
{:click-type :cell
:query-type :aggregated
:column-name "sum"
:drill-type :drill-thru/quick-filter
:expected {:type :drill-thru/quick-filter
:operators [{:name "<"}
{:name ">"}
{:name "="}
{:name "≠"}]
:new-stage? true}
:drill-args ["="]
:expected-query {:stages [{}
{:filters [[:= {}
[:field {} "sum"]
(get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated :row "sum"])]]}]}})))
(deftest ^:parallel apply-quick-filter-on-correct-level-test-2
(testing "quick-filter not on an aggregation should NOT introduce an new stage"
(lib.drill-thru.tu/test-drill-application
{:click-type :cell
:query-type :aggregated
:column-name "CREATED_AT"
:drill-type :drill-thru/quick-filter
:expected {:type :drill-thru/quick-filter
:operators [{:name "<"}
{:name ">"}
{:name "="}
{:name "≠"}]
:new-stage? false}
:drill-args ["<"]
:expected-query {:stages [{:filters [[:< {}
[:field {:temporal-unit :month} (meta/id :orders :created-at)]
(get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated :row "CREATED_AT"])]]}]}})))
(deftest ^:parallel apply-quick-filter-on-correct-level-test-3
(testing "quick-filter on an aggregation should introduce an new stage (#34346)"
(lib.drill-thru.tu/test-drill-application
{:click-type :cell
:query-type :aggregated
:column-name "max"
:drill-type :drill-thru/quick-filter
:expected {:type :drill-thru/quick-filter
:operators [{:name "=", :filter [:is-null {} [:field {} "max"]]}
{:name "≠", :filter [:not-null {} [:field {} "max"]]}]
:new-stage? true}
:drill-args ["≠"]
:expected-query {:stages [{}
{:filters [[:not-null {} [:field {} "max"]]]}]}})))
......@@ -204,28 +204,22 @@
(deftest ^:parallel custom-column-test
(testing "should support sorting for custom column without table relation (metabase#34499)"
(let [{:keys [query row]} (get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated])
query (as-> query query
(lib/expression query "CustomColumn" (lib/+ 1 1))
(lib/expression query "CustomTax" (lib/+ (meta/field-metadata :orders :tax) 2))
(lib/aggregate query (lib/avg (lib/expression-ref query "CustomTax")))
(lib/breakout query (lib/expression-ref query "CustomColumn")))
row (merge row
{"CustomColumn" 2
"avg" 13.2})
context (lib.drill-thru.tu/test-case-context query
row
{:column-name "CustomColumn"
:click-type :header
:query-type :aggregated})
drill (m/find-first
#(= (:type %) :drill-thru/sort)
(lib/available-drill-thrus query -1 context))]
(is (=? {:type :drill-thru/sort
:column {:name "CustomColumn"}
:sort-directions [:asc :desc]}
drill))
(let [query' (lib/drill-thru query drill)]
(is (=? [[:asc {} [:expression {} "CustomColumn"]]]
(lib/order-bys query')))))))
(let [query (as-> (get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated :query]) query
(lib/expression query "CustomColumn" (lib/+ 1 1))
(lib/expression query "CustomTax" (lib/+ (meta/field-metadata :orders :tax) 2))
(lib/aggregate query (lib/avg (lib/expression-ref query "CustomTax")))
(lib/breakout query (lib/expression-ref query "CustomColumn")))
row (merge (get-in lib.drill-thru.tu/test-queries ["ORDERS" :aggregated :row])
{"CustomColumn" 2
"avg" 13.2})]
(lib.drill-thru.tu/test-drill-application
{:column-name "CustomColumn"
:click-type :header
:query-type :aggregated
:custom-query query
:custom-row row
:drill-type :drill-thru/sort
:expected {:type :drill-thru/sort
:column {:name "CustomColumn"}
:sort-directions [:asc :desc]}
:expected-query {:stages [{:order-by [[:asc {} [:expression {} "CustomColumn"]]]}]}}))))
......@@ -54,26 +54,31 @@
"RATING" 4
"CREATED_AT" "2024-09-08T22:03:20.239+03:00"}}}})
(def ^:private Row
[:map-of :string :any])
(def ^:private TestCase
[:map
[:click-type [:enum :cell :header]]
[:query-type [:enum :aggregated :unaggregated]]
[:column-name :string]
;; defaults to "ORDERS"
[:query-table {:optional true} [:enum "ORDERS" "PRODUCTS"]]
[:custom-query {:optional true} ::lib.schema/query]])
(def ^:private Row
[:map-of :string :any])
[:query-table {:optional true} [:maybe [:enum "ORDERS" "PRODUCTS"]]]
[:custom-query {:optional true} [:maybe ::lib.schema/query]]
[:custom-row {:optional true} [:maybe Row]]])
(mu/defn query-and-row-for-test-case :- [:map
[:query ::lib.schema/query]
[:row Row]]
[{:keys [query-table query-type]
[{:keys [query-table query-type custom-query custom-row]
:or {query-table "ORDERS"}
:as test-case} :- TestCase]
(or (get-in test-queries [query-table query-type])
(throw (ex-info "Invalid query-table/query-:type no matching test query" {:test-case test-case}))))
{:query (or custom-query
(get-in test-queries [query-table query-type :query])
(throw (ex-info "Invalid query-table/query-:type no matching test query" {:test-case test-case})))
:row (or custom-row
(get-in test-queries [query-table query-type :row])
(throw (ex-info "Invalid query-table/query-:type no matching test query" {:test-case test-case})))})
(mu/defn test-case-context :- ::lib.schema.drill-thru/context
[query :- ::lib.schema/query
......@@ -123,33 +128,81 @@
(is (=? expected
(lib/available-drill-thrus query -1 context)))))))
(def ^:private ReturnsDrillTestCase
(def ^:private TestCaseWithDrillType
[:merge
TestCase
[:map
[:drill-type ::lib.schema.drill-thru/drill-thru.type]
[:expected [:map
[:type ::lib.schema.drill-thru/drill-thru.type]]]]])
[:drill-type ::lib.schema.drill-thru/drill-thru.type]]])
(def ^:private ReturnsDrillTestCase
[:merge
TestCaseWithDrillType
[:map
[:expected [:map
[:type ::lib.schema.drill-thru/drill-thru.type]]]]])
(mu/defn test-returns-drill
"Test that a certain drill gets returned."
[{:keys [drill-type query-table column-name click-type query-type custom-query expected]
(mu/defn test-returns-drill :- ::lib.schema.drill-thru/drill-thru
"Test that a certain drill gets returned. Returns the drill."
[{:keys [drill-type query-table column-name click-type query-type expected]
:or {query-table "ORDERS"}
:as test-case} :- ReturnsDrillTestCase]
(testing (lib.util/format "should return %s drill config for %s.%s %s in %s query"
drill-type
query-table
column-name
(name click-type)
(name query-type))
(let [{:keys [query row]} (query-and-row-for-test-case test-case)
query (or custom-query query)
context (test-case-context query row test-case)]
(testing (str "\nQuery =\n" (u/pprint-to-str query)
"\nContext =\n" (u/pprint-to-str context))
(let [drills (lib/available-drill-thrus query -1 context)]
(testing (str "\nAvailable Drills =\n" (u/pprint-to-str (into #{} (map :type) drills)))
(let [{:keys [query row]} (query-and-row-for-test-case test-case)
context (test-case-context query row test-case)]
(testing (str (lib.util/format "should return %s drill for %s.%s %s in %s query"
drill-type
query-table
column-name
(name click-type)
(name query-type))
"\nQuery =\n" (u/pprint-to-str query)
"\nContext =\n" (u/pprint-to-str context))
(let [drills (lib/available-drill-thrus query -1 context)]
(testing (str "\nAvailable Drills =\n" (u/pprint-to-str (into #{} (map :type) drills)))
(let [drill (m/find-first (fn [drill]
(= (:type drill) drill-type))
drills)]
(is (=? expected
(m/find-first (fn [drill]
(= (:type drill) drill-type))
drills)))))))))
drill))
drill))))))
(mu/defn test-drill-not-returned
"Test that a drill is NOT returned in a certain situation."
[{:keys [drill-type query-table column-name click-type query-type]
:or {query-table "ORDERS"}
:as test-case} :- TestCaseWithDrillType]
(let [{:keys [query row]} (query-and-row-for-test-case test-case)
context (test-case-context query row test-case)]
(testing (str (lib.util/format "should NOT return %s drill for %s.%s %s in %s query"
drill-type
query-table
column-name
(name click-type)
(name query-type))
"\nQuery = \n" (u/pprint-to-str query)
"\nRow = \n" (u/pprint-to-str row)
"\nContext =\n" (u/pprint-to-str context))
(let [drills (into #{}
(map :type)
(lib/available-drill-thrus query context))]
(testing (str "\nAvailable drills =\n" (u/pprint-to-str drills))
(is (not (contains? drills drill-type))))))))
(def ^:private DrillApplicationTestCase
[:merge
ReturnsDrillTestCase
[:map
[:expected-query :map]
[:drill-args {:optional true} [:maybe [:sequential :any]]]]])
(mu/defn test-drill-application
"Test that a certain drill gets returned, AND when applied to a query returns the expected query."
[{:keys [expected-query drill-args], :as test-case} :- DrillApplicationTestCase]
(let [{:keys [query]} (query-and-row-for-test-case test-case)
drill (test-returns-drill test-case)]
(testing (str "Should return expected query when applying the drill"
"\nQuery = \n" (u/pprint-to-str query)
"\nDrill = \n" (u/pprint-to-str drill))
(let [query' (apply lib/drill-thru query -1 drill drill-args)]
(is (=? expected-query
query'))))))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment