Merge pull request #3931 from metabase/bigquery-supports-basic-aggregations

Make sure BQ declares :basic-aggregations; enable BQ SQL parameters

src/metabase/driver/bigquery.clj

@@ -106,7 +106,7 @@
    :fields (set (table-schema->metabase-field-info (.getSchema (get-table database table-name))))})
 
 
-(def ^:private ^:const query-timeout-seconds 60)
+(def ^:private ^:const ^Integer query-timeout-seconds 60)
 
 (defn- ^QueryResponse execute-bigquery
   ([{{:keys [project-id]} :details, :as database} query-string]
@@ -236,6 +236,9 @@
     :quarter-of-year (hx/quarter expr)
     :year            (hx/year expr)))
 
+(defn- date-string->literal [^String date-string]
+  (hx/->timestamp (hx/literal (u/format-date "yyyy-MM-dd 00:00" (u/->Date date-string)))))
+
 (defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
   (case seconds-or-milliseconds
     :seconds      (hsql/call :sec_to_timestamp  expr)
@@ -326,6 +329,12 @@
                     ag-type)))
     :else (str schema-name \. table-name \. field-name)))
 
+;; TODO - Making 2 DB calls for each field to fetch its dataset is inefficient and makes me cry, but this method is currently only used for SQL params so it's not a huge deal at this point
+(defn- field->identifier [{table-id :table_id, :as field}]
+  (let [db-id   (db/select-one-field :db_id 'Table :id table-id)
+        dataset (:dataset-id (db/select-one-field :details Database, :id db-id))]
+    (hsql/raw (apply format "[%s.%s.%s]" dataset (field/qualified-name-components field)))))
+
 ;; We have to override the default SQL implementations of breakout and order-by because BigQuery propogates casting functions in SELECT
 ;; BAD:
 ;; SELECT msec_to_timestamp([sad_toucan_incidents.incidents.timestamp]) AS [sad_toucan_incidents.incidents.timestamp], count(*) AS [count]
@@ -381,9 +390,11 @@
           :connection-details->spec  (constantly nil)                           ; since we don't use JDBC
           :current-datetime-fn       (constantly :%current_timestamp)
           :date                      (u/drop-first-arg date)
+          :date-string->literal      (u/drop-first-arg date-string->literal)
           :field->alias              (u/drop-first-arg field->alias)
+          :field->identifier         (u/drop-first-arg field->identifier)
           :prepare-value             (u/drop-first-arg prepare-value)
-          :quote-style               (constantly :sqlserver)                    ; we want identifiers quoted [like].[this]
+          :quote-style               (constantly :sqlserver)                    ; we want identifiers quoted [like].[this] initially (we have to convert them to [like.this] before executing)
           :string-length-fn          (u/drop-first-arg string-length-fn)
           :unix-timestamp->timestamp (u/drop-first-arg unix-timestamp->timestamp)})
 
@@ -419,9 +430,15 @@
           ;; people can manually specifiy "foreign key" relationships in admin and everything should work correctly.
           ;; Since we can't infer any "FK" relationships during sync our normal FK tests are not appropriate for BigQuery, so they're disabled for the time being.
           ;; TODO - either write BigQuery-speciifc tests for FK functionality or add additional code to manually set up these FK relationships for FK tables
-          :features              (constantly (when-not config/is-test?
-                                               ;; during unit tests don't treat bigquery as having FK support
-                                               #{:foreign-keys}))
+          :features              (constantly (set/union #{:basic-aggregations
+                                                          :standard-deviation-aggregations
+                                                          :native-parameters
+                                                          ;; Expression aggregations *would* work, but BigQuery doesn't support the auto-generated column names. BQ column names
+                                                          ;; can only be alphanumeric or underscores. If we slugified the auto-generated column names, we could enable this feature.
+                                                          #_:expression-aggregations}
+                                                        (when-not config/is-test?
+                                                          ;; during unit tests don't treat bigquery as having FK support
+                                                          #{:foreign-keys})))
           :field-values-lazy-seq (u/drop-first-arg field-values-lazy-seq)
           :mbql->native          (u/drop-first-arg mbql->native)}))
 

src/metabase/driver/generic_sql.clj

@@ -19,6 +19,7 @@
            java.util.Map
            (clojure.lang Keyword PersistentVector)
            com.mchange.v2.c3p0.ComboPooledDataSource
+           metabase.models.field.FieldInstance
            (metabase.query_processor.interface Field Value)))
 
 (defprotocol ISQLDriver
@@ -58,9 +59,20 @@
   (date [this, ^Keyword unit, field-or-value]
     "Return a HoneySQL form for truncating a date or timestamp field or value to a given resolution, or extracting a date component.")
 
+  (date-string->literal [this, ^String date-string]
+    "*OPTIONAL*. Return an appropriate HoneySQL form to represent a DATE-STRING literal.
+     The default implementation is just `hx/literal`; in other words, it just single-quotes DATE-STRING. Some drivers like BigQuery or Oracle need to do something more advanced.
+     (This is used for the implementation of SQL parameters).")
+
   (excluded-schemas ^java.util.Set [this]
     "*OPTIONAL*. Set of string names of schemas to skip syncing tables from.")
 
+  (field->identifier [this, ^FieldInstance field]
+    "*OPTIONAL*. Return a HoneySQL form that should be used as the identifier for FIELD.
+     The default implementation returns a keyword generated by from the components returned by `field/qualified-name-components`.
+     Other drivers like BigQuery need to do additional qualification, e.g. the dataset name as well.
+     (At the time of this writing, this is only used by the SQL parameters implementation; in the future it will probably be used in more places as well.)")
+
   (field-percent-urls [this field]
     "*OPTIONAL*. Implementation of the `:field-percent-urls-fn` to be passed to `make-analyze-table`.
      The default implementation is `fast-field-percent-urls`, which avoids a full table scan. Substitue this with `slow-field-percent-urls` for databases
@@ -203,6 +215,7 @@
   ([table field]
    (hx/qualify-and-escape-dots (:schema table) (:name table) (:name field))))
 
+
 (defn- query
   "Execute a HONEYSQL-FROM query against DATABASE, DRIVER, and optionally TABLE."
   ([driver database honeysql-form]
@@ -412,7 +425,9 @@
    :apply-page           (resolve 'metabase.driver.generic-sql.query-processor/apply-page)
    :column->special-type (constantly nil)
    :current-datetime-fn  (constantly :%now)
+   :date-string->literal (u/drop-first-arg hx/literal)
    :excluded-schemas     (constantly nil)
+   :field->identifier    (u/drop-first-arg (comp (partial apply hsql/qualify) field/qualified-name-components))
    :field->alias         (u/drop-first-arg name)
    :field-percent-urls   fast-field-percent-urls
    :prepare-value        (u/drop-first-arg :value)

src/metabase/driver/oracle.clj

@@ -86,6 +86,11 @@
                            3)
     :year            (hsql/call :extract :year v)))
 
+(defn- date-string->literal [^String date-string]
+  (hsql/call :to_timestamp
+    (hx/literal (u/format-date "yyyy-MM-dd" (u/->Date date-string)))
+    (hx/literal "YYYY-MM-DD")))
+
 (def ^:private ^:const now             (hsql/raw "SYSDATE"))
 (def ^:private ^:const date-1970-01-01 (hsql/call :to_timestamp (hx/literal :1970-01-01) (hx/literal :YYYY-MM-DD)))
 
@@ -218,6 +223,7 @@
           :connection-details->spec  (u/drop-first-arg connection-details->spec)
           :current-datetime-fn       (constantly now)
           :date                      (u/drop-first-arg date)
+          :date-string->literal      (u/drop-first-arg date-string->literal)
           :excluded-schemas          (fn [& _]
                                        (set/union
                                         #{"ANONYMOUS"

src/metabase/query_processor/sql_parameters.clj

@@ -46,21 +46,10 @@
   (first (hsql/format x
            :quoting ((resolve 'metabase.driver.generic-sql/quote-style) *driver*))))
 
-(defn- format-oracle-date [s]
-  (format "to_timestamp('%s', 'YYYY-MM-DD')" (u/format-date "yyyy-MM-dd" (u/->Date s))))
-
-(defn- oracle-driver? ^Boolean []
-  ;; we can't just import OracleDriver the normal way here because that would cause a cyclic load dependency
-  (boolean (when-let [oracle-driver-class (u/ignore-exceptions (Class/forName "metabase.driver.oracle.OracleDriver"))]
-             (instance? oracle-driver-class *driver*))))
-
-(defn- format-date
-  ;; This is a dirty dirty HACK! Unfortuantely Oracle is super-dumb when it comes to automatically converting strings to dates
-  ;; so we need to add the cast here
-  [date]
-  (if (oracle-driver?)
-    (format-oracle-date date)
-    (str \' date \')))
+(defn- format-date-string
+  "Format DATE-STRING as an appropriate literal using the driver's definition of `date-string->literal`."
+  ^String [^String date-string]
+  (honeysql->sql ((resolve 'metabase.driver.generic-sql/date-string->literal) *driver* date-string)))
 
 (extend-protocol ISQLParamSubstituion
   nil         (->sql [_]    "NULL")
@@ -73,20 +62,20 @@
 
   FieldInstance
   (->sql [this]
-    (->sql (let [identifier (apply hsql/qualify (field/qualified-name-components this))]
+    (->sql (let [identifier ((resolve 'metabase.driver.generic-sql/field->identifier) *driver* this)]
              (if (re-find #"^date/" (:type this))
                ((resolve 'metabase.driver.generic-sql/date) *driver* :day identifier)
                identifier))))
 
   Date
   (->sql [{:keys [s]}]
-    (format-date s))
+    (format-date-string s))
 
   DateRange
   (->sql [{:keys [start end]}]
     (if (= start end)
-      (format "= %s" (format-date start))
-      (format "BETWEEN %s AND %s" (format-date start) (format-date end))))
+      (format "= %s" (format-date-string start))
+      (format "BETWEEN %s AND %s" (format-date-string start) (format-date-string end))))
 
   Dimension
   (->sql [{:keys [field param], :as dimension}]
@@ -175,10 +164,10 @@
 
 (defn- parse-value-for-type [param-type value]
   (cond
-    (= param-type "number")                                (->NumberValue value)
+    (= param-type "number")                          (->NumberValue value)
     (and (= param-type "dimension")
-         (= (get-in value [:param :type]) "number"))       (update-in value [:param :value] ->NumberValue)
-    :else                                                  value))
+         (= (get-in value [:param :type]) "number")) (update-in value [:param :value] ->NumberValue)
+    :else                                            value))
 
 (defn- value-for-tag
   "Given a map TAG (a value in the `:template_tags` dictionary) return the corresponding value from the PARAMS sequence.

test/metabase/query_processor/sql_parameters_test.clj

@@ -12,7 +12,8 @@
             [metabase.test.data.datasets :as datasets]
             [metabase.test.data.generic-sql :as generic-sql]
             [metabase.test.util :as tu]
-            [metabase.test.data.generic-sql :as generic]))
+            [metabase.test.data.generic-sql :as generic]
+            [metabase.util :as u]))
 
 
 ;;; ------------------------------------------------------------ simple substitution -- {{x}} ------------------------------------------------------------
@@ -351,12 +352,15 @@
   (generic-sql/quote-name datasets/*driver* identifier))
 
 (defn- checkins-identifier []
-  (let [{table-name :name, schema :schema} (db/select-one ['Table :name :schema], :id (data/id :checkins))]
-    (str (when (seq schema)
-           (str (quote-name schema) \.))
-         (quote-name table-name))))
-
-;; as with the MBQL parameters tests redshift and crate fail for unknown reasons; disable their tests for now
+  ;; HACK ! I don't have all day to write protocol methods to make this work the "right" way so for BigQuery we will just hackily return the correct identifier here
+  (if (= datasets/*engine* :bigquery)
+    "[test_data.checkins]"
+    (let [{table-name :name, schema :schema} (db/select-one ['Table :name :schema], :id (data/id :checkins))]
+      (str (when (seq schema)
+             (str (quote-name schema) \.))
+           (quote-name table-name)))))
+
+;; as with the MBQL parameters tests Redshift and Crate fail for unknown reasons; disable their tests for now
 (def ^:private ^:const sql-parameters-engines
   (set/difference (engines-that-support :native-parameters) #{:redshift :crate}))