Merge pull request #4707 from metabase/bigquery-expression-aggregations

Handle special characters in BigQuery expression aggregations

src/metabase/driver/bigquery.clj

@@ -382,6 +382,12 @@
 (defn- string-length-fn [field-key]
   (hsql/call :length field-key))
 
+;; From the dox: Fields must contain only letters, numbers, and underscores, start with a letter or underscore, and be at most 128 characters long.
+(defn- format-custom-field-name ^String [^String custom-field-name]
+  (s/join (take 128 (-> (s/trim custom-field-name)
+                        (s/replace #"[^\w\d_]" "_")
+                        (s/replace #"(^\d)" "_$1")))))
+
 
 (defrecord BigQueryDriver []
   clojure.lang.Named
@@ -407,46 +413,45 @@
 
   driver/IDriver
   (merge driver/IDriverDefaultsMixin
-         {:analyze-table         analyze/generic-analyze-table
-          :can-connect?          (u/drop-first-arg can-connect?)
-          :date-interval         (u/drop-first-arg (comp prepare-value u/relative-date))
-          :describe-database     (u/drop-first-arg describe-database)
-          :describe-table        (u/drop-first-arg describe-table)
-          :details-fields        (constantly [{:name         "project-id"
-                                               :display-name "Project ID"
-                                               :placeholder  "praxis-beacon-120871"
-                                               :required     true}
-                                              {:name         "dataset-id"
-                                               :display-name "Dataset ID"
-                                               :placeholder  "toucanSightings"
-                                               :required     true}
-                                              {:name         "client-id"
-                                               :display-name "Client ID"
-                                               :placeholder  "1201327674725-y6ferb0feo1hfssr7t40o4aikqll46d4.apps.googleusercontent.com"
-                                               :required     true}
-                                              {:name         "client-secret"
-                                               :display-name "Client Secret"
-                                               :placeholder  "dJNi4utWgMzyIFo2JbnsK6Np"
-                                               :required     true}
-                                              {:name         "auth-code"
-                                               :display-name "Auth Code"
-                                               :placeholder  "4/HSk-KtxkSzTt61j5zcbee2Rmm5JHkRFbL5gD5lgkXek"
-                                               :required     true}])
-          :execute-query         (u/drop-first-arg execute-query)
+         {:analyze-table            analyze/generic-analyze-table
+          :can-connect?             (u/drop-first-arg can-connect?)
+          :date-interval            (u/drop-first-arg (comp prepare-value u/relative-date))
+          :describe-database        (u/drop-first-arg describe-database)
+          :describe-table           (u/drop-first-arg describe-table)
+          :details-fields           (constantly [{:name         "project-id"
+                                                  :display-name "Project ID"
+                                                  :placeholder  "praxis-beacon-120871"
+                                                  :required     true}
+                                                 {:name         "dataset-id"
+                                                  :display-name "Dataset ID"
+                                                  :placeholder  "toucanSightings"
+                                                  :required     true}
+                                                 {:name         "client-id"
+                                                  :display-name "Client ID"
+                                                  :placeholder  "1201327674725-y6ferb0feo1hfssr7t40o4aikqll46d4.apps.googleusercontent.com"
+                                                  :required     true}
+                                                 {:name         "client-secret"
+                                                  :display-name "Client Secret"
+                                                  :placeholder  "dJNi4utWgMzyIFo2JbnsK6Np"
+                                                  :required     true}
+                                                 {:name         "auth-code"
+                                                  :display-name "Auth Code"
+                                                  :placeholder  "4/HSk-KtxkSzTt61j5zcbee2Rmm5JHkRFbL5gD5lgkXek"
+                                                  :required     true}])
+          :execute-query            (u/drop-first-arg execute-query)
           ;; Don't enable foreign keys when testing because BigQuery *doesn't* have a notion of foreign keys. Joins are still allowed, which puts us in a weird position, however;
           ;; 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 (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)}))
+          :features                 (constantly (set/union #{:basic-aggregations
+                                                             :standard-deviation-aggregations
+                                                             :native-parameters
+                                                             :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)
+          :format-custom-field-name (u/drop-first-arg format-custom-field-name)
+          :mbql->native             (u/drop-first-arg mbql->native)}))
 
 (driver/register-driver! :bigquery driver)

src/metabase/driver/generic_sql.clj

@@ -27,9 +27,12 @@
    Methods marked *OPTIONAL* have default implementations in `ISQLDriverDefaultsMixin`."
 
   (active-tables ^java.util.Set [this, ^DatabaseMetaData metadata]
-    "Return a set of maps containing information about the active tables/views, collections, or equivalent that currently exist in DATABASE.
+    "*OPTIONAL* Return a set of maps containing information about the active tables/views, collections, or equivalent that currently exist in DATABASE.
      Each map should contain the key `:name`, which is the string name of the table. For databases that have a concept of schemas,
-     this map should also include the string name of the table's `:schema`.")
+     this map should also include the string name of the table's `:schema`.
+
+   Two different implementations are provided in this namespace: `fast-active-tables` (the default), and `post-filtered-active-tables`. You should be fine using
+   the default, but refer to the documentation for those functions for more details on the differences.")
 
   ;; The following apply-* methods define how the SQL Query Processor handles given query clauses. Each method is called when a matching clause is present
   ;; in QUERY, and should return an appropriately modified version of KORMA-QUERY. Most drivers can use the default implementations for all of these methods,
@@ -71,7 +74,7 @@
   (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
-     where this doesn't work, such as SQL Server")
+     where this doesn't work, such as SQL Server.")
 
   (field->alias ^String [this, ^Field field]
     "*OPTIONAL*. Return the alias that should be used to for FIELD, i.e. in an `AS` clause. The default implementation calls `name`, which

src/metabase/driver/generic_sql/query_processor.clj

@@ -151,7 +151,7 @@
 
 (defn- apply-expression-aggregation [driver honeysql-form expression]
   (h/merge-select honeysql-form [(expression-aggregation->honeysql driver expression)
-                                 (hx/escape-dots (annotate/aggregation-name expression))]))
+                                 (hx/escape-dots (driver/format-custom-field-name driver (annotate/aggregation-name expression)))]))
 
 (defn- apply-single-aggregation [driver honeysql-form {:keys [aggregation-type field], :as aggregation}]
   (h/merge-select honeysql-form [(aggregation->honeysql driver aggregation-type field)