From 7e38a48fca298805dc306801e0fec56a7f7d2864 Mon Sep 17 00:00:00 2001 From: github-automation-metabase <166700802+github-automation-metabase@users.noreply.github.com> Date: Wed, 14 Aug 2024 12:02:43 -0400 Subject: [PATCH] Enable type info computation for string id fields (#46488) (#46819) * Enable type info computation for fields coming from model source query * Add test * Add handling for fields coming from joins Co-authored-by: lbrdnk <lbrdnk@users.noreply.github.com> --- .../middleware/wrap_value_literals.clj | 30 +++++++++++++--- .../middleware/wrap_value_literals_test.clj | 36 ++++++++++++++++++- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/metabase/query_processor/middleware/wrap_value_literals.clj b/src/metabase/query_processor/middleware/wrap_value_literals.clj index 878cfff438e..e1a8355e13a 100644 --- a/src/metabase/query_processor/middleware/wrap_value_literals.clj +++ b/src/metabase/query_processor/middleware/wrap_value_literals.clj @@ -21,6 +21,12 @@ ;;; --------------------------------------------------- Type Info ---------------------------------------------------- +(def ^:private ^:dynamic *inner-query* + "To be bound in [[metabase.query-processor.middleware.wrap-value-literals/wrap-value-literals-in-mbql-query]]. + Original motivation is to provide metadata required for computation of _type info_. See the + [[metabase.query-processor.middleware.wrap-value-literals/str-id-field->type-info]] docstring for details." + nil) + (defmulti ^:private type-info "Get information about database, base, and semantic types for an object. This is passed to along to various `->honeysql` method implementations so drivers have the information they need to handle raw values like Strings, @@ -43,12 +49,27 @@ (when (types/temporal-field? field-info) {:unit :default})))) -(defmethod type-info :field [[_ id-or-name opts]] +(defn- str-id-field->type-info + "Return _type info_ for `_field` with string `field-name`, coming from the source query or joins." + [[_tag field-name {:keys [join-alias] :as _opts} :as _field] inner-query] + (when (string? field-name) + ;; Use corresponding source-metadata from joins or `inner-query`. + (let [source-metadatas (if join-alias + (some #(when (= join-alias (:alias %)) + (:source-metadata %)) + (:joins inner-query)) + (:source-metadata inner-query))] + (some #(when (= (:name %) field-name) + (select-keys % [:base_type :effective_type :database_type])) + source-metadatas)))) + +(defmethod type-info :field [[_ id-or-name opts :as field]] (merge ;; With Mlv2 queries, this could be combined with `:expression` below and use the column from the ;; query rather than metadata/field - (when (integer? id-or-name) - (type-info (lib.metadata/field (qp.store/metadata-provider) id-or-name))) + (if (integer? id-or-name) + (type-info (lib.metadata/field (qp.store/metadata-provider) id-or-name)) + (str-id-field->type-info field *inner-query*)) (when (:temporal-unit opts) {:unit (:temporal-unit opts)}) (when (:base-type opts) @@ -296,7 +317,8 @@ [{:keys [source-query], :as inner-query} options] (let [inner-query (cond-> inner-query source-query (update :source-query wrap-value-literals-in-mbql-query options))] - (wrap-value-literals-in-mbql inner-query))) + (binding [*inner-query* inner-query] + (wrap-value-literals-in-mbql inner-query)))) (mu/defn wrap-value-literals :- mbql.s/Query "Middleware that wraps ran value literals in `:value` (for integers, strings, etc.) or `:absolute-datetime` (for diff --git a/test/metabase/query_processor/middleware/wrap_value_literals_test.clj b/test/metabase/query_processor/middleware/wrap_value_literals_test.clj index 7e13adc00c8..162175f27d5 100644 --- a/test/metabase/query_processor/middleware/wrap_value_literals_test.clj +++ b/test/metabase/query_processor/middleware/wrap_value_literals_test.clj @@ -10,9 +10,11 @@ [metabase.lib.test-util :as lib.tu] [metabase.lib.test-util.macros :as lib.tu.macros] [metabase.query-processor.middleware.wrap-value-literals :as qp.wrap-value-literals] + [metabase.query-processor.preprocess :as qp.preprocess] [metabase.query-processor.store :as qp.store] [metabase.query-processor.timezone :as qp.timezone] - [metabase.test :as mt])) + [metabase.test :as mt] + [toucan2.tools.with-temp :as toucan2.with-temp])) (driver/register! ::tz-driver, :abstract? true) @@ -301,3 +303,35 @@ lib.convert/->legacy-MBQL wrap-value-literals (lib/query query)))))))) + +(deftest ^:parallel model-source-type-info-test + (testing "type info is added to fields coming from model source query (#46059)" + ;; Basically, this checks whether the [[metabase.query-processor.middleware.wrap-value-literals/type-info]] :field + ;; adds options to values in expressions, where other arg is field clause with name instead of int id. + (toucan2.with-temp/with-temp [:model/Card {id :id} {:dataset_query (mt/mbql-query venues) + :type :model}] + (let [query (mt/mbql-query + venues + {:source-table (str "card__" id) + :filter [:= [:field "ID" {:base-type :type/Integer}] 1]}) + preprocessed (qp.preprocess/preprocess query)] + ;; [:query :filter 2 2 :database_type] points to wrapped value's options + (is (= "BIGINT" (get-in preprocessed [:query :filter 2 2 :database_type]))))))) + +(deftest ^:parallel model-join-type-info-test + (testing "type info is added to fields coming from join" + ;; Basically, this checks whether the [[metabase.query-processor.middleware.wrap-value-literals/type-info]] :field + ;; adds options to values in expressions, where other arg is field clause with name instead of int id. + (toucan2.with-temp/with-temp [:model/Card {id :id} {:dataset_query (mt/mbql-query venues) + :type :model}] + (let [query (mt/mbql-query + venues + {:filter [:= [:field "ID" {:base-type :type/Integer :join-alias "x"}] 1] + :joins [{:alias "x" + :condition [:= + $id + [:field "ID" {:base-type :type/Integer :join-alias "x"}]] + :source-table (str "card__" id)}]}) + preprocessed (qp.preprocess/preprocess query)] + ;; [:query :filter 2 2 :database_type] points to wrapped value's options + (is (= "BIGINT" (get-in preprocessed [:query :filter 2 2 :database_type]))))))) -- GitLab