QIP

.dir-locals.el

@@ -22,6 +22,7 @@
                               (context 2)
                               (create-database-definition 1)
                               (execute-query 1)
+                              (execute-sql! 2)
                               (expect 1)
                               (expect-eval-actual-first 1)
                               (expect-expansion 0)

circle.yml

@@ -11,13 +11,13 @@ dependencies:
     - pip install awscli==1.7.3
 test:
   override:
-    # 0) runs unit tests w/ H2 local DB. Runs against both Mongo + H2 test datasets
+    # 0) runs unit tests w/ H2 local DB. Runs against Mongo, H2, Postgres, MySQL test datasets
     # 1) runs unit tests w/ Postgres local DB. Only runs against H2 test dataset so we can be sure tests work in either scenario
     # 2) runs Eastwood linter
     # 3) Bikeshed linter
     # 4) runs JS linter + JS test
     # 5) runs lein uberjar
-    - case $CIRCLE_NODE_INDEX in 0) MB_TEST_DATASETS=h2,mongo,postgres lein test ;; 1) MB_DB_TYPE=postgres MB_DB_DBNAME=circle_test MB_DB_PORT=5432 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 2) lein eastwood ;; 3) lein bikeshed --max-line-length 240 ;; 4) npm run lint && npm run build && npm run test ;; 5) CI_DISABLE_WEBPACK_MINIFICATION=1 lein uberjar ;; esac:
+    - case $CIRCLE_NODE_INDEX in 0) MB_TEST_DATASETS=h2,mongo,postgres,mysql lein test ;; 1) MB_DB_TYPE=postgres MB_DB_DBNAME=circle_test MB_DB_PORT=5432 MB_DB_USER=ubuntu MB_DB_HOST=localhost lein test ;; 2) lein eastwood ;; 3) lein bikeshed --max-line-length 240 ;; 4) npm run lint && npm run build && npm run test ;; 5) CI_DISABLE_WEBPACK_MINIFICATION=1 lein uberjar ;; esac:
         parallel: true
 deployment:
   master:

resources/frontend_client/app/services.js

@@ -502,6 +502,40 @@ CorvusServices.service('CorvusCore', ['$resource', 'User', function($resource, U
                 }]
             }]
         },
+        mysql: {
+            name: 'MySQL',
+            fields: [{
+                displayName: "Host",
+                fieldName: "host",
+                type: "text",
+                placeholder: "localhost",
+                placeholderIsDefault: true
+            }, {
+                displayName: "Port",
+                fieldName: "port",
+                type: "text",
+                transform: parseInt,
+                placeholder: "3306",
+                placeholderIsDefault: true
+            }, {
+                displayName: "Database name",
+                fieldName: "dbname",
+                type: "text",
+                placeholder: "birds_of_the_world",
+                required: true
+            }, {
+                displayName: "Database username",
+                fieldName: "user",
+                type: "text",
+                placeholder: "What username do you use to login to the database?",
+                required: true
+            }, {
+                displayName: "Database password",
+                fieldName: "password",
+                type: "password",
+                placeholder: "*******"
+            }]
+        },
         h2: {
             name: 'H2',
             fields: [{

src/metabase/driver/generic_sql/interface.clj

@@ -15,3 +15,14 @@
   (timezone->set-timezone-sql          [this timezone]
     "Return a string that represents the SQL statement that should be used to set the timezone
      for the current transaction."))
+
+(defprotocol ISqlDriverQuoteName
+  "Optionally protocol to override how the Generic SQL driver quotes the names of databases, tables, and fields."
+  (quote-name [this ^String nm]
+    "Quote a name appropriately for this database."))
+
+;; Default implementation quotes using "
+(extend-protocol ISqlDriverQuoteName
+  Object
+  (quote-name [_ nm]
+    (str \" nm \")))

src/metabase/driver/generic_sql/query_processor.clj

@@ -93,18 +93,18 @@
     ([this]
      (formatted this false))
     ([{:keys [table-name field-name base-type special-type]} include-as?]
-     ;; TODO - add Table names
-     (cond
-       (contains? #{:DateField :DateTimeField} base-type) `(raw ~(str (format "CAST(\"%s\".\"%s\" AS DATE)" table-name field-name)
-                                                                      (when include-as?
-                                                                        (format " AS \"%s\"" field-name))))
-       (= special-type :timestamp_seconds)                `(raw ~(str (i/cast-timestamp-to-date (:driver *query*) table-name field-name :seconds)
-                                                                      (when include-as?
-                                                                        (format " AS \"%s\"" field-name))))
-       (= special-type :timestamp_milliseconds)           `(raw ~(str (i/cast-timestamp-to-date (:driver *query*) table-name field-name :milliseconds)
-                                                                      (when include-as?
-                                                                        (format " AS \"%s\"" field-name))))
-       :else                                              (keyword (format "%s.%s" table-name field-name)))))
+     (let [quot (partial i/quote-name (:driver *query*))]
+       (cond
+         (contains? #{:DateField :DateTimeField} base-type) `(raw ~(str (format "CAST(%s.%s AS DATE)" (quot table-name) (quot field-name))
+                                                                        (when include-as?
+                                                                          (format " AS %s" (quot field-name)))))
+         (= special-type :timestamp_seconds)                `(raw ~(str (i/cast-timestamp-to-date (:driver *query*) table-name field-name :seconds)
+                                                                        (when include-as?
+                                                                          (format " AS %s" (quot field-name)))))
+         (= special-type :timestamp_milliseconds)           `(raw ~(str (i/cast-timestamp-to-date (:driver *query*) table-name field-name :milliseconds)
+                                                                        (when include-as?
+                                                                          (format " AS %s" (quot field-name)))))
+         :else                                              (keyword (format "%s.%s" (quot table-name) (quot field-name)))))))
 
 
   ;; e.g. the ["aggregation" 0] fields we allow in order-by

src/metabase/driver/mysql.clj

@@ -6,7 +6,7 @@
                                                 ISyncDriverSpecificSyncField driver-specific-sync-field!]])
             (metabase.driver.generic-sql [interface :refer :all])))
 
-(def ^:private ^:const column->base-type
+(def ^:private ^:const column->base-type*
   {:bigint     :BigIntegerField
    :binary     :UnknownField
    :bit        :UnknownField
@@ -38,6 +38,10 @@
    :varchar    :CharField
    :year       :IntegerField})
 
+(defn- column->base-type [t]
+  (println "t ================================================================================>" t)
+  (column->base-type* t))
+
 (defrecord MySQLDriver []
   ISqlDriverDatabaseSpecific
   (connection-details->connection-spec [_ details]
@@ -47,12 +51,21 @@
     details)
 
   (cast-timestamp-to-date [_ table-name field-name seconds-or-milliseconds]
-    ;; TODO
-    )
+    (format "CAST(TIMESTAMPADD(%s, `%s`.`%s`, DATE '1970-01-01') AS DATE)"
+            (case seconds-or-milliseconds
+              :seconds      "SECOND"
+              :milliseconds "MILLISECOND")
+            table-name field-name))
 
   (timezone->set-timezone-sql [_ timezone]
-    ;; see http://stackoverflow.com/questions/930900/how-to-set-time-zone-of-mysql
-    (format "SET @@session.time_zone = '%s';" timezone)))
+    ;; If this fails you need to load the timezone definitions from your system into MySQL;
+    ;; run the command `mysql_tzinfo_to_sql /usr/share/zoneinfo | mysql -u root mysql`
+    ;; See https://dev.mysql.com/doc/refman/5.7/en/time-zone-support.html for details
+    (format "SET @@session.time_zone = '%s';" timezone))
+
+  ISqlDriverQuoteName
+  (quote-name [_ nm]
+    (str \` nm \`)))
 
 (extend MySQLDriver
   IDriver                     GenericSQLIDriverMixin

test/metabase/test/data/datasets.clj

@@ -12,6 +12,7 @@
             (metabase.test.data [data :as data]
                                 [h2 :as h2]
                                 [mongo :as mongo]
+                                [mysql :as mysql]
                                 [postgres :as postgres])
             [metabase.util :as u]))
 
@@ -76,7 +77,7 @@
   (timestamp-field-type [_] :DateField))
 
 
-;; ## Generic SQL (H2)
+;; ## Generic SQL
 
 ;; TODO - Mongo implementation (etc.) might find these useful
 (def ^:private memoized-table-name->id
@@ -91,67 +92,67 @@
      {:pre [(string? field-name)]}
      (sel :one :id Field :name field-name, :table_id (memoized-table-name->id db-id table-name)))))
 
+(defn- generic-sql-load-data! [{:keys [dbpromise], :as this}]
+  (when-not (realized? dbpromise)
+    (deliver dbpromise ((u/runtime-resolved-fn 'metabase.test.data 'get-or-create-database!) (dataset-loader this) data/test-data)))
+  @dbpromise)
 
-(deftype H2DriverData [dbpromise]
-  IDataset
-  (dataset-loader [_]
-    (h2/dataset-loader))
-
-  (load-data! [this]
-    (when-not (realized? dbpromise)
-      (deliver dbpromise ((u/runtime-resolved-fn 'metabase.test.data 'get-or-create-database!) (dataset-loader this) data/test-data)))
-    @dbpromise)
-
-  (db [this]
-    (load-data! this))
+(def ^:private GenericSQLIDatasetMixin
+  {:load-data!           generic-sql-load-data!
+   :db                   generic-sql-load-data!
+   :table-name->id       (fn [this table-name]
+                           (memoized-table-name->id (:id (db this)) (name table-name)))
+   :table-name->table    (fn [this table-name]
+                           (Table (table-name->id this (name table-name))))
+   :field-name->id       (fn [this table-name field-name]
+                           (memoized-field-name->id (:id (db this)) (name table-name) (name field-name)))
+   :format-name          (fn [_ table-or-field-name]
+                           table-or-field-name)
+   :fks-supported?       (constantly true)
+   :timestamp-field-type (constantly :DateTimeField)
+   :id-field-type        (constantly :BigIntegerField)})
 
-  (table-name->id [this table-name]
-    (memoized-table-name->id (:id (db this)) (s/upper-case (name table-name))))
 
-  (table-name->table [this table-name]
-    (Table (table-name->id this (s/upper-case (name table-name)))))
+;;; ### H2
 
-  (field-name->id [this table-name field-name]
-    (memoized-field-name->id (:id (db this)) (s/upper-case (name table-name)) (s/upper-case (name field-name))))
+(defrecord H2DriverData [dbpromise])
 
-  (format-name [_ table-or-field-name]
-    (clojure.string/upper-case table-or-field-name))
+(extend H2DriverData
+  IDataset
+  (merge GenericSQLIDatasetMixin
+         {:dataset-loader    (fn [_]
+                               (h2/dataset-loader))
+          :table-name->id    (fn [this table-name]
+                               (memoized-table-name->id (:id (db this)) (s/upper-case (name table-name))))
+          :table-name->table (fn [this table-name]
+                               (Table (table-name->id this (s/upper-case (name table-name)))))
+          :field-name->id    (fn [this table-name field-name]
+                               (memoized-field-name->id (:id (db this)) (s/upper-case (name table-name)) (s/upper-case (name field-name))))
+          :format-name       (fn [_ table-or-field-name]
+                               (clojure.string/upper-case table-or-field-name))
+          :id-field-type     (constantly :BigIntegerField)}))
 
-  (fks-supported?       [_] true)
-  (id-field-type        [_] :BigIntegerField)
-  (timestamp-field-type [_] :DateTimeField))
 
+;;; ### Postgres
 
-;; ## Postgres
+(defrecord PostgresDriverData [dbpromise])
 
-(deftype PostgresDriverData [dbpromise]
+(extend PostgresDriverData
   IDataset
-  (dataset-loader [_]
-    (postgres/dataset-loader))
+  (merge GenericSQLIDatasetMixin
+         {:dataset-loader (fn [_]
+                            (postgres/dataset-loader))}))
 
-  (load-data! [this]
-    (when-not (realized? dbpromise)
-      (deliver dbpromise ((u/runtime-resolved-fn 'metabase.test.data 'get-or-create-database!) (dataset-loader this) data/test-data)))
-    @dbpromise)
-
-  (db [this]
-    (load-data! this))
 
-  (table-name->id [this table-name]
-    (memoized-table-name->id (:id (db this)) (name table-name)))
+;;; ### MySQL
 
-  (table-name->table [this table-name]
-    (Table (table-name->id this (name table-name))))
+(defrecord MySQLDriverData [dbpromise])
 
-  (field-name->id [this table-name field-name]
-    (memoized-field-name->id (:id (db this)) (name table-name) (name field-name)))
-
-  (format-name [_ table-or-field-name]
-    table-or-field-name)
-
-  (fks-supported?       [_] true)
-  (id-field-type        [_] :IntegerField)
-  (timestamp-field-type [_] :DateTimeField))
+(extend MySQLDriverData
+  IDataset
+  (merge GenericSQLIDatasetMixin
+         {:dataset-loader (fn [_]
+                            (mysql/dataset-loader))}))
 
 
 ;; # Concrete Instances
@@ -160,7 +161,8 @@
   "Map of dataset keyword name -> dataset instance (i.e., an object that implements `IDataset`)."
   {:mongo    (MongoDriverData.)
    :h2       (H2DriverData. (promise))
-   :postgres (PostgresDriverData. (promise))})
+   :postgres (PostgresDriverData. (promise))
+   :mysql    (MySQLDriverData. (promise))})
 
 (def ^:const all-valid-dataset-names
   "Set of names of all valid datasets."

test/metabase/test/data/generic_sql.clj

@@ -2,6 +2,7 @@
   "Common functionality for various Generic SQL dataset loaders."
   (:require [clojure.tools.logging :as log]
             [korma.core :as k]
+            [metabase.driver.generic-sql.interface :refer [quote-name]]
             [metabase.test.data.interface :as i])
   (:import (metabase.test.data.interface DatabaseDefinition
                                          TableDefinition)))
@@ -31,39 +32,42 @@
 
   ;; Now create the new table
   (execute-sql! dataset-loader database-definition
-    (format "CREATE TABLE \"%s\" (%s, \"%s\" %s, PRIMARY KEY (\"%s\"));"
-            table-name
-            (->> field-definitions
-                 (map (fn [{:keys [field-name base-type]}]
-                        (format "\"%s\" %s" field-name (field-base-type->sql-type dataset-loader base-type))))
-                 (interpose ", ")
-                 (apply str))
-            (pk-field-name dataset-loader)
-            (pk-sql-type dataset-loader)
-            (pk-field-name dataset-loader))))
+    (let [quot (partial quote-name dataset-loader)
+          pk-field-name (quot (pk-field-name dataset-loader))]
+      (format "CREATE TABLE %s (%s, %s %s, PRIMARY KEY (%s));"
+              (quot table-name)
+              (->> field-definitions
+                   (map (fn [{:keys [field-name base-type]}]
+                          (format "%s %s" (quot field-name) (field-base-type->sql-type dataset-loader base-type))))
+                   (interpose ", ")
+                   (apply str))
+              pk-field-name (pk-sql-type dataset-loader)
+              pk-field-name))))
 
 
-(defn drop-physical-table! [dataset-loader database-definition table-definition]
+(defn drop-physical-table! [dataset-loader database-definition {:keys [table-name]}]
   (execute-sql! dataset-loader database-definition
-    (format "DROP TABLE IF EXISTS \"%s\";" (:table-name table-definition))))
+    (format "DROP TABLE IF EXISTS %s;" (quote-name dataset-loader table-name)))) ; don't quote table name because MySQL doesn't like it
 
 
 (defn create-physical-db! [dataset-loader {:keys [table-definitions], :as database-definition}]
-  ;; Create all the Tables
-  (doseq [^TableDefinition table-definition table-definitions]
-    (i/create-physical-table! dataset-loader database-definition table-definition))
+  (let [quot (partial quote-name dataset-loader)]
+    ;; Create all the Tables
+    (doseq [^TableDefinition table-definition table-definitions]
+      (i/create-physical-table! dataset-loader database-definition table-definition))
 
-  ;; Now add the foreign key constraints
-  (doseq [{:keys [table-name field-definitions]} table-definitions]
-    (doseq [{dest-table-name :fk, field-name :field-name} field-definitions]
-      (when dest-table-name
-        (execute-sql! dataset-loader database-definition
-          (format "ALTER TABLE \"%s\" ADD CONSTRAINT \"FK_%s_%s\" FOREIGN KEY (\"%s\") REFERENCES \"%s\" (\"%s\");"
-                  table-name
-                  field-name (name dest-table-name)
-                  field-name
-                  (name dest-table-name)
-                  (pk-field-name dataset-loader)))))))
+    ;; Now add the foreign key constraints
+    (doseq [{:keys [table-name field-definitions]} table-definitions]
+      (doseq [{dest-table-name :fk, field-name :field-name} field-definitions]
+        (when dest-table-name
+          (let [dest-table-name (name dest-table-name)]
+            (execute-sql! dataset-loader database-definition
+              (format "ALTER TABLE %s ADD CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s);"
+                      (quot table-name)
+                      (quot (format "FK_%s_%s_%s" table-name field-name dest-table-name))
+                      (quot field-name)
+                      (quot dest-table-name)
+                      (quot (pk-field-name dataset-loader))))))))))
 
 
 (defn load-table-data! [dataset-loader database-definition table-definition]

test/metabase/test/data/mysql.clj

@@ -5,6 +5,7 @@
             [environ.core :refer [env]]
             (korma [core :as k]
                    [db :as kdb])
+            [metabase.driver.generic-sql.interface :refer [ISqlDriverQuoteName quote-name]]
             (metabase.test.data [generic-sql :as generic]
                                 [interface :refer :all]))
   (:import (metabase.test.data.interface DatabaseDefinition
@@ -24,18 +25,19 @@
    :TimeField       "TIME"})
 
 (defn- mysql-connection-details [^DatabaseDefinition {:keys [short-lived?]}]
-  (merge {:host         "localhost"
-          :port         3306
-          :short-lived? short-lived?}
-         ;; HACK
-         (when (env :circleci)
-           {:user "ubuntu"})))
+  {:host         "localhost"
+   :port         3306
+   :short-lived? short-lived?
+   :user         (if (env :circleci) "ubuntu"
+                     "root")})
 
 (defn- db-connection-details [^DatabaseDefinition database-definition]
   (assoc (mysql-connection-details database-definition)
-         :db (:database-name database-definition)))
+         :db (:database-name database-definition)
+         :timezone :America/Los_Angeles))
 
 (defn- execute! [scope ^DatabaseDefinition database-definition & format-strings]
+  (println "SQL -> " (apply format format-strings))
   (jdbc/execute! (-> ((case scope
                         :mysql mysql-connection-details
                         :db    db-connection-details) database-definition)
@@ -58,12 +60,16 @@
                         (assoc :make-pool? false)
                         kdb/create-db))))
 
-  (generic/pk-sql-type   [_] "MEDIUMINT NOT NULL AUTO_INCREMENT")
+  (generic/pk-sql-type   [_] "INTEGER NOT NULL AUTO_INCREMENT")
   (generic/pk-field-name [_] "id")
 
   (generic/field-base-type->sql-type [_ field-type]
     (if (map? field-type) (:native field-type)
-        (field-base-type->sql-type field-type))))
+        (field-base-type->sql-type field-type)))
+
+  ISqlDriverQuoteName
+  (quote-name [_ nm]
+    (str \` nm \`)))
 
 (extend-protocol IDatasetLoader
   MySQLDatasetLoader
@@ -75,7 +81,7 @@
            :timezone :America/Los_Angeles))
 
   (drop-physical-db! [_ database-definition]
-    (execute! :mysql database-definition "DROP DATABASE IF EXISTS \"%s\";" (:database-name database-definition)))
+    (execute! :mysql database-definition "DROP DATABASE IF EXISTS `%s`;" (:database-name database-definition)))
 
   (drop-physical-table! [this database-definition table-definition]
     (generic/drop-physical-table! this database-definition table-definition))
@@ -83,9 +89,9 @@
   (create-physical-table! [this database-definition table-definition]
     (generic/create-physical-table! this database-definition table-definition))
 
-  (create-physical-db! [this {:keys [database-name], :as database-definition}]
-    (execute! :mysql database-definition "DROP DATABASE IF EXISTS \"%s\";" database-name)
-    (execute! :mysql database-definition "CREATE DATABASE \"%s\";" database-name)
+  (create-physical-db! [this database-definition]
+    (drop-physical-db! this database-definition)
+    (execute! :mysql database-definition "CREATE DATABASE `%s`;" (:database-name database-definition))
 
     ;; double check that we can connect to the newly created DB
     (metabase.driver/can-connect-with-details? :mysql (db-connection-details database-definition) :rethrow-exceptions)

test/metabase/test/data/postgres.clj

@@ -84,7 +84,7 @@
     (generic/create-physical-table! this database-definition table-definition))
 
   (create-physical-db! [this {:keys [database-name], :as database-definition}]
-    (execute! :pg database-definition "DROP DATABASE IF EXISTS \"%s\";" database-name)
+    (drop-physical-db! this database-definition)
     (execute! :pg database-definition "CREATE DATABASE \"%s\";" database-name)
 
     ;; double check that we can connect to the newly created DB