diff --git a/modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj b/modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj
index ff5d155e67973be6a05e2d4856a85d6ca646c00a..236a65fb75c6b3a3c23e339c3c53c9236f4afeba 100644
--- a/modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj
+++ b/modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj
@@ -301,6 +301,7 @@
:name "ga:eventLabel"
:settings nil
:source :breakout
+ :nfc_path nil
:parent_id nil
:visibility_type :normal
:display_name "ga:eventLabel"
diff --git a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
index ae0509e3c50ac60c78a042a09297137c8a137545..bc7e85cbbb8206b16f5482a309d9e64abd8df2aa 100644
--- a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
+++ b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
@@ -121,6 +121,7 @@
:settings nil
:source :fields
:field_ref [:field (mt/id :extsales :buyerid) nil]
+ :nfc_path nil
:parent_id nil
:id (mt/id :extsales :buyerid)
:visibility_type :normal
@@ -135,6 +136,7 @@
:settings nil
:source :fields
:field_ref [:field (mt/id :extsales :salesid) nil]
+ :nfc_path nil
:parent_id nil
:id (mt/id :extsales :salesid)
:visibility_type :normal
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 76f08148d00686bfa25a8d3646c3dde8ae83f460..94743f5fc82d6c85b09e783028c14c37f0b54b0a 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11238,6 +11238,21 @@ databaseChangeLog:
AND db.object = p.object
WHERE p.object IS NULL;
+ - changeSet:
+ id: v43.00-043
+ author: howonlee
+ comment: Added 0.43.0 - Nested field columns in fields
+ changes:
+ - addColumn:
+ columns:
+ - column:
+ remarks: Nested field column paths, flattened
+ name: nfc_path
+ type: varchar(254)
+ constraints:
+ nullable: true
+ tableName: metabase_field
+
- changeSet:
id: v43.00-044
author: noahmoss
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index c2a75eca49b9c5b4d892a311f3a21fd5902ffcc2..8b3c0d683423e8518f26a37f0aab37e0aaa52c9c 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -32,6 +32,8 @@
(driver/register! :postgres, :parent :sql-jdbc)
+(defmethod driver/database-supports? [:postgres :nested-field-columns] [_ _ _] true)
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | metabase.driver impls |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -342,14 +344,27 @@
(pretty [_]
(format "%s::%s" (pr-str expr) (name psql-type)))))
+(defn- json-query [identifier nfc-path]
+ (letfn [(handle-name [x] (if (number? x) (str x) (name x)))]
+ (apply hsql/call [:json_extract_path_text
+ (hx/cast :json (keyword (first nfc-path)))
+ (mapv #(hx/cast :text (handle-name %)) (rest nfc-path))])))
+
(defmethod sql.qp/->honeysql [:postgres :field]
[driver [_ id-or-name _opts :as clause]]
- (let [{database-type :database_type} (when (integer? id-or-name)
- (qp.store/field id-or-name))
+ (let [stored-field (when (integer? id-or-name)
+ (qp.store/field id-or-name))
parent-method (get-method sql.qp/->honeysql [:sql :field])
- identifier (parent-method driver clause)]
- (if (= database-type "money")
+ identifier (parent-method driver clause)
+ nfc-path (:nfc_path stored-field)]
+ (cond
+ (= (:database_type stored-field) "money")
(pg-conversion identifier :numeric)
+
+ (some? nfc-path)
+ (json-query identifier nfc-path)
+
+ :else
identifier)))
(defmethod unprepare/unprepare-value [:postgres Date]
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index 0569206769d70fa94adbecdaee8a546cd7324379..1054532fb05993d7e2730cef5297c4407efb2fd9 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -172,7 +172,8 @@
:visibility_type :keyword
:has_field_values :keyword
:fingerprint :json-for-fingerprints
- :settings :json})
+ :settings :json
+ :nfc_path :json})
:properties (constantly {:timestamped? true})
:pre-insert pre-insert})
diff --git a/src/metabase/query_processor/middleware/add_source_metadata.clj b/src/metabase/query_processor/middleware/add_source_metadata.clj
index 0083abe5f6d551f2eecc2faf9d8d12ece8d26e8a..2b3024964a5b3ce5efe3204b11fa5f9a3f2a7d7b 100644
--- a/src/metabase/query_processor/middleware/add_source_metadata.clj
+++ b/src/metabase/query_processor/middleware/add_source_metadata.clj
@@ -56,7 +56,7 @@
:query (assoc-in source-query [:middleware :disable-remaps?] true)}))]
(for [col cols]
(select-keys col [:name :id :table_id :display_name :base_type :effective_type :coercion_strategy
- :semantic_type :unit :fingerprint :settings :source_alias :field_ref :parent_id])))
+ :semantic_type :unit :fingerprint :settings :source_alias :field_ref :nfc_path :parent_id])))
(catch Throwable e
(log/error e (str (trs "Error determining expected columns for query: {0}" (ex-message e))))
nil)))
diff --git a/src/metabase/query_processor/store.clj b/src/metabase/query_processor/store.clj
index 934b3a63bead66afb2ab40b0b6cf4a891abe86bc..84b4e01544c2d09e4b1bd7d1e6c52f0172f89081 100644
--- a/src/metabase/query_processor/store.clj
+++ b/src/metabase/query_processor/store.clj
@@ -96,6 +96,7 @@
:fingerprint
:id
:name
+ :nfc_path
:parent_id
:semantic_type
:settings
@@ -117,6 +118,7 @@
:semantic_type (s/maybe su/FieldSemanticOrRelationType)
:fingerprint (s/maybe su/Map)
:parent_id (s/maybe su/IntGreaterThanZero)
+ :nfc_path (s/maybe [su/NonBlankString])
s/Any s/Any}))
diff --git a/src/metabase/sync/fetch_metadata.clj b/src/metabase/sync/fetch_metadata.clj
index 1841b1051c5adf63a10b1134c33e82cfad4b7dcb..e5f2b1eae14dd161e8a67f483ba1b4b753502dd3 100644
--- a/src/metabase/sync/fetch_metadata.clj
+++ b/src/metabase/sync/fetch_metadata.clj
@@ -3,6 +3,7 @@
tables, schemas, and fields, and their types. For example, with SQL databases, these functions use the JDBC
DatabaseMetaData to get this information."
(:require [metabase.driver :as driver]
+ [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.util :as driver.u]
[metabase.sync.interface :as i]
[schema.core :as s]))
@@ -23,3 +24,10 @@
(let [driver (driver.u/database->driver database)]
(when (driver/supports? driver :foreign-keys)
(driver/describe-table-fks driver database table))))
+
+(s/defn nfc-metadata :- (s/maybe #{i/TableMetadataField})
+ "Get information about the nested field column fields within `table`."
+ [database :- i/DatabaseInstance, table :- i/TableInstance]
+ (let [driver (driver.u/database->driver database)]
+ (when (driver/supports? driver :nested-field-columns)
+ (sql-jdbc.sync/describe-nested-field-columns driver database table))))
diff --git a/src/metabase/sync/interface.clj b/src/metabase/sync/interface.clj
index bfab0baeb2faf5d98d93c5b6f1f9917519330cc5..19c3f9f252ee587618b57a65f0935ce8b422fca9 100644
--- a/src/metabase/sync/interface.clj
+++ b/src/metabase/sync/interface.clj
@@ -32,6 +32,7 @@
(s/optional-key :field-comment) (s/maybe su/NonBlankString)
(s/optional-key :pk?) s/Bool
(s/optional-key :nested-fields) #{(s/recursive #'TableMetadataField)}
+ (s/optional-key :nfc-path) [s/Any]
(s/optional-key :custom) {s/Any s/Any}
;; for future backwards compatability, when adding things
s/Keyword s/Any})
diff --git a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
index 5080a5aa2b25bf802f5ebd0986b12e52c82241e8..fe57f568d94ca9799574f45d9364f27ccf34de1d 100644
--- a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
+++ b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
@@ -3,7 +3,9 @@
about Fields in a Table, and for fetching the DB metadata itself. This metadata is used by the logic in other
`metabase.sync.sync-metadata.fields.*` namespaces to determine what sync operations need to be performed by
comparing the differences in the two sets of Metadata."
- (:require [medley.core :as m]
+ (:require [clojure.set :as set]
+ [medley.core :as m]
+ [metabase.driver :as driver]
[metabase.models.field :as field :refer [Field]]
[metabase.models.table :as table]
[metabase.sync.fetch-metadata :as fetch-metadata]
@@ -48,17 +50,6 @@
(assoc metabase-field :nested-fields (set (for [nested-field nested-fields]
(add-nested-fields nested-field parent-id->fields)))))))
-(s/defn ^:private add-nested-field-columns :- common/TableMetadataFieldWithID
- "Nested field columns are flattened, unlike the ordinary nested fields.
- Add the pertinent nested fields to the parent column"
- [mb-field :- common/TableMetadataFieldWithID,
- nfc-fields :- #{common/TableMetadataFieldWithID}]
- (let [column-fields (filter #(= (keyword (mb-field :name))
- (get-in % [:nfc-path 0])) nfc-fields)]
- (if-not (seq column-fields)
- mb-field
- (assoc mb-field :nested-fields (set column-fields)))))
-
(s/defn fields->our-metadata :- #{common/TableMetadataFieldWithID}
"Given a sequence of Metabase Fields, format them and return them in a hierachy so the format matches the one
`db-metadata` comes back in."
@@ -75,7 +66,7 @@
"Fetch active Fields from the Metabase application database for a given `table`."
[table :- i/TableInstance]
(db/select [Field :name :database_type :base_type :effective_type :coercion_strategy :semantic_type
- :parent_id :id :description :database_position]
+ :parent_id :id :description :database_position :nfc_path]
:table_id (u/the-id table)
:active true
{:order-by table/field-order-rule}))
@@ -94,4 +85,6 @@
"Fetch metadata about Fields belonging to a given `table` directly from an external database by calling its driver's
implementation of `describe-table`."
[database :- i/DatabaseInstance table :- i/TableInstance]
- (:fields (fetch-metadata/table-metadata database table)))
+ (cond-> (:fields (fetch-metadata/table-metadata database table))
+ (driver/database-supports? (:engine database) :nested-field-columns database)
+ (set/union (fetch-metadata/nfc-metadata database table))))
diff --git a/src/metabase/sync/sync_metadata/fields/sync_instances.clj b/src/metabase/sync/sync_metadata/fields/sync_instances.clj
index 7207bb706ebade4e1bad68c380decc9307933eb8..8163a94ad8d546bcf47df6392b81ad7084513ca7 100644
--- a/src/metabase/sync/sync_metadata/fields/sync_instances.clj
+++ b/src/metabase/sync/sync_metadata/fields/sync_instances.clj
@@ -39,7 +39,7 @@
[table :- i/TableInstance, new-field-metadatas :- [i/TableMetadataField], parent-id :- common/ParentID]
(when (seq new-field-metadatas)
(db/insert-many! Field
- (for [{:keys [database-type base-type effective-type coercion-strategy field-comment database-position], field-name :name :as field} new-field-metadatas]
+ (for [{:keys [database-type base-type effective-type coercion-strategy field-comment database-position nfc-path], field-name :name :as field} new-field-metadatas]
(do
(when (and effective-type
base-type
@@ -62,6 +62,7 @@
:coercion_strategy (when effective-type coercion-strategy)
:semantic_type (common/semantic-type field)
:parent_id parent-id
+ :nfc_path nfc-path
:description field-comment
:position database-position
:database_position database-position})))))
@@ -174,7 +175,8 @@
(s/defn ^:private sync-nested-field-instances! :- (s/maybe su/IntGreaterThanOrEqualToZero)
"Recursively sync Field instances (i.e., rows in application DB) for *all* the nested Fields of all Fields in
- `db-metadata` and `our-metadata`."
+ `db-metadata` and `our-metadata`.
+ Not for the flattened nested fields for JSON columns in normal RDBMSes (nested field columns)"
[table :- i/TableInstance
db-metadata :- #{i/TableMetadataField}
our-metadata :- #{common/TableMetadataFieldWithID}]
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index d0bf486a7179cf23bb0c9509cf0d653b42d73fce..9226cbaaabaf29667714516632303413ead54d2a 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -285,6 +285,17 @@
(is (= :type/SerializedJSON
(db/select-one-field :semantic_type Field, :id (mt/id :venues :address))))))))
+(deftest json-query-test
+ (testing "Transforming MBQL query with JSON in it to postgres query works"
+ (is (= ["json_extract_path_text(CAST(bleh AS json), (CAST(? AS text)))" "meh"]
+ (hsql/format (#'postgres/json-query 'mlep [:bleh :meh])))))
+ (testing "What if types are weird and we have lists"
+ (is (= ["json_extract_path_text(CAST(bleh AS json), (CAST(? AS text), CAST(? AS text), CAST(? AS text)))"
+ "meh"
+ "foobar"
+ "1234"]
+ (hsql/format (#'postgres/json-query 'mlep '(:bleh "meh" :foobar 1234)))))))
+
(deftest describe-nested-field-columns-test
(mt/test-driver :postgres
(testing "flattened-row"
diff --git a/test/metabase/query_processor/async_test.clj b/test/metabase/query_processor/async_test.clj
index 3bf1ecbe5f89a99116bc33912f7f51362eb23ea2..88e48dbc71738a5829a0783d13dad4337c3d687a 100644
--- a/test/metabase/query_processor/async_test.clj
+++ b/test/metabase/query_processor/async_test.clj
@@ -28,6 +28,7 @@
:table_id (mt/id :venues)
:settings nil
:source :fields
+ :nfc_path nil
:parent_id nil
:visibility_type :normal
:id (mt/id :venues :name)
diff --git a/test/metabase/query_processor/middleware/add_source_metadata_test.clj b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
index e340b6f3cb4b9a6e400ca7955e07929673be1e6d..99bcf3a4905349db41f71a79c0defb6562801ced 100644
--- a/test/metabase/query_processor/middleware/add_source_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
@@ -17,7 +17,7 @@
(select-keys
col
[:id :table_id :name :display_name :base_type :effective_type :coercion_strategy
- :semantic_type :unit :fingerprint :settings :field_ref :parent_id])))
+ :semantic_type :unit :fingerprint :settings :field_ref :nfc_path :parent_id])))
(defn- venues-source-metadata
([]
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 94f75e96e1b74e2e6ec3fc774a666d862c9b3a3a..68f1f24e86d889d23c34f3d098624c8b6dda8783 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -220,6 +220,7 @@
:name "parent.child"
:settings nil
:field_ref [:field (u/the-id child) nil]
+ :nfc_path nil
:parent_id (u/the-id parent)
:id (u/the-id child)
:visibility_type :normal
@@ -241,6 +242,7 @@
:name "grandparent.parent.child"
:settings nil
:field_ref [:field (u/the-id child) nil]
+ :nfc_path nil
:parent_id (u/the-id parent)
:id (u/the-id child)
:visibility_type :normal
diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
index 79d7bade959d79b8d102f7b4efaddd9deaab2e96..b25aec98cc2302ea87cb93c1831729f215420c7d 100644
--- a/test/metabase/query_processor_test.clj
+++ b/test/metabase/query_processor_test.clj
@@ -69,6 +69,7 @@
{:description nil
:visibility_type :normal
:settings nil
+ :nfc_path nil
:parent_id nil
:source :fields})
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 26e16985f511a5fa1031ac0802ee89b8044a3eee..e6352720ee819c05ffb52fbeb0681f6f6d2def5e 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -70,7 +70,7 @@
native-source?
(-> (assoc :field_ref [:field "PRICE" {:base-type :type/Integer}]
:effective_type :type/Integer)
- (dissoc :description :parent_id :visibility_type))
+ (dissoc :description :parent_id :nfc_path :visibility_type))
(not has-source-metadata?)
(dissoc :id :semantic_type :settings :fingerprint :table_id :coercion_strategy))
@@ -446,7 +446,7 @@
;; because this field literal comes from a native query that does not include `:source-metadata` it won't have
;; the usual extra keys
(dissoc :semantic_type :coercion_strategy :table_id
- :id :settings :fingerprint))
+ :id :settings :fingerprint :nfc_path))
(qp.test/aggregate-col :count)]
(mt/cols
(mt/with-temp Card [card {:dataset_query {:database (mt/id)
diff --git a/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj b/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
index 6a9e446682dc47c67ef7a16599f8e67ced4dec45..05835d46b30b916d58eb80b620aed69c59995eb8 100644
--- a/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
+++ b/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
@@ -11,51 +11,6 @@
[metabase.util :as u]
[toucan.db :as db]))
-(deftest add-nested-field-columns-test
- (testing "adds nested field columns for one field"
- (let [our-field {:name "coherent_json_val"
- :base-type :type/*
- :database-type "blah",
- :database-position 0,
- :id 1}
- nfc-fields '#{{:name "incoherent_json_val → b",
- :database-type "blah",
- :base-type :type/*,
- :database-position 0,
- :id 2,
- :nfc-path [:incoherent_json_val "b"]}
- {:name "coherent_json_val → a",
- :database-type "blah",
- :base-type :type/*,
- :database-position 0,
- :id 3,
- :nfc-path [:coherent_json_val "a"]}
- {:name "coherent_json_val → b",
- :database-type "blah",
- :base-type :type/*,
- :database-position 0,
- :id 4,
- :nfc-path [:coherent_json_val "b"]}}]
- (is (= (#'sync-fields.fetch-metadata/add-nested-field-columns our-field nfc-fields)
- {:name "coherent_json_val",
- :base-type :type/*,
- :database-type "blah",
- :database-position 0,
- :id 1,
- :nested-fields
- #{{:name "coherent_json_val → b",
- :database-type "blah",
- :base-type :type/*,
- :database-position 0,
- :id 4,
- :nfc-path [:coherent_json_val "b"]}
- {:name "coherent_json_val → a",
- :database-type "blah",
- :base-type :type/*,
- :database-position 0,
- :id 3,
- :nfc-path [:coherent_json_val "a"]}}})))))
-
;; `our-metadata` should match up with what we have in the DB
(deftest does-metadata-match-test
(mt/with-temp Database [db {:engine ::toucanery/toucanery}]
diff --git a/test/metabase/test/mock/util.clj b/test/metabase/test/mock/util.clj
index 2883f4e00facac95cc47e85755e20c7038295717..9e19dcd8ff33ce3abedc569b4efca62e601414de 100644
--- a/test/metabase/test/mock/util.clj
+++ b/test/metabase/test/mock/util.clj
@@ -24,6 +24,7 @@
:fk_target_field_id false
:updated_at true
:active true
+ :nfc_path nil
:parent_id false
:semantic_type nil
:id true