From 7866f7e20be21cb8d80db70b6ce7c583c97c6b68 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Wed, 3 Apr 2024 22:27:25 +0700
Subject: [PATCH] Fix unable to search field values from partitioned tables
(#40767)
---
.../driver/bigquery_cloud_sdk_test.clj | 101 ++++++++++++++++++
src/metabase/api/field.clj | 25 +----
src/metabase/db/metadata_queries.clj | 94 ++++++++++------
.../driver/common/parameters/dates.clj | 3 +-
.../driver/common/parameters/operators.clj | 4 +-
src/metabase/legacy_mbql/util.cljc | 30 ++++++
src/metabase/models/field_values.clj | 7 +-
src/metabase/models/params.clj | 34 +-----
src/metabase/models/params/chain_filter.clj | 7 +-
src/metabase/models/table.clj | 26 +++--
.../middleware/check_features.clj | 10 +-
.../middleware/parameters/mbql.clj | 7 +-
.../sync/analyze/fingerprint/insights.clj | 3 +-
src/metabase/sync/util.clj | 3 +-
test/metabase/db/metadata_queries_test.clj | 53 ++++++++-
test/metabase/models/params_test.clj | 3 +-
.../query_processor_test/aggregation_test.clj | 2 +-
17 files changed, 282 insertions(+), 130 deletions(-)
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
index 9626e5a5ef1..fb00db41876 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
@@ -12,6 +12,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.compile :as qp.compile]
[metabase.query-processor.pipeline :as qp.pipeline]
+ [metabase.query-processor.test-util :as qp.test-util]
[metabase.sync :as sync]
[metabase.test :as mt]
[metabase.test.data.bigquery-cloud-sdk :as bigquery.tx]
@@ -419,6 +420,106 @@
(finally
(drop-table-if-exists! table-name))))))))
+(deftest search-field-from-table-requires-a-filter-test
+ (testing "#40673"
+ (mt/test-driver :bigquery-cloud-sdk
+ (mt/with-model-cleanup [:model/Table]
+ (let [partitioned-table "fv_partitioned_table"]
+ (try
+ (doseq [sql [(format "CREATE TABLE %s (id INT64, category STRING)
+ PARTITION BY _PARTITIONDATE
+ OPTIONS (require_partition_filter = TRUE);"
+ (fmt-table-name partitioned-table))
+ (format "INSERT INTO %s (id, category)
+ VALUES (1, \"coffee\"), (2, \"tea\"), (3, \"matcha\");"
+ (fmt-table-name partitioned-table))]]
+ (bigquery.tx/execute! sql))
+ (sync/sync-database! (mt/db) {:scan :schema})
+ (let [category-field-id (mt/id :fv_partitioned_table :category)]
+ (t2/update! :model/Field category-field-id {:has_field_values :search})
+ (t2/delete! :model/FieldValues :field_id category-field-id)
+ (= [["coffee"]]
+ (mt/user-http-request :crowberto :get 200 (format "/field/%d/search/%d" category-field-id category-field-id)
+ :value "co")))
+ (finally
+ (drop-table-if-exists! partitioned-table))))))))
+
+(deftest chain-filter-with-fields-from-table-requires-a-filter-test
+ (testing "#40673"
+ (mt/test-driver :bigquery-cloud-sdk
+ (binding [qp.test-util/*enable-fk-support-for-disabled-drivers-in-tests* true]
+ (mt/with-model-cleanup [:model/Table]
+ (let [category-table-name "cf_category"
+ product-table-name "cf_product"]
+ (try
+ (doseq [sql [(format "CREATE TABLE %s (id INT64, category STRING, PRIMARY KEY(id) NOT ENFORCED)
+ PARTITION BY _PARTITIONDATE
+ OPTIONS (require_partition_filter = TRUE);"
+ (fmt-table-name category-table-name))
+ (format "INSERT INTO %s (id, category)
+ VALUES (1, \"coffee\"), (2, \"tea\");"
+ (fmt-table-name category-table-name))
+ (format "CREATE TABLE %s (id INT64, category_id INT64, name STRING)
+ PARTITION BY _PARTITIONDATE
+ OPTIONS (require_partition_filter = TRUE);"
+ (fmt-table-name product-table-name))
+ (format "ALTER TABLE %1$s
+ ADD CONSTRAINT fk_product_category_id FOREIGN KEY (category_id)
+ REFERENCES %2$s(id) NOT ENFORCED;"
+ (fmt-table-name product-table-name)
+ (fmt-table-name category-table-name))
+ (format "INSERT INTO %s (id, category_id, name)
+ VALUES (1, 1, \"Americano\"), (2, 1, \"Cold brew\"), (3, 2, \"Herbal\"), (4, 2, \"Oolong\");"
+ (fmt-table-name product-table-name))]]
+ (bigquery.tx/execute! sql))
+ (sync/sync-database! (mt/db) {:scan :schema})
+ ;; Fake fk relationship for bigquery because apparently fk on bigquery is not a thing.
+ ;; We want this to test whether chain filter add a filter on partitioned fields from joned tables.
+ (t2/update! :model/Field (mt/id :cf_product :category_id) {:fk_target_field_id (mt/id :cf_category :id)})
+ (mt/with-temp
+ [:model/Card card-category {:database_id (mt/id)
+ :table_id (mt/id :cf_category)
+ :dataset_query (mt/mbql-query cf_category)}
+ :model/Card card-product {:database_id (mt/id)
+ :table_id (mt/id :cf_product)
+ :dataset_query (mt/mbql-query cf_product)}
+ :model/Dashboard dashboard {:parameters [{:name "Category"
+ :slug "category"
+ :id "_CATEGORY_"
+ :type :string/=}
+ {:name "Product Name"
+ :slug "Product name"
+ :id "_NAME_"
+ :type :string/=}]}
+ :model/DashboardCard _dashcard {:card_id (:id card-category)
+ :dashboard_id (:id dashboard)
+ :parameter_mappings [{:parameter_id "_CATEGORY_"
+ :card_id (:id card-category)
+ :target [:dimension (mt/$ids $cf_category.category)]}]}
+ :model/DashboardCard _dashcard {:card_id (:id card-product)
+ :dashboard_id (:id dashboard)
+ :parameter_mappings [{:parameter_id "_NAME_"
+ :card_id (:id card-product)
+ :target [:dimension (mt/$ids $cf_product.name)]}]}]
+
+ (testing "chained filter works"
+ (is (= {:has_more_values false
+ :values [["Americano"] ["Cold brew"]]}
+ (mt/user-http-request :crowberto :get 200 (format "/dashboard/%d/params/%s/values?%s=%s"
+ (:id dashboard) "_NAME_" "_CATEGORY_" "coffee")))))
+ (testing "getting values works"
+ (is (= {:has_more_values false
+ :values [["Americano"] ["Cold brew"] ["Herbal"] ["Oolong"]]}
+ (mt/user-http-request :crowberto :get 200 (format "/dashboard/%d/params/%s/values" (:id dashboard) "_NAME_")))))
+ (testing "searching values works"
+ (is (= {:has_more_values false
+ :values [["Oolong"]]}
+ (mt/user-http-request :crowberto :get 200 (format "/dashboard/%d/params/%s/search/oo" (:id dashboard) "_NAME_"))))))
+
+ (finally
+ (doseq [table-name [product-table-name category-table-name]]
+ (drop-table-if-exists! table-name))))))))))
+
(deftest query-integer-pk-or-fk-test
(mt/test-driver :bigquery-cloud-sdk
(testing "We should be able to query a Table that has a :type/Integer column marked as a PK or FK"
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index 6d6743f29bf..330f15b8831 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -351,26 +351,6 @@
(t2/select-one Field :id fk-target-field-id)
field))
-(defn- search-values-query
- "Generate the MBQL query used to power FieldValues search in [[search-values]] below. The actual query generated
- differs slightly based on whether the two Fields are the same Field.
-
- Note: the generated MBQL query assume that both `field` and `search-field` are from the same table."
- [field search-field value limit]
- {:database (db-id field)
- :type :query
- :query {:source-table (table-id field)
- :filter (when (some? value)
- [:contains [:field (u/the-id search-field) nil] value {:case-sensitive false}])
- ;; if both fields are the same then make sure not to refer to it twice in the `:breakout` clause.
- ;; Otherwise this will break certain drivers like BigQuery that don't support duplicate
- ;; identifiers/aliases
- :breakout (if (= (u/the-id field) (u/the-id search-field))
- [[:field (u/the-id field) nil]]
- [[:field (u/the-id field) nil]
- [:field (u/the-id search-field) nil]])
- :limit limit}})
-
(mu/defn search-values :- [:maybe ms/FieldValuesList]
"Search for values of `search-field` that contain `value` (up to `limit`, if specified), and return pairs like
@@ -398,9 +378,8 @@
(try
(let [field (follow-fks field)
search-field (follow-fks search-field)
- limit (or maybe-limit default-max-field-search-limit)
- results (qp/process-query (search-values-query field search-field value limit))]
- (get-in results [:data :rows]))
+ limit (or maybe-limit default-max-field-search-limit)]
+ (metadata-queries/search-values-query field search-field value limit))
(catch Throwable e
(log/error e (trs "Error searching field values"))
[]))))
diff --git a/src/metabase/db/metadata_queries.clj b/src/metabase/db/metadata_queries.clj
index 25c852a7b3d..286268e0de7 100644
--- a/src/metabase/db/metadata_queries.clj
+++ b/src/metabase/db/metadata_queries.clj
@@ -8,7 +8,6 @@
[metabase.driver.util :as driver.u]
[metabase.legacy-mbql.schema :as mbql.s]
[metabase.legacy-mbql.schema.helpers :as helpers]
- [metabase.models.table :as table]
[metabase.query-processor :as qp]
[metabase.query-processor.interface :as qp.i]
[metabase.util :as u]
@@ -16,7 +15,8 @@
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
-(defn- qp-query [db-id mbql-query]
+(defn- qp-query
+ [db-id mbql-query]
{:pre [(integer? db-id)]}
(-> (binding [qp.i/*disable-qp-logging* true]
(qp/process-query
@@ -36,39 +36,44 @@
:type/Date [:> field-form "0001-01-01"]
:type/DateTime [:> field-form "0001-01-01T00:00:00"])))
-(defn- query-with-default-partitioned-field-filter
- [query table-id]
- (let [;; In bigquery, range or datetime partitioned table can have only one partioned field,
- ;; Ingestion time partitioned table can use either _PARTITIONDATE or _PARTITIONTIME as
- ;; partitioned field
- partition-field (or (t2/select-one :model/Field
- :table_id table-id
- :database_partitioned true
- :active true
- ;; prefer _PARTITIONDATE over _PARTITIONTIME for ingestion time query
- {:order-by [[:name :asc]]})
- (throw (ex-info (format "No partitioned field found for table: %d" table-id)
- {:table_id table-id})))
- filter-form (partition-field->filter-form partition-field)]
- (update query :filter (fn [existing-filter]
- (if (some? existing-filter)
- [:and existing-filter filter-form]
- filter-form)))))
+(defn add-required-filters-if-needed
+ "Add a dummy filter for tables that require filters.
+ Look into tables from source tables and all the joins.
+ Currently this only apply to partitioned tables on bigquery that requires a partition filter.
+ In the future we probably want this to be dispatched by database engine or handled by QP."
+ [query]
+ (let [table-ids (->> (conj (keep :source-table (:joins query)) (:source-table query))
+ (filter pos-int?))
+ required-filter-fields (when (seq table-ids)
+ (t2/select :model/Field {:select [:f.*]
+ :from [[:metabase_field :f]]
+ :left-join [[:metabase_table :t] [:= :t.id :f.table_id]]
+ :where [:and
+ [:= :f.active true]
+ [:= :f.database_partitioned true]
+ [:= :t.active true]
+ [:= :t.database_require_filter true]
+ [:in :t.id table-ids]]}))
+ update-query-filter-fn (fn [existing-filter new-filter]
+ (if (some? existing-filter)
+ [:and existing-filter new-filter]
+ new-filter))]
+ (case (count required-filter-fields)
+ 0
+ query
+ 1
+ (update query :filter update-query-filter-fn (partition-field->filter-form (first required-filter-fields)))
+ ;; > 1
+ (update query :filter update-query-filter-fn (into [:and] (map partition-field->filter-form required-filter-fields))))))
(defn- field-mbql-query
[table mbql-query]
- (cond-> mbql-query
- true
- (assoc :source-table (:id table))
-
- ;; Some table requires a filter to be able to query the data
- ;; Currently this only applied to Partitioned table in bigquery where the partition field
- ;; is required as a filter.
- ;; In the future we probably want this to be dispatched by database engine type
- (:database_require_filter table)
- (query-with-default-partitioned-field-filter (:id table))))
-
-(defn- field-query [{table-id :table_id} mbql-query]
+ (-> mbql-query
+ (assoc :source-table (:id table))
+ add-required-filters-if-needed))
+
+(defn- field-query
+ [{table-id :table_id} mbql-query]
{:pre [(integer? table-id)]}
(let [table (t2/select-one :model/Table :id table-id)]
(qp-query (:db_id table)
@@ -104,6 +109,25 @@
(field-query field {:breakout [[:field (u/the-id field) nil]]
:limit (min max-results absolute-max-distinct-values-limit)})))
+;; I'm not sure whether this field-distinct-values and field-distinct-values belong to this namespace
+;; maybe it's better to keep this in metabase.models.field or metabase.models.field-values
+(defn search-values-query
+ "Generate the MBQL query used to power FieldValues search in [[metabase.api.field/search-values]]. The actual query generated
+ differs slightly based on whether the two Fields are the same Field.
+
+ Note: the generated MBQL query assume that both `field` and `search-field` are from the same table."
+ [field search-field value limit]
+ (field-query field {:filter (when (some? value)
+ [:contains [:field (u/the-id search-field) nil] value {:case-sensitive false}])
+ ;; if both fields are the same then make sure not to refer to it twice in the `:breakout` clause.
+ ;; Otherwise this will break certain drivers like BigQuery that don't support duplicate
+ ;; identifiers/aliases
+ :breakout (if (= (u/the-id field) (u/the-id search-field))
+ [[:field (u/the-id field) nil]]
+ [[:field (u/the-id field) nil]
+ [:field (u/the-id search-field) nil]])
+ :limit limit}))
+
(defn field-distinct-count
"Return the distinct count of `field`."
[field & [limit]]
@@ -149,7 +173,7 @@
[table
fields
{:keys [truncation-size limit order-by] :or {limit max-sample-rows} :as _opts}]
- (let [database (table/database table)
+ (let [database (t2/select-one :model/Database (:db_id table))
driver (driver.u/database->driver database)
text-fields (filter text-field? fields)
field->expressions (when (and truncation-size (driver/database-supports? driver :expressions database))
@@ -169,8 +193,8 @@
order-by
(assoc :order-by order-by)
- (:database_require_filter table)
- (query-with-default-partitioned-field-filter (:id table)))
+ true
+ add-required-filters-if-needed)
:middleware {:format-rows? false
:skip-results-metadata? true}}))
diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj
index 54c5cd6a75b..bae7440dbec 100644
--- a/src/metabase/driver/common/parameters/dates.clj
+++ b/src/metabase/driver/common/parameters/dates.clj
@@ -7,7 +7,6 @@
[metabase.legacy-mbql.schema :as mbql.s]
[metabase.legacy-mbql.util :as mbql.u]
[metabase.lib.schema.parameter :as lib.schema.parameter]
- [metabase.models.params :as params]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [tru]]
@@ -473,7 +472,7 @@
returns a corresponding MBQL filter clause for a given field reference."
[date-string :- :string
field :- [:or ms/PositiveInt mbql.s/Field]]
- (or (execute-decoders all-date-string-decoders :filter (params/wrap-field-id-if-needed field) date-string)
+ (or (execute-decoders all-date-string-decoders :filter (mbql.u/wrap-field-id-if-needed field) date-string)
(throw (ex-info (tru "Don''t know how to parse date string {0}" (pr-str date-string))
{:type qp.error-type/invalid-parameter
:date-string date-string}))))
diff --git a/src/metabase/driver/common/parameters/operators.clj b/src/metabase/driver/common/parameters/operators.clj
index af36c58df1f..7ba42202953 100644
--- a/src/metabase/driver/common/parameters/operators.clj
+++ b/src/metabase/driver/common/parameters/operators.clj
@@ -9,8 +9,8 @@
:value [3 5]}"
(:require
[metabase.legacy-mbql.schema :as mbql.s]
+ [metabase.legacy-mbql.util :as mbql.u]
[metabase.lib.schema.parameter :as lib.schema.parameter]
- [metabase.models.params :as params]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.util.i18n :refer [tru]]
[metabase.util.malli :as mu]))
@@ -60,7 +60,7 @@
`:type qp.error-type/invalid-parameter` if arity is incorrect."
[{param-type :type [a b :as param-value] :value [_ field :as _target] :target options :options :as _param}]
(verify-type-and-arity field param-type param-value)
- (let [field' (params/wrap-field-id-if-needed field)]
+ (let [field' (mbql.u/wrap-field-id-if-needed field)]
(case (operator-arity param-type)
:binary
(cond-> [(keyword (name param-type)) field' a b]
diff --git a/src/metabase/legacy_mbql/util.cljc b/src/metabase/legacy_mbql/util.cljc
index 148e479f4cc..9c88ccdcf5d 100644
--- a/src/metabase/legacy_mbql/util.cljc
+++ b/src/metabase/legacy_mbql/util.cljc
@@ -762,3 +762,33 @@
(sequential? form) (recur (onto-stack (map-indexed vector form)) matches)
:else (recur stack matches)))
matches)))
+
+(defn wrap-field-id-if-needed
+ "Wrap a raw Field ID in a `:field` clause if needed."
+ [field-id-or-form]
+ (cond
+ (mbql-clause? field-id-or-form)
+ field-id-or-form
+
+ (integer? field-id-or-form)
+ [:field field-id-or-form nil]
+
+ :else
+ field-id-or-form))
+
+(mu/defn unwrap-field-clause :- [:maybe mbql.s/field]
+ "Unwrap something that contains a `:field` clause, such as a template tag.
+ Also handles unwrapped integers for legacy compatibility.
+
+ (unwrap-field-clause [:field 100 nil]) ; -> [:field 100 nil]"
+ [field-form]
+ (if (integer? field-form)
+ [:field field-form nil]
+ (lib.util.match/match-one field-form :field)))
+
+(mu/defn unwrap-field-or-expression-clause :- mbql.s/Field
+ "Unwrap a `:field` clause or expression clause, such as a template tag. Also handles unwrapped integers for
+ legacy compatibility."
+ [field-or-ref-form]
+ (or (unwrap-field-clause field-or-ref-form)
+ (lib.util.match/match-one field-or-ref-form :expression)))
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index 9b9eb8fc464..c42e2727bb5 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -27,10 +27,10 @@
[java-time.api :as t]
[malli.core :as mc]
[medley.core :as m]
+ [metabase.db.metadata-queries :as metadata-queries]
[metabase.db.query :as mdb.query]
[metabase.models.interface :as mi]
[metabase.models.serialization :as serdes]
- [metabase.plugins.classloader :as classloader]
[metabase.public-settings.premium-features :refer [defenterprise]]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
@@ -343,9 +343,8 @@
FieldValues. You most likely should not be using this directly in code outside of this namespace, unless it's for a
very specific reason, such as certain cases where we fetch ad-hoc FieldValues for GTAP-filtered Fields.)"
[field]
- (classloader/require 'metabase.db.metadata-queries)
(try
- (let [distinct-values ((resolve 'metabase.db.metadata-queries/field-distinct-values) field)
+ (let [distinct-values (metadata-queries/field-distinct-values field)
limited-distinct-values (take-by-length *total-max-length* distinct-values)]
{:values limited-distinct-values
;; has_more_values=true means the list of values we return is a subset of all possible values.
@@ -359,7 +358,7 @@
;; So, if the returned `distinct-values` has length equal to that exact limit,
;; we assume the returned values is just a subset of what we have in DB.
(= (count distinct-values)
- @(resolve 'metabase.db.metadata-queries/absolute-max-distinct-values-limit)))})
+ metadata-queries/absolute-max-distinct-values-limit))})
(catch Throwable e
(log/error e (trs "Error fetching field values"))
nil)))
diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj
index f851304d887..f93700a7a1b 100644
--- a/src/metabase/models/params.clj
+++ b/src/metabase/models/params.clj
@@ -47,36 +47,6 @@
(throw (ex-info (tru ":parameter_mappings must be a sequence of maps with :parameter_id and :type keys")
{:parameter_mappings parameter_mappings}))))
-(mu/defn unwrap-field-clause :- [:maybe mbql.s/field]
- "Unwrap something that contains a `:field` clause, such as a template tag.
- Also handles unwrapped integers for legacy compatibility.
-
- (unwrap-field-clause [:field 100 nil]) ; -> [:field 100 nil]"
- [field-form]
- (if (integer? field-form)
- [:field field-form nil]
- (lib.util.match/match-one field-form :field)))
-
-(mu/defn unwrap-field-or-expression-clause :- mbql.s/Field
- "Unwrap a `:field` clause or expression clause, such as a template tag. Also handles unwrapped integers for
- legacy compatibility."
- [field-or-ref-form]
- (or (unwrap-field-clause field-or-ref-form)
- (lib.util.match/match-one field-or-ref-form :expression)))
-
-(defn wrap-field-id-if-needed
- "Wrap a raw Field ID in a `:field` clause if needed."
- [field-id-or-form]
- (cond
- (mbql.u/mbql-clause? field-id-or-form)
- field-id-or-form
-
- (integer? field-id-or-form)
- [:field field-id-or-form nil]
-
- :else
- field-id-or-form))
-
(def ^:dynamic *ignore-current-user-perms-and-return-all-field-values*
"Whether to ignore permissions for the current User and return *all* FieldValues for the Fields being parameterized by
Cards and Dashboards. This determines how `:param_values` gets hydrated for Card and Dashboard. Normally, this is
@@ -123,7 +93,7 @@
;; for unknown reasons. See #8917
(if field-form
(try
- (unwrap-field-or-expression-clause field-form)
+ (mbql.u/unwrap-field-or-expression-clause field-form)
(catch Exception e
(log/error e "Failed unwrap field form" field-form)))
(log/error "Could not find matching field clause for target:" target))))))
@@ -298,7 +268,7 @@
[card]
(set (for [[_ {dimension :dimension}] (get-in card [:dataset_query :native :template-tags])
:when dimension
- :let [field (unwrap-field-clause dimension)]
+ :let [field (mbql.u/unwrap-field-clause dimension)]
:when field]
field)))
diff --git a/src/metabase/models/params/chain_filter.clj b/src/metabase/models/params/chain_filter.clj
index abf2e54b9ac..8777bde0a21 100644
--- a/src/metabase/models/params/chain_filter.clj
+++ b/src/metabase/models/params/chain_filter.clj
@@ -67,6 +67,7 @@
[clojure.string :as str]
[honey.sql :as sql]
[metabase.db :as mdb]
+ [metabase.db.metadata-queries :as metadata-queries]
[metabase.db.query :as mdb.query]
[metabase.driver.common.parameters.dates :as params.dates]
[metabase.legacy-mbql.util :as mbql.u]
@@ -418,15 +419,15 @@
;; TODO -- would this be more efficient if we just did an INNER JOIN against the original
;; Table instead of a LEFT JOIN with this additional filter clause? Would that still
;; work?
- :filter [:not-null original-field-clause]
+ :filter [:not-null original-field-clause]
;; for Field->Field remapping we want to return pairs of [original-value remapped-value],
;; but sort by [remapped-value]
:order-by [[:asc [:field field-id nil]]]}))
(add-joins source-table-id joins)
- (add-filters source-table-id joined-table-ids constraints)))
+ (add-filters source-table-id joined-table-ids constraints)
+ metadata-queries/add-required-filters-if-needed))
:middleware {:disable-remaps? true}})
-
;;; ------------------------ Chain filter (powers GET /api/dashboard/:id/params/:key/values) -------------------------
(mu/defn ^:private unremapped-chain-filter :- ms/FieldValuesResult
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index f31bd78dede..7b47e42c96b 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -6,8 +6,6 @@
[metabase.driver :as driver]
[metabase.models.audit-log :as audit-log]
[metabase.models.data-permissions :as data-perms]
- [metabase.models.database :refer [Database]]
- [metabase.models.field :refer [Field]]
[metabase.models.humanization :as humanization]
[metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
@@ -61,7 +59,7 @@
(t2/define-before-insert :model/Table
[table]
(let [defaults {:display_name (humanization/name->human-readable-name (:name table))
- :field_order (driver/default-field-order (t2/select-one-fn :engine Database :id (:db_id table)))}]
+ :field_order (driver/default-field-order (t2/select-one-fn :engine :model/Database :id (:db_id table)))}]
(merge defaults table)))
(defn- set-new-table-permissions!
@@ -128,9 +126,9 @@
[table]
(doall
(map-indexed (fn [new-position field]
- (t2/update! Field (u/the-id field) {:position new-position}))
+ (t2/update! :model/Field (u/the-id field) {:position new-position}))
;; Can't use `select-field` as that returns a set while we need an ordered list
- (t2/select [Field :id]
+ (t2/select [:model/Field :id]
:table_id (u/the-id table)
{:order-by (case (:field_order table)
:custom [[:custom_position :asc]]
@@ -147,7 +145,7 @@
(defn- valid-field-order?
"Field ordering is valid if all the fields from a given table are present and only from that table."
[table field-ordering]
- (= (t2/select-pks-set Field
+ (= (t2/select-pks-set :model/Field
:table_id (u/the-id table)
:active true)
(set field-ordering)))
@@ -159,8 +157,8 @@
(t2/update! Table (u/the-id table) {:field_order :custom})
(doall
(map-indexed (fn [position field-id]
- (t2/update! Field field-id {:position position
- :custom_position position}))
+ (t2/update! :model/Field field-id {:position position
+ :custom_position position}))
field-order)))
@@ -221,7 +219,7 @@
[tables]
(with-objects :fields
(fn [table-ids]
- (t2/select Field
+ (t2/select :model/Field
:active true
:table_id [:in table-ids]
:visibility_type [:not= "retired"]
@@ -239,14 +237,14 @@
(defn database
"Return the `Database` associated with this `Table`."
[table]
- (t2/select-one Database :id (:db_id table)))
+ (t2/select-one :model/Database :id (:db_id table)))
;;; ------------------------------------------------- Serialization -------------------------------------------------
(defmethod serdes/dependencies "Table" [table]
[[{:model "Database" :id (:db_id table)}]])
(defmethod serdes/generate-path "Table" [_ table]
- (let [db-name (t2/select-one-fn :name 'Database :id (:db_id table))]
+ (let [db-name (t2/select-one-fn :name :model/Database :id (:db_id table))]
(filterv some? [{:model "Database" :id db-name}
(when (:schema table)
{:model "Schema" :id (:schema table)})
@@ -261,18 +259,18 @@
schema-name (when (= 3 (count path))
(-> path second :id))
table-name (-> path last :id)
- db-id (t2/select-one-pk Database :name db-name)]
+ db-id (t2/select-one-pk :model/Database :name db-name)]
(t2/select-one Table :name table-name :db_id db-id :schema schema-name)))
(defmethod serdes/extract-one "Table"
[_model-name _opts {:keys [db_id] :as table}]
(-> (serdes/extract-one-basics "Table" table)
- (assoc :db_id (t2/select-one-fn :name 'Database :id db_id))))
+ (assoc :db_id (t2/select-one-fn :name :model/Database :id db_id))))
(defmethod serdes/load-xform "Table"
[{:keys [db_id] :as table}]
(-> (serdes/load-xform-basics table)
- (assoc :db_id (t2/select-one-fn :id 'Database :name db_id))))
+ (assoc :db_id (t2/select-one-fn :id :model/Database :name db_id))))
(defmethod serdes/storage-path "Table" [table _ctx]
(concat (serdes/storage-table-path-prefix (serdes/path table))
diff --git a/src/metabase/query_processor/middleware/check_features.clj b/src/metabase/query_processor/middleware/check_features.clj
index bc211c20f80..bafb59ab4f7 100644
--- a/src/metabase/query_processor/middleware/check_features.clj
+++ b/src/metabase/query_processor/middleware/check_features.clj
@@ -12,10 +12,12 @@
(defn assert-driver-supports
"Assert that the driver/database supports keyword `feature`."
[feature]
- (when-not (driver/database-supports? driver/*driver* feature (lib.metadata/database (qp.store/metadata-provider)))
- (throw (ex-info (tru "{0} is not supported by this driver." (name feature))
- {:type qp.error-type/unsupported-feature
- :feature feature}))))
+ (let [database (lib.metadata/database (qp.store/metadata-provider))]
+ (when-not (driver/database-supports? driver/*driver* feature database)
+ (throw (ex-info (tru "{0} is not supported by {1} driver." (name feature) (name driver/*driver*))
+ {:type qp.error-type/unsupported-feature
+ :feature feature
+ :driver driver/*driver*})))))
;; TODO - definitely a little incomplete. It would be cool if we cool look at the metadata in the schema namespace and
;; auto-generate this logic
diff --git a/src/metabase/query_processor/middleware/parameters/mbql.clj b/src/metabase/query_processor/middleware/parameters/mbql.clj
index 9b8c77901ff..2b032d7aa7c 100644
--- a/src/metabase/query_processor/middleware/parameters/mbql.clj
+++ b/src/metabase/query_processor/middleware/parameters/mbql.clj
@@ -9,7 +9,6 @@
[metabase.lib.core :as lib]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.lib.util.match :as lib.util.match]
- [metabase.models.params :as params]
[metabase.query-processor.store :as qp.store]
[metabase.util.malli :as mu]))
@@ -74,7 +73,7 @@
;; single value, date range. Generate appropriate MBQL clause based on date string
(params.dates/date-type? param-type)
(params.dates/date-string->filter
- (parse-param-value-for-type query param-type param-value (params/unwrap-field-or-expression-clause field))
+ (parse-param-value-for-type query param-type param-value (mbql.u/unwrap-field-or-expression-clause field))
field)
;; TODO - We can't tell the difference between a dashboard parameter (convert to an MBQL filter) and a native
@@ -86,8 +85,8 @@
;; single-value, non-date param. Generate MBQL [= [field <field> nil] <value>] clause
:else
[:=
- (params/wrap-field-id-if-needed field)
- (parse-param-value-for-type query param-type param-value (params/unwrap-field-or-expression-clause field))]))
+ (mbql.u/wrap-field-id-if-needed field)
+ (parse-param-value-for-type query param-type param-value (mbql.u/unwrap-field-or-expression-clause field))]))
(defn expand
"Expand parameters for MBQL queries in `query` (replacing Dashboard or Card-supplied params with the appropriate
diff --git a/src/metabase/sync/analyze/fingerprint/insights.clj b/src/metabase/sync/analyze/fingerprint/insights.clj
index 77dca8ba96f..8936c12225f 100644
--- a/src/metabase/sync/analyze/fingerprint/insights.clj
+++ b/src/metabase/sync/analyze/fingerprint/insights.clj
@@ -7,7 +7,6 @@
[kixi.stats.protocols :as p]
[medley.core :as m]
[metabase.legacy-mbql.util :as mbql.u]
- [metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
[metabase.sync.analyze.fingerprint.fingerprinters :as fingerprinters]
[metabase.sync.util :as sync-util]
@@ -224,7 +223,7 @@
:col (:name number-col)
:unit unit))))))
(format "Error generating timeseries insight keyed by: %s"
- (sync-util/name-for-logging (mi/instance Field datetime))))))
+ (sync-util/name-for-logging (mi/instance :model/Field datetime))))))
(defn insights
"Based on the shape of returned data construct a transducer to statistically analyize data."
diff --git a/src/metabase/sync/util.clj b/src/metabase/sync/util.clj
index 539f885bcf8..a5c8153bf61 100644
--- a/src/metabase/sync/util.clj
+++ b/src/metabase/sync/util.clj
@@ -9,7 +9,6 @@
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.events :as events]
- [metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
[metabase.models.task-history :refer [TaskHistory]]
[metabase.query-processor.interface :as qp.i]
@@ -379,7 +378,7 @@
[& {:keys [id name]}]
(format "Field %s ''%s''" (or (str id) "") name))
-(defmethod name-for-logging Field [field]
+(defmethod name-for-logging :model/Field [field]
(field-name-for-logging field))
;;; this is used for result metadata stuff.
diff --git a/test/metabase/db/metadata_queries_test.clj b/test/metabase/db/metadata_queries_test.clj
index bd34a7f2ae0..5bf40ed3900 100644
--- a/test/metabase/db/metadata_queries_test.clj
+++ b/test/metabase/db/metadata_queries_test.clj
@@ -89,7 +89,7 @@
(is (=? [:> [:field (:id field2) {:base-type :type/Integer}] (mt/malli=? int?)]
(get (#'metadata-queries/field-mbql-query table {}) :filter))))))
-(deftest text-field?-test
+(deftest ^:parallel text-field?-test
(testing "recognizes fields suitable for fingerprinting"
(doseq [field [{:base_type :type/Text}
{:base_type :type/Text :semantic_type :type/State}
@@ -99,3 +99,54 @@
{:base_type :type/Text :semantic_type :type/SerializedJSON} ; "legacy" json fields in pg
{:base_type :type/Text :semantic_type :type/XML}]]
(is (not (#'metadata-queries/text-field? field))))))
+
+(deftest add-required-filter-if-needed-test
+ (mt/with-temp
+ [:model/Database db {:engine :h2}
+ :model/Table product {:name "PRODUCT" :db_id (:id db)}
+ :model/Field _product-id {:name "ID" :table_id (:id product) :base_type :type/Integer}
+ :model/Field _product-name {:name "NAME" :table_id (:id product) :base_type :type/Integer}
+ :model/Table buyer {:name "BUYER" :database_require_filter true :db_id (:id db)}
+ :model/Field buyer-id {:name "ID" :table_id (:id buyer) :base_type :type/Integer :database_partitioned true}
+ :model/Table order {:name "ORDER" :database_require_filter true :db_id (:id db)}
+ :model/Field order-id {:name "ID" :table_id (:id order) :base_type :type/Integer}
+ :model/Field _order-buyer-id {:name "BUYER_ID" :table_id (:id order) :base_type :type/Integer}
+ :model/Field order-product-id {:name "PRODUCT_ID" :table_id (:id order) :base_type :type/Integer :database_partitioned true}]
+ (mt/with-db db
+ (mt/$ids
+ (testing "no op for tables that do not require filter"
+ (let [query (:query (mt/mbql-query product))]
+ (is (= query
+ (metadata-queries/add-required-filter-if-needed query)))))
+
+ (testing "if the source table requires a filter, add the partitioned filter"
+ (let [query (:query (mt/mbql-query order))]
+ (is (= (assoc query
+ :filter [:> [:field (:id order-product-id) {:base-type :type/Integer}] -9223372036854775808])
+ (metadata-queries/add-required-filters-if-needed query)))))
+
+ (testing "if a joined table require a filter, add the partitioned filter"
+ (let [query (:query (mt/mbql-query product {:joins [{:source-table (:id order)
+ :condition [:= $order.product_id $product.id]
+ :alias "Product"}]}))]
+ (is (= (assoc query
+ :filter [:> [:field (:id order-product-id) {:base-type :type/Integer}] -9223372036854775808])
+ (metadata-queries/add-required-filters-if-needed query)))))
+
+ (testing "if both source tables and joined table require a filter, add both"
+ (let [query (:query (mt/mbql-query order {:joins [{:source-table (:id buyer)
+ :condition [:= $order.buyer_id $buyer.id]
+ :alias "BUYER"}]}))]
+ (is (= (assoc query
+ :filter [:and
+ [:> [:field (:id buyer-id) {:base-type :type/Integer}] -9223372036854775808]
+ [:> [:field (:id order-product-id) {:base-type :type/Integer}] -9223372036854775808]])
+ (metadata-queries/add-required-filters-if-needed query)))))
+
+ (testing "Should add an and clause for existing filter"
+ (let [query (:query (mt/mbql-query order {:filter [:> $order.id 1]}))]
+ (is (= (assoc query
+ :filter [:and
+ [:> [:field (:id order-id) nil] 1]
+ [:> [:field (:id order-product-id) {:base-type :type/Integer}] -9223372036854775808]])
+ (metadata-queries/add-required-filters-if-needed query)))))))))
diff --git a/test/metabase/models/params_test.clj b/test/metabase/models/params_test.clj
index 401acf55356..baa1078db5b 100644
--- a/test/metabase/models/params_test.clj
+++ b/test/metabase/models/params_test.clj
@@ -3,6 +3,7 @@
(:require
[clojure.test :refer :all]
[metabase.api.public-test :as public-test]
+ [metabase.legacy-mbql.util :as mbql.u]
[metabase.models :refer [Card Field]]
[metabase.models.params :as params]
[metabase.test :as mt]
@@ -15,7 +16,7 @@
[:field "name" {:base-type :type/Text}] [:field "name" {:base-type :type/Text}]}]
(testing x
(is (= expected
- (params/wrap-field-id-if-needed x))))))
+ (mbql.u/wrap-field-id-if-needed x))))))
;;; ---------------------------------------------- name_field hydration ----------------------------------------------
diff --git a/test/metabase/query_processor_test/aggregation_test.clj b/test/metabase/query_processor_test/aggregation_test.clj
index 08a501f6450..662366a19e0 100644
--- a/test/metabase/query_processor_test/aggregation_test.clj
+++ b/test/metabase/query_processor_test/aggregation_test.clj
@@ -73,7 +73,7 @@
(testing "Make sure standard deviations fail for drivers that don't support it"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"standard-deviation-aggregations is not supported by this driver"
+ #"standard-deviation-aggregations is not supported by sqlite driver"
(mt/run-mbql-query venues
{:aggregation [[:stddev $latitude]]}))))))
--
GitLab