Fix using native queries as source queries in joins (#10546)

src/metabase/driver/sql/query_processor.clj

@@ -23,6 +23,7 @@
              [honeysql-extensions :as hx]
              [i18n :refer [tru]]
              [schema :as su]]
+            [potemkin.types :as p.types]
             [pretty.core :refer [PrettyPrintable]]
             [schema.core :as s])
   (:import metabase.util.honeysql_extensions.Identifier))
@@ -40,6 +41,21 @@
   Each nested query increments this counter by 1."
   0)
 
+(p.types/deftype+ SQLSourceQuery [sql params]
+  hformat/ToSql
+  (to-sql [_]
+    (dorun (map hformat/add-anon-param params))
+    ;; strip off any trailing semicolons
+    (str "(" (str/replace sql #";+\s*$" "") ")"))
+  PrettyPrintable
+  (pretty [_]
+    (list 'SQLSourceQuery. sql params))
+  Object
+  (equals [_ other]
+    (and (instance? SQLSourceQuery other)
+         (= sql    (.sql ^SQLSourceQuery other))
+         (= params (.params ^SQLSourceQuery other)))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                            Interface (Multimethods)                                            |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -477,8 +493,14 @@
 (defmethod join-source :sql
   [driver {:keys [source-table source-query]}]
   (binding [*table-alias* nil]
-    (if source-query
+    (cond
+      (and source-query (:native source-query))
+      (SQLSourceQuery. (:native source-query) (:params source-query))
+
+      source-query
       (build-honeysql-form driver {:query source-query})
+
+      :else
       (->honeysql driver (qp.store/table source-table)))))
 
 (def ^:private HoneySQLJoin
@@ -629,16 +651,6 @@
     FROM ( SELECT * FROM some_table ) source"
   :source)
 
-(deftype ^:private SQLSourceQuery [sql params]
-  hformat/ToSql
-  (to-sql [_]
-    (dorun (map hformat/add-anon-param params))
-    ;; strip off any trailing semicolons
-    (str "(" (str/replace sql #";+\s*$" "") ")"))
-  PrettyPrintable
-  (pretty [_]
-    (list 'SQLSourceQuery. sql params)))
-
 (defn- apply-source-query
   "Handle a `:source-query` clause by adding a recursive `SELECT` or native query. At the time of this writing, all
   source queries are aliased as `source`."

test/metabase/driver/sql/query_processor_test.clj

@@ -150,7 +150,7 @@
    :group-by [(id :field "PUBLIC" "VENUES" "PRICE")]
    :order-by [[(id :field-alias "avg_2") :asc]]}
   (qp.test-util/with-everything-store
-    (metabase.driver/with-driver :h2
+    (driver/with-driver :h2
       (#'sql.qp/mbql->honeysql
        ::id-swap
        (data/mbql-query venues
@@ -163,8 +163,37 @@
   {:query "SELECT \"source\".* FROM (SELECT * FROM some_table WHERE name = ?) \"source\" WHERE \"source\".\"name\" <> ?"
    :params ["Cam" "Lucky Pigeon"]}
   (qp.test-util/with-everything-store
-    (metabase.driver/with-driver :h2
+    (driver/with-driver :h2
       (sql.qp/mbql->native :h2
         (data/mbql-query venues
           {:source-query {:native "SELECT * FROM some_table WHERE name = ?;", :params ["Cam"]}
            :filter       [:!= *name/Integer "Lucky Pigeon"]})))))
+
+;; Joins against native SQL queries should get converted appropriately!
+;; make sure correct HoneySQL is generated
+(expect
+  [[(sql.qp/->SQLSourceQuery "SELECT * FROM VENUES;" [])
+    (hx/identifier :table-alias "card")]
+   [:=
+    (hx/identifier :field "PUBLIC" "CHECKINS" "VENUE_ID")
+    (hx/identifier :field "card" "id")]]
+  (qp.test-util/with-everything-store
+    (driver/with-driver :h2
+      (sql.qp/join->honeysql :h2
+        (data/$ids checkins
+          {:source-query {:native "SELECT * FROM VENUES;", :params []}
+           :alias        "card"
+           :strategy     :left-join
+           :condition    [:= $venue_id &card.*id/Integer]})))))
+
+;; make sure the generated HoneySQL will compile to the correct SQL
+(expect
+  ["INNER JOIN (SELECT * FROM VENUES) card ON PUBLIC.CHECKINS.VENUE_ID = card.id"]
+  (hsql/format {:join (qp.test-util/with-everything-store
+                        (driver/with-driver :h2
+                          (sql.qp/join->honeysql :h2
+                            (data/$ids checkins
+                              {:source-query {:native "SELECT * FROM VENUES;", :params []}
+                               :alias        "card"
+                               :strategy     :left-join
+                               :condition    [:= $venue_id &card.*id/Integer]}))))}))

test/metabase/query_processor_test.clj

@@ -211,14 +211,40 @@
       (m/dissoc-in [:data :results_metadata])
       (m/dissoc-in [:data :insights])))
 
-(defn- default-format-rows-by-fns [table-kw]
-  (case table-kw
-    :categories [int identity]
-    :checkins   [int identity int int]
-    :users      [int identity identity]
-    :venues     [int identity int 4.0 4.0 int]
-    (throw
-     (IllegalArgumentException. (format "Sorry, we don't have default format-rows-by fns for Table %s." table-kw)))))
+(defmulti format-rows-fns
+  "Return vector of functions (or floating-point numbers, for rounding; see `format-rows-by`) to use to format result
+  rows with `format-rows-by` or `formatted-rows`. The first arg to these macros is converted to a sequence of
+  functions by calling this function.
+
+  Sequential args are assumed to already be a sequence of functions and are returned as-is. Keywords can be thought of
+  as aliases and map to a pre-defined sequence of functions. The usual test data tables have predefined fn sequences;
+  you can add addition ones for use locally by adding more implementations for this method.
+
+    (format-rows-fns [int identity]) ;-> [int identity]
+    (format-rows-fns :venues)        ;-> [int identity int 4.0 4.0 int]"
+  {:arglists '([keyword-or-fns-seq])}
+  (fn [x]
+    (if (keyword? x) x (class x))))
+
+(defmethod format-rows-fns clojure.lang.Sequential
+  [this]
+  this)
+
+(defmethod format-rows-fns :categories
+  [_]
+  [int identity])
+
+(defmethod format-rows-fns :checkins
+  [_]
+  [int identity int int])
+
+(defmethod format-rows-fns :users
+  [_]
+  [int identity identity])
+
+(defmethod format-rows-fns :venues
+  [_]
+  [int identity int 4.0 4.0 int])
 
 (defn- format-rows-fn
   "Handle a value formatting function passed to `format-rows-by`."
@@ -254,11 +280,7 @@
      (println "Error running query:" (u/pprint-to-str 'red response))
      (throw (ex-info (:error response) response)))
 
-   (let [format-fns (map
-                     format-rows-fn
-                     (if (keyword? format-fns)
-                       (default-format-rows-by-fns format-fns)
-                       format-fns))]
+   (let [format-fns (map format-rows-fn (format-rows-fns format-fns))]
      (-> response
          ((fn format-rows [rows]
             (cond

test/metabase/query_processor_test/explicit_joins_test.clj

@@ -447,3 +447,20 @@
                          :condition    [:= *user_id &v.venues.id]}]
          :order-by     [[:asc $id]]
          :limit        2}))))
+
+;; we should be able to use a SQL question as a source query in a Join
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :left-join)
+  ;; again, Oracle is wack, not sure why :/
+  (let [tz (if (= driver/*driver* :oracle) "07:00:00.000Z"  "00:00:00.000Z")]
+    [[1 (str "2014-04-07T" tz) 5 12 12 "The Misfit Restaurant + Bar" 2 34.0154 -118.497 2]
+     [2 (str "2014-09-18T" tz) 1 31 31 "Bludso's BBQ"                5 33.8894 -118.207 2]])
+  (tt/with-temp Card [{card-id :id, :as card} (qp.test-util/card-with-source-metadata-for-query
+                                               (data/native-query (qp/query->native (data/mbql-query venues))))]
+    (qp.test/formatted-rows [int identity int int int identity int 4.0 4.0 int]
+      (data/run-mbql-query checkins
+        {:joins    [{:fields       :all
+                     :source-table (str "card__" card-id)
+                     :alias        "card"
+                     :condition    [:= $venue_id &card.*venues.id]}]
+         :order-by [[:asc $id]]
+         :limit    2}))))

test/metabase/query_processor_test/nested_queries_test.clj

@@ -593,13 +593,12 @@
 ;; out what you were referring to and behave appropriately
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
   [[10]]
-  (qp.test/format-rows-by [int]
-    (qp.test/rows
-      (data/run-mbql-query venues
-        {:source-query {:source-table $$venues
-                        :fields       [$id $name $category_id $latitude $longitude $price]}
-         :aggregation  [[:count]]
-         :filter       [:= $category_id 50]}))))
+  (qp.test/formatted-rows [int]
+    (data/run-mbql-query venues
+      {:source-query {:source-table $$venues
+                      :fields       [$id $name $category_id $latitude $longitude $price]}
+       :aggregation  [[:count]]
+       :filter       [:= $category_id 50]})))
 
 ;; make sure that if a nested query includes joins queries based on it still work correctly (#8972)
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
@@ -610,40 +609,37 @@
    [37 "bigmista's barbecue"  5 34.118  -118.26  2]
    [38 "Zeke's Smokehouse"    5 34.2053 -118.226 2]
    [39 "Baby Blues BBQ"       5 34.0003 -118.465 2]]
-  (qp.test/format-rows-by :venues
-    (qp.test/rows
-      (qp/process-query
-        (data/mbql-query venues
-          {:source-query
-           {:source-table $$venues
-            :filter       [:= $venues.category_id->categories.name "BBQ"]
-            :order-by     [[:asc $id]]}})))))
+  (qp.test/formatted-rows :venues
+    (qp/process-query
+      (data/mbql-query venues
+        {:source-query
+         {:source-table $$venues
+          :filter       [:= $venues.category_id->categories.name "BBQ"]
+          :order-by     [[:asc $id]]}}))))
 
 ;; Make sure we parse datetime strings when compared against type/DateTime field literals (#9007)
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
   [[395]
    [980]]
-  (qp.test/format-rows-by [int]
-    (qp.test/rows
-      (data/run-mbql-query checkins
-        {:source-query {:source-table $$checkins
-                        :order-by     [[:asc $id]]}
-         :fields       [$id]
-         :filter       [:= *date "2014-03-30"]}))))
+  (qp.test/formatted-rows [int]
+    (data/run-mbql-query checkins
+      {:source-query {:source-table $$checkins
+                      :order-by     [[:asc $id]]}
+       :fields       [$id]
+       :filter       [:= *date "2014-03-30"]})))
 
 ;; make sure filters in source queries are applied correctly!
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
   [["Fred 62"     1]
    ["Frolic Room" 1]]
-  (qp.test/format-rows-by [str int]
-    (qp.test/rows
-      (data/run-mbql-query checkins
-        {:source-query {:source-table $$checkins
-                        :filter       [:> $date "2015-01-01"]}
-         :aggregation  [:count]
-         :order-by     [[:asc $venue_id->venues.name]]
-         :breakout     [$venue_id->venues.name]
-         :filter       [:starts-with $venue_id->venues.name "F"]}))))
+  (qp.test/formatted-rows [str int]
+    (data/run-mbql-query checkins
+      {:source-query {:source-table $$checkins
+                      :filter       [:> $date "2015-01-01"]}
+       :aggregation  [:count]
+       :order-by     [[:asc $venue_id->venues.name]]
+       :breakout     [$venue_id->venues.name]
+       :filter       [:starts-with $venue_id->venues.name "F"]})))
 
 ;; Do nested queries work with two of the same aggregation? (#9767)
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
@@ -675,15 +671,14 @@
 ;; can you use nested queries that have expressions in them?
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys :expressions)
   [[30] [20]]
-  (qp.test/format-rows-by [int int]
-    (qp.test/rows
-      (data/run-mbql-query venues
-        {:source-query
-         {:source-table $$venues
-          :fields       [[:expression "price-times-ten"]]
-          :expressions  {"price-times-ten" [:* $price 10]}
-          :order-by     [[:asc $id]]
-          :limit        2}}))))
+  (qp.test/formatted-rows [int int]
+    (data/run-mbql-query venues
+      {:source-query
+       {:source-table $$venues
+        :fields       [[:expression "price-times-ten"]]
+        :expressions  {"price-times-ten" [:* $price 10]}
+        :order-by     [[:asc $id]]
+        :limit        2}})))
 
 (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys :expressions)
   [[30] [20]]
@@ -693,7 +688,6 @@
                                                        :order-by     [[:asc $id]]
                                                        :limit        2})}]
 
-    (qp.test/format-rows-by [int int]
-      (qp.test/rows
-        (data/run-mbql-query nil
-          {:source-table (str "card__" card-id)})))))
+    (qp.test/formatted-rows [int int]
+      (data/run-mbql-query nil
+        {:source-table (str "card__" card-id)}))))