From 9e8cdef20ddb2f00d9b44ad5bce8c098ee7522d4 Mon Sep 17 00:00:00 2001
From: Howon Lee <hlee.howon@gmail.com>
Date: Thu, 16 Jun 2022 05:28:41 -0700
Subject: [PATCH] JSON query in postgres now walks identifier tree if one
encountered (fixes #22967) (#23278)
Pursuant to #22967.
Previously the notion of the identifier in json-query in postgres assumed a singular identifier at the top-level of the Identifier record. This is not always the case in complicated binning scenarios - the identifier could also just comprise an entire tree of Identifier record maps with the actual Identifier we're looking to turn into the JSON query in some leaf somewhere.
Therefore, the json-query's determination of what the identifier is now walks that identifier record tree and replaces the identifiers that have JSON field semantics with the reification of ToSql from json-query.
Previous attempt at solution hacked together something at the json-query level, changing the reification, but that only worked for one level down, instead of just walking the Identifier tree and therefore getting stuff at arbitrary tree depth. And we do get nontrivial tree depth in the bucketing in #22967.
---
src/metabase/driver/postgres.clj | 19 ++++++++++--------
test/metabase/driver/postgres_test.clj | 27 +++++++++++++++++++++++++-
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index 128f8eee721..585358473cc 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -5,6 +5,7 @@
[clojure.set :as set]
[clojure.string :as str]
[clojure.tools.logging :as log]
+ [clojure.walk :as walk]
[honeysql.core :as hsql]
[honeysql.format :as hformat]
[java-time :as t]
@@ -32,7 +33,8 @@
[pretty.core :refer [PrettyPrintable]])
(:import [java.sql ResultSet ResultSetMetaData Time Types]
[java.time LocalDateTime OffsetDateTime OffsetTime]
- [java.util Date UUID]))
+ [java.util Date UUID]
+ metabase.util.honeysql_extensions.Identifier))
(comment
ddl.postgres/keep-me)
@@ -305,11 +307,10 @@
(format "%s::%s" (pr-str expr) (name psql-type)))))
(defmethod sql.qp/json-query :postgres
- [_ identifier nfc-field]
+ [_ unwrapped-identifier nfc-field]
(letfn [(handle-name [x] (if (number? x) (str x) (name x)))]
(let [field-type (:database_type nfc-field)
nfc-path (:nfc_path nfc-field)
- unwrapped-identifier (:form identifier)
parent-identifier (field/nfc-field->parent-identifier unwrapped-identifier nfc-field)
names (format "{%s}" (str/join "," (map handle-name (rest nfc-path))))]
(reify
@@ -320,11 +321,10 @@
(defmethod sql.qp/->honeysql [:postgres :field]
[driver [_ id-or-name opts :as clause]]
- (let [stored-field (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)
- _nfc-path (:nfc_path stored-field)]
+ identifier (parent-method driver clause)]
(cond
(= (:database_type stored-field) "money")
(pg-conversion identifier :numeric)
@@ -332,7 +332,10 @@
(field/json-field? stored-field)
(if (::sql.qp/forced-alias opts)
(keyword (::add/source-alias opts))
- (sql.qp/json-query :postgres identifier stored-field))
+ (walk/postwalk #(if (instance? Identifier %)
+ (sql.qp/json-query :postgres % stored-field)
+ %)
+ identifier))
:else
identifier)))
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index 4b3c758c4b8..061b683db71 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -18,6 +18,7 @@
[metabase.models.secret :as secret]
[metabase.models.table :refer [Table]]
[metabase.query-processor :as qp]
+ [metabase.query-processor.store :as qp.store]
[metabase.sync :as sync]
[metabase.sync.sync-metadata :as sync-metadata]
[metabase.test :as mt]
@@ -286,7 +287,7 @@
(is (= false (driver/database-supports? :postgres :nested-field-columns no-json-unfold-db))))))
(deftest ^:parallel json-query-test
- (let [boop-identifier (hx/with-type-info (hx/identifier :field "boop" "bleh -> meh") {})]
+ (let [boop-identifier (:form (hx/with-type-info (hx/identifier :field "boop" "bleh -> meh") {}))]
(testing "Transforming MBQL query with JSON in it to postgres query works"
(let [boop-field {:nfc_path [:bleh :meh] :database_type "integer"}]
(is (= ["(boop.bleh#>> ?::text[])::integer " "{meh}"]
@@ -304,6 +305,30 @@
(is (= ["(boop.bleh#>> ?::text[])::bigint " "{boop,foobar,1234}"]
(hsql/format (#'sql.qp/json-query :postgres boop-identifier boolean-boop-field))))))))
+(deftest json-field-test
+ (mt/test-driver :postgres
+ (testing "Deal with complicated identifier (#22967)"
+ (let [details (mt/dbdef->connection-details :postgres :db {:database-name "complicated_identifiers"
+ :json-unfolding true})]
+ (mt/with-temp* [Database [database {:engine :postgres, :details details}]
+ Table [table {:db_id (u/the-id database)
+ :name "complicated_identifiers"}]
+ Field [val-field {:table_id (u/the-id table)
+ :nfc_path [:jsons "values" "qty"]
+ :database_type "integer"}]]
+ (qp.store/with-store
+ (qp.store/fetch-and-store-database! (u/the-id database))
+ (qp.store/fetch-and-store-tables! [(u/the-id table)])
+ (qp.store/fetch-and-store-fields! [(u/the-id val-field)])
+ (let [field-clause [:field (u/the-id val-field) {:binning
+ {:strategy :num-bins,
+ :num-bins 100,
+ :min-value 0.75,
+ :max-value 54.0,
+ :bin-width 0.75}}]]
+ (is (= ["((floor((((complicated_identifiers.jsons#>> ?::text[])::integer - 0.75) / 0.75)) * 0.75) + 0.75)" "{values,qty}"]
+ (hsql/format (sql.qp/->honeysql :postgres field-clause)))))))))))
+
(deftest json-alias-test
(mt/test-driver :postgres
(testing "json breakouts and order bys have alias coercion"
--
GitLab