Cast PG 'money' columns to ::numeric so they can be used in numeric fns (#11717)

[ci postgres]

src/metabase/driver/postgres.clj

@@ -6,10 +6,13 @@
              [string :as str]]
             [clojure.java.jdbc :as jdbc]
             [clojure.tools.logging :as log]
-            [honeysql.core :as hsql]
+            [honeysql
+             [core :as hsql]
+             [format :as hformat]]
             [java-time :as t]
             [metabase
              [driver :as driver]
+             [models :refer [Field]]
              [util :as u]]
             [metabase.db.spec :as db.spec]
             [metabase.driver.common :as driver.common]
@@ -23,7 +26,8 @@
             [metabase.util
              [date-2 :as u.date]
              [honeysql-extensions :as hx]
-             [ssh :as ssh]])
+             [ssh :as ssh]]
+            [pretty.core :refer [PrettyPrintable]])
   (:import [java.sql ResultSet ResultSetMetaData Time Types]
            [java.time LocalDateTime OffsetDateTime OffsetTime]
            [java.util Date UUID]))
@@ -151,16 +155,37 @@
   [driver value]
   (let [[_ value {base-type :base_type, database-type :database_type}] value]
     (when (some? value)
-      (cond
-        (isa? base-type :type/UUID)         (UUID/fromString value)
-        (isa? base-type :type/IPAddress)    (hx/cast :inet value)
-        (isa? base-type :type/PostgresEnum) (hx/quoted-cast database-type value)
-        :else                               (sql.qp/->honeysql driver value)))))
+      (condp #(isa? %2 %1) base-type
+        :type/UUID         (UUID/fromString value)
+        :type/IPAddress    (hx/cast :inet value)
+        :type/PostgresEnum (hx/quoted-cast database-type value)
+        (sql.qp/->honeysql driver value)))))
 
 (defmethod sql.qp/->honeysql [:postgres Time]
   [_ time-value]
   (hx/->time time-value))
 
+(defn- pg-conversion
+  "HoneySQL form that adds a Postgres-style `::` cast e.g. `expr::type`.
+
+    (pg-conversion :my_field ::integer) -> HoneySQL -[Compile]-> \"my_field\"::integer"
+  [expr psql-type]
+  (reify
+    hformat/ToSql
+    (to-sql [_]
+      (format "%s::%s" (hformat/to-sql expr) (name psql-type)))
+    PrettyPrintable
+    (pretty [_]
+      (format "%s::%s" (pr-str expr) (name psql-type)))))
+
+(defmethod sql.qp/->honeysql [:postgres (class Field)]
+  [driver {database-type :database_type, :as field}]
+  (let [parent-method (get-method sql.qp/->honeysql [:sql (class Field)])
+        identifier    (parent-method driver field)]
+    (if (= database-type "money")
+      (pg-conversion identifier :numeric)
+      identifier)))
+
 (defmethod unprepare/unprepare-value [:postgres Date]
   [_ value]
   (format "'%s'::timestamp" (u.date/format value)))

src/metabase/query_processor/middleware/normalize_query.clj

 (ns metabase.query-processor.middleware.normalize-query
   "Middleware that converts a query into a normalized, canonical form."
-  (:require [metabase.mbql.normalize :as normalize]))
+  (:require [metabase.mbql.normalize :as normalize]
+            [metabase.query-processor.error-type :as error-type]))
 
 (defn normalize
   "Middleware that converts a query into a normalized, canonical form, including things like converting all identifiers
   into standard `lisp-case` ones, removing/rewriting legacy clauses, removing empty ones, etc. This is done to
   simplifiy the logic in the QP steps following this."
   [qp]
-  (fn [query respond raise canceled-chan]
-    (qp (normalize/normalize query) respond raise canceled-chan)))
+  (letfn [(normalize [query]
+            (try
+              (normalize/normalize query)
+              (catch Throwable e
+                (throw (ex-info (.getMessage e)
+                         {:type  error-type/invalid-query
+                          :query query}
+                         e)))))]
+    (fn [query respond raise canceled-chan]
+      (qp (normalize query) respond raise canceled-chan))))

test/metabase/driver/postgres_test.clj

@@ -276,17 +276,52 @@
                           {:aggregation [[:count]]
                            :filter      [:= $ip "192.168.1.1"]}))))))))
 
+(defn- do-with-money-test-db [thunk]
+  (drop-if-exists-and-create-db! "money_columns_test")
+  (let [details (mt/dbdef->connection-details :postgres :db {:database-name "money_columns_test"})]
+    (jdbc/with-db-connection [conn (sql-jdbc.conn/connection-details->spec :postgres details)]
+      (doseq [sql+args [["CREATE table bird_prices (bird TEXT, price money);"]
+                        ["INSERT INTO bird_prices (bird, price) VALUES (?, ?::numeric::money), (?, ?::numeric::money);"
+                         "Lucky Pigeon"   6.0
+                         "Katie Parakeet" 23.99]]]
+        (jdbc/execute! conn sql+args)))
+    (mt/with-temp Database [db {:engine :postgres, :details (assoc details :dbname "money_columns_test")}]
+      (sync/sync-database! db)
+      (mt/with-db db
+        (thunk)))))
+
 (deftest money-columns-test
   (mt/test-driver :postgres
     (testing "We should support the Postgres MONEY type"
       (testing "It should be possible to return money column results (#3754)"
-        (mt/with-log-level :trace
-          (is (= [{:money 1000.00M}]
-                 (jdbc/query
-                  (sql-jdbc.conn/connection-details->spec :postgres (mt/dbdef->connection-details :postgres :server nil))
-                  "SELECT 1000::money AS \"money\";"
-                  {:read-columns   (partial sql-jdbc.execute/read-columns :postgres)
-                   :set-parameters (partial sql-jdbc.execute/set-parameters :postgres)}))))))))
+        (is (= [{:money 1000.00M}]
+               (jdbc/query
+                (sql-jdbc.conn/connection-details->spec :postgres (mt/dbdef->connection-details :postgres :server nil))
+                "SELECT 1000::money AS \"money\";"
+                {:read-columns   (partial sql-jdbc.execute/read-columns :postgres)
+                 :set-parameters (partial sql-jdbc.execute/set-parameters :postgres)}))))
+      (do-with-money-test-db
+       (fn []
+         (testing "We should be able to select avg() of a money column (#11498)"
+           (is (= [[14.995M]]
+                  (mt/rows
+                    (mt/run-mbql-query bird_prices
+                      {:aggregation [[:avg $price]]})))))
+         (testing "Should be able to filter on a money column"
+           (is (= [["Katie Parakeet" 23.99M]]
+                  (mt/rows
+                    (mt/run-mbql-query bird_prices
+                      {:filter [:= $price 23.99]}))))
+           (is (= []
+                  (mt/rows
+                    (mt/run-mbql-query bird_prices
+                      {:filter [:!= $price $price]})))))
+         (testing "Should be able to sort by price"
+           (is (= [["Katie Parakeet" 23.99M]
+                   ["Lucky Pigeon" 6.00M]]
+                  (mt/rows
+                    (mt/run-mbql-query bird_prices
+                      {:order-by [[:desc $price]]}))))))))))
 
 (defn- enums-test-db-details [] (mt/dbdef->connection-details :postgres :db {:database-name "enums_test"}))