Merge pull request #467 from metabase/timestamp_support

Backend implementation for timestamp special type

src/metabase/driver/generic_sql/query_processor.clj

@@ -11,7 +11,8 @@
             [metabase.driver.generic-sql.query-processor.annotate :as annotate]
             (metabase.models [database :refer [Database]]
                              [field :refer [Field]]
-                             [table :refer [Table]])))
+                             [table :refer [Table]])
+            [metabase.util :as u]))
 
 
 (declare apply-form
@@ -122,6 +123,10 @@
 ;;     ["AND"
 ;;       [">" 1413 1]
 ;;       [">=" 1412 4]]
+
+(defn- field-id->special-type [field-id]
+  (sel :one :field [Field :special_type] :id field-id))
+
 (defn- filter-subclause->predicate
   "Given a filter SUBCLAUSE, return a Korma filter predicate form for use in korma `where`.
 
@@ -136,9 +141,11 @@
                                                                                 {lon-kw ['> lon-min]}]))
     [_ field-id & _] {(field-id->kw field-id)
                       ;; If the field in question is a date field we need to cast the YYYY-MM-DD string that comes back from the UI to a SQL date
-                      (let [cast-value-if-needed (if (date-field-id? field-id) (fn [value]
-                                                                                 `(raw ~(format "CAST('%s' AS DATE)" value)))
-                                                     identity)]
+                      (let [cast-value-if-needed (cond
+                                                   (date-field-id? field-id)            u/parse-date-yyyy-mm-dd
+                                                   (= (field-id->special-type field-id)
+                                                      :timestamp_seconds)               u/date-yyyy-mm-dd->unix-timestamp
+                                                   :else                                identity)]
                         (match subclause
                           ["NOT_NULL" _]        ['not= nil]
                           ["IS_NULL" _]         ['=    nil]

src/metabase/driver/mongo/query_processor.clj

@@ -307,9 +307,10 @@
                      `(ObjectId. ~value))]
     (memoize
      (fn [field-id]
-       (let [{base-type :base_type, field-name :name} (sel :one [Field :base_type :name] :id field-id)]
+       (let [{base-type :base_type, field-name :name, special-type :special_type} (sel :one [Field :base_type :name :special_type] :id field-id)]
          (cond
            (contains? #{:DateField :DateTimeField} base-type) u/parse-date-yyyy-mm-dd
+           (= special-type :timestamp_seconds)                u/date-yyyy-mm-dd->unix-timestamp
            (and (= field-name "_id")
                 (= base-type  :UnknownField))                 ->ObjectId
            :else                                              identity))))))

src/metabase/models/field.clj

@@ -24,27 +24,29 @@
     :name
     :number
     :state
+    :timestamp_seconds
     :url
     :zip_code})
 
 (def ^:const special-type->name
   "User-facing names for the `Field` special types."
-  {:avatar "Avatar Image URL"
-   :category "Category"
-   :city "City"
-   :country "Country"
-   :desc "Description"
-   :fk "Foreign Key"
-   :id "Entity Key"
-   :image "Image URL"
-   :json "Field containing JSON"
-   :latitude "Latitude"
-   :longitude "Longitude"
-   :name "Entity Name"
-   :number "Number"
-   :state "State"
-   :url "URL"
-   :zip_code "Zip Code"})
+  {:avatar            "Avatar Image URL"
+   :category          "Category"
+   :city              "City"
+   :country           "Country"
+   :desc              "Description"
+   :fk                "Foreign Key"
+   :id                "Entity Key"
+   :image             "Image URL"
+   :json              "Field containing JSON"
+   :latitude          "Latitude"
+   :longitude         "Longitude"
+   :name              "Entity Name"
+   :number            "Number"
+   :state             "State"
+   :timestamp_seconds "Timestamp - seconds since 1970"
+   :url               "URL"
+   :zip_code          "Zip Code"})
 
 (def ^:const base-types
   "Possible values for `Field` `:base_type`."

src/metabase/util.clj

@@ -100,19 +100,27 @@
 (defn parse-iso8601
   "Parse a string value expected in the iso8601 format into a `java.sql.Date`."
   ^java.sql.Date
-  [datetime]
+  [^String datetime]
   (some->> datetime
            (time/parse (time/formatters :date-time))
            (coerce/to-long)
            (java.sql.Date.)))
 
 
-(def ^{:arglists '([date])} parse-date-yyyy-mm-dd
+(def ^{:arglists '([date])} ^java.util.Date parse-date-yyyy-mm-dd
   "Parse a date in the `yyyy-mm-dd` format and return a `java.util.Date`."
   (let [sdf (java.text.SimpleDateFormat. "yyyy-MM-dd")]
-    (fn [date]
+    (fn [^String date]
       (.parse sdf date))))
 
+(defn date-yyyy-mm-dd->unix-timestamp
+  "Convert a string DATE in the `YYYY-MM-DD` format to a Unix timestamp in seconds."
+  ^Float [^String date]
+  (-> date
+      parse-date-yyyy-mm-dd
+      .getTime
+      (/ 1000)))
+
 (defn now-iso8601
   "format the current time as iso8601 date/time string."
   []

test/metabase/driver/query_processor_test.clj

@@ -6,7 +6,9 @@
             [metabase.driver.query-processor :refer :all]
             (metabase.models [field :refer [Field]]
                              [table :refer [Table]])
-            [metabase.test.data.datasets :as datasets :refer [*dataset*]]))
+            [metabase.test.data :refer [with-temp-db]]
+            (metabase.test.data [dataset-definitions :refer [us-history-1607-to-1774]]
+                                [datasets :as datasets :refer [*dataset*]])))
 
 ;; ##  Dataset-Independent Data Fns
 
@@ -41,6 +43,9 @@
 (defn timestamp-field-type []
   (datasets/timestamp-field-type *dataset*))
 
+(defn dataset-loader []
+  (datasets/dataset-loader *dataset*))
+
 
 ;; ## Dataset-Independent QP Tests
 
@@ -781,3 +786,19 @@
         :aggregation ["rows"],
         :order_by [[(id :users :id) "ascending"]]}})
      (update-in [:data :rows] (partial mapv (partial filterv #(not (isa? (type %) java.util.Date)))))))
+
+
+;; ## Unix timestamp special type fields <3
+
+;; There are 4 events in us-history-1607-to-1774 whose year is < 1765
+(datasets/expect-with-all-datasets 4
+ (with-temp-db [db (dataset-loader) us-history-1607-to-1774]
+   (-> (driver/process-query {:database (:id db)
+                              :type     :query
+                              :query    {:source_table (:id &events)
+                                         :aggregation  ["count"]
+                                         :filter       ["<" (:id &events.timestamp) "1765-01-01"]}})
+       :data
+       :rows
+       first
+       first)))

test/metabase/test/data.clj

 (ns metabase.test.data
   "Code related to creating and deleting test databases + datasets."
-  (:require [clojure.string :as s]
+  (:require (clojure [string :as s]
+                     [walk :as walk])
             [clojure.tools.logging :as log]
             [colorize.core :as color]
-            [metabase.db :refer :all]
-            [metabase.driver :as driver]
+            [medley.core :as m]
+            (metabase [db :refer :all]
+                      [driver :as driver])
             (metabase.models [database :refer [Database]]
                              [field :refer [Field] :as field]
                              [table :refer [Table]])
@@ -33,7 +35,8 @@
           (log/info (color/blue "Loading data..."))
           (doseq [^TableDefinition table-definition (:table-definitions database-definition)]
             (log/info (color/blue (format "Loading data for table '%s'..." (:table-name table-definition))))
-            (load-table-data! dataset-loader database-definition table-definition))
+            (load-table-data! dataset-loader database-definition table-definition)
+            (log/info (color/blue (format "Inserted %d rows." (count (:rows table-definition))))))
 
           ;; Add DB object to Metabase DB
           (log/info (color/blue "Adding DB to Metabase..."))
@@ -124,14 +127,77 @@
 
 ;; ## Temporary Dataset Macros
 
+;; The following functions are used internally by with-temp-db to implement easy Table/Field lookup
+;; with `$table` and `$table.field` forms.
+
+(defn- table-id->field-name->field
+  "Return a map of lowercased `Field` names -> fields for `Table` with TABLE-ID."
+  [table-id]
+  (->> (sel :many :field->obj [Field :name] :table_id table-id)
+       (m/map-keys s/lower-case)))
+
+(defn- db-id->table-name->table
+  "Return a map of lowercased `Table` names -> Tables for `Database` with DATABASE-ID.
+   Add a delay `:field-name->field` to each Table that calls `table-id->field-name->field` for that Table."
+  [database-id]
+  (->> (sel :many :field->obj [Table :name] :db_id database-id)
+       (m/map-keys s/lower-case)
+       (m/map-vals (fn [table]
+                     (assoc table :field-name->field (delay (table-id->field-name->field (:id table))))))))
+
+(defn -temp-db-add-getter-delay
+  "Add a delay `:table-name->table` to DB that calls `db-id->table-name->table`."
+  [db]
+  (assoc db :table-name->table (delay (db-id->table-name->table (:id db)))))
+
+(defn -temp-get
+  "Internal - don't call this directly.
+   With two args, fetch `Table` with TABLE-NAME using `:table-name->table` delay on TEMP-DB.
+   With three args, fetch `Field` with FIELD-NAME by recursively fetching `Table` and using its `:field-name->field` delay."
+  ([temp-db table-name]
+   {:pre [(map? temp-db)
+          (string? table-name)]}
+   (@(:table-name->table temp-db) table-name))
+  ([temp-db table-name field-name]
+   {:pre [(string? field-name)]}
+   (@(:field-name->field (-temp-get temp-db table-name)) field-name)))
+
+(defn- walk-expand-&
+  "Walk BODY looking for symbols like `&table` or `&table.field` and expand them to appropriate `-temp-get` forms."
+  [db-binding body]
+  (walk/prewalk
+   (fn [form]
+     (or (when (symbol? form)
+           (when-let [symbol-name (re-matches #"^&.+$" (name form))]
+             `(-temp-get ~db-binding ~@(-> symbol-name
+                                           (s/replace #"&" "")
+                                           (s/split #"\.")))))
+         form))
+   body))
+
 (defmacro with-temp-db
   "Load and sync DATABASE-DEFINITION with DATASET-LOADER and execute BODY with
    the newly created `Database` bound to DB-BINDING.
-   Remove `Database` and destroy data afterward."
+   Remove `Database` and destroy data afterward.
+
+   Within BODY, symbols like `&table` and `&table.field` will be expanded into function calls to
+   fetch corresponding `Tables` and `Fields`. These are accessed via lazily-created maps of
+   Table/Field names to the objects themselves. To facilitate mutli-driver tests, these names are lowercased.
+
+     (with-temp-db [db (h2/dataset-loader) us-history-1607-to-1774]
+       (driver/process-quiery {:database (:id db)
+                               :type     :query
+                               :query    {:source_table (:id &events)
+                                          :aggregation  [\"count\"]
+                                          :filter       [\"<\" (:id &events.timestamp) \"1765-01-01\"]}}))"
   [[db-binding dataset-loader ^DatabaseDefinition database-definition] & body]
   `(let [loader# ~dataset-loader
-         dbdef# ~database-definition]
-     (try (let [~db-binding (get-or-create-database! loader# dbdef#)]
-            ~@body)
-          (finally
-            (remove-database! loader# dbdef#)))))
+         ;; Add :short-lived? to the database definition so dataset loaders can use different connection options if desired
+         dbdef# (map->DatabaseDefinition (assoc ~database-definition :short-lived? true))]
+     (try
+       (remove-database! loader# dbdef#)                              ; Remove DB if it already exists for some weird reason
+       (let [~db-binding (-> (get-or-create-database! loader# dbdef#)
+                             -temp-db-add-getter-delay)]              ; Add the :table-name->table delay used by -temp-get
+         ~@(walk-expand-& db-binding body))                           ; expand $table and $table.field forms into -temp-get calls
+       (finally
+         (remove-database! loader# dbdef#)))))

test/metabase/test/data/dataset_definitions.clj

+(ns metabase.test.data.dataset-definitions
+  "Definitions of various datasets for use in tests with `with-temp-db`."
+  (:require [metabase.test.data.interface :refer [def-database-definition]]))
+
+;; ## Helper Functions
+
+(defn create-unix-timestamp
+  "Create a Unix timestamp (in seconds).
+
+     (create-unix-timestamp :year 2012 :month 12 :date 27)"
+  ^Long [& {:keys [year month date hour minute second nano]
+            :or   {year 0, month 1, date 1, hour 0, minute 0, second 0, nano 0}}]
+  (-> (java.sql.Timestamp. (- year 1900) (- month 1) date hour minute second nano)
+      .getTime
+      (/ 1000)
+      long)) ; coerce to long since Korma doesn't know how to insert bigints
+
+
+;; ## Datasets
+
+(def-database-definition us-history-1607-to-1774
+  ["events" [{:field-name   "name"
+              :base-type    :CharField}
+             {:field-name   "timestamp"
+              :base-type    :BigIntegerField
+              :special-type :timestamp_seconds}]
+   [["Jamestown Settlement Founded"    (create-unix-timestamp :year 1607 :month  5 :date 14)]
+    ["Mayflower Compact Signed"        (create-unix-timestamp :year 1620 :month 11 :date 11)]
+    ["Ben Franklin's Kite Experiment"  (create-unix-timestamp :year 1752 :month 96 :date 15)]
+    ["French and Indian War Begins"    (create-unix-timestamp :year 1754 :month  5 :date 28)]
+    ["Stamp Act Enacted"               (create-unix-timestamp :year 1765 :month  3 :date 22)]
+    ["Quartering Act Enacted"          (create-unix-timestamp :year 1765 :month  3 :date 24)]
+    ["Stamp Act Congress Meets"        (create-unix-timestamp :year 1765 :month 10 :date 19)]
+    ["Stamp Act Repealed"              (create-unix-timestamp :year 1766 :month  3 :date 18)]
+    ["Townshend Acts Passed"           (create-unix-timestamp :year 1767 :month  6 :date 29)]
+    ["Boston Massacre"                 (create-unix-timestamp :year 1770 :month  3 :date  5)]
+    ["Tea Act Passed"                  (create-unix-timestamp :year 1773 :month  5 :date 10)]
+    ["Boston Tea Party"                (create-unix-timestamp :year 1773 :month 12 :date 16)]
+    ["Boston Port Act Passed"          (create-unix-timestamp :year 1774 :month  3 :date 31)]
+    ["First Continental Congress Held" (create-unix-timestamp :year 1774 :month  9 :date  5)]]])

test/metabase/test/data/datasets.clj

@@ -6,7 +6,9 @@
             [environ.core :refer [env]]
             [expectations :refer :all]
             [metabase.driver.mongo.test-data :as mongo-data]
-            [metabase.test.data :as generic-sql-data]))
+            [metabase.test.data :as generic-sql-data]
+            (metabase.test.data [h2 :as h2]
+                                [mongo :as mongo])))
 
 ;; # IDataset
 
@@ -14,6 +16,8 @@
   "Functions needed to fetch test data for various drivers."
   (load-data! [this]
     "Load the test data for this dataset.")
+  (dataset-loader [this]
+    "Return a dataset loader (an object that implements `IDatasetLoader`) for this dataset/driver.")
   (db [this]
     "Return `Database` containing test data for this driver.")
   (table-name->table [this table-name]
@@ -42,6 +46,8 @@
   (load-data! [_]
     @mongo-data/mongo-test-db
     (assert (integer? @mongo-data/mongo-test-db-id)))
+  (dataset-loader [_]
+    (mongo/dataset-loader))
   (db [_]
     @mongo-data/mongo-test-db)
   (table-name->table [_ table-name]
@@ -69,6 +75,8 @@
   (load-data! [_]
     @generic-sql-data/test-db
     (assert (integer? @generic-sql-data/db-id)))
+  (dataset-loader [_]
+    (h2/dataset-loader))
   (db [this]
     @generic-sql-data/test-db)
   (table-name->table [_ table-name]

test/metabase/test/data/h2.clj

@@ -10,62 +10,59 @@
                                          FieldDefinition
                                          TableDefinition)))
 
-;; ## DatabaseDefinition extensions
-
-(defprotocol IH2DatabaseDefinition
-  "Additional methods for `DatabaseDefinition` used by the H2 dataset loader."
-  (filename ^String [this]
-    "Return filename that should be used for connecting to and H2 instance of this database (not including the `.mv.db` extension).")
-
-  (connection-details [this]
-    "Return a Metabase `Database.details` for an H2 instance of this database.")
-
-  (korma-connection-pool [this]
-    "Return an H2 korma connection pool to this database.")
-
-  (exec-sql [this ^String raw-sql]
-    "Execute RAW-SQL against H2 instance of this database."))
-
-(extend-protocol IH2DatabaseDefinition
-  DatabaseDefinition
-  (filename [this]
-    (format "%s/target/%s" (System/getProperty "user.dir") (escaped-name this)))
-
-  (connection-details [this]
-    {:db (format "file:%s;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1" (filename this))})
-
-  (korma-connection-pool [this]
-    (kdb/create-db (kdb/h2 (assoc (connection-details this)
+;; ## DatabaseDefinition helper functions
+
+(defn filename
+  "Return filename that should be used for connecting to H2 database defined by DATABASE-DEFINITION.
+   This does not include the `.mv.db` extension."
+  [^DatabaseDefinition database-definition]
+  (format "%s/target/%s" (System/getProperty "user.dir") (escaped-name database-definition)))
+
+(defn connection-details
+  "Return a Metabase `Database.details` for H2 database defined by DATABASE-DEFINITION."
+  [^DatabaseDefinition database-definition]
+  {:db (format (if (:short-lived? database-definition) "file:%s" ; for short-lived connections don't create a server thread and don't use a keep-alive connection
+                   "file:%s;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1")
+               (filename database-definition))})
+
+(defn korma-connection-pool
+  "Return an H2 korma connection pool to H2 database defined by DATABASE-DEFINITION."
+  [^DatabaseDefinition database-definition]
+  (kdb/create-db (kdb/h2 (assoc (connection-details database-definition)
                                   :naming {:keys   s/lower-case
                                            :fields s/upper-case}))))
 
-  (exec-sql [this raw-sql]
-    (log/info raw-sql)
-    (k/exec-raw (korma-connection-pool this) raw-sql)))
+(defn exec-sql
+  "Execute RAW-SQL against H2 instance of H2 database defined by DATABASE-DEFINITION."
+  [^DatabaseDefinition database-definition ^String raw-sql]
+  (log/info raw-sql)
+  (k/exec-raw (korma-connection-pool database-definition) raw-sql))
 
 
-;; ## TableDefinition extensions
+;; ## TableDefinition helper functions
 
-(defprotocol IH2TableDefinition
-  "Additional methods for `TableDefinition` used by the H2 dataset loader."
-  (korma-entity [this ^DatabaseDefinition database-definition]))
-
-(extend-protocol IH2TableDefinition
-  TableDefinition
-  (korma-entity [this database-definition]
-    (-> (k/create-entity (:table-name this))
-        (k/database (korma-connection-pool database-definition)))))
+(defn korma-entity
+  "Return a Korma entity (e.g., one that can be passed to `select` or `sel` for the table
+   defined by TABLE-DEFINITION in the H2 database defined by DATABASE-DEFINITION."
+  [^TableDefinition table-definition ^DatabaseDefinition database-definition]
+  (-> (k/create-entity (:table-name table-definition))
+      (k/database (korma-connection-pool database-definition))))
 
 
 ;; ## Internal Stuff
 
 (def ^:private ^:const field-base-type->sql-type
   "Map of `Field.base_type` to the SQL type we should use for that column when creating a DB."
-  {:CharField     "VARCHAR(254)"
-   :DateField     "DATE"
-   :DateTimeField "TIMESTAMP"
-   :FloatField    "DOUBLE"
-   :IntegerField  "INTEGER"})
+  {:BigIntegerField "BIGINT"
+   :BooleanField    "BOOL"
+   :CharField       "VARCHAR(254)"
+   :DateField       "DATE"
+   :DateTimeField   "DATETIME"
+   :DecimalField    "DECIMAL"
+   :FloatField      "FLOAT"
+   :IntegerField    "INTEGER"
+   :TextField       "TEXT"
+   :TimeField       "TIME"})
 
 ;; ## Public Concrete DatasetLoader instance
 
@@ -121,14 +118,11 @@
                        dest-table-name))))))))
 
   (load-table-data! [_ database-definition table-definition]
-    (log/info (format "Loading data for %s..." (:table-name table-definition)))
     (let [rows              (:rows table-definition)
           fields-for-insert (map :field-name (:field-definitions table-definition))]
-
       (-> (korma-entity table-definition database-definition)
           (k/insert (k/values (map (partial zipmap fields-for-insert)
-                                   rows))))
-      (log/info (format "Inserted %d rows." (count rows)))))
+                                   rows))))))
 
   (drop-physical-table! [_ database-definition table-definition]
     (exec-sql

test/metabase/test/data/interface.clj

@@ -21,17 +21,15 @@
                             rows])
 
 (defrecord DatabaseDefinition [^String database-name
-                               table-definitions])
+                               table-definitions
+                               ;; Optional. Set this to non-nil to let dataset loaders know that we don't intend to keep it
+                               ;; for long -- they can adjust connection behavior, e.g. choosing simple connections instead of creating pools.
+                               ^Boolean short-lived?])
 
-
-(defprotocol IEscapedName
-  (^String escaped-name [this]
-    "Return escaped version of DATABASE-NAME suitable for use as a filename / database name / etc."))
-
-(extend-protocol IEscapedName
-  DatabaseDefinition
-  (escaped-name [this]
-    (s/replace (:database-name this) #"\s+" "_")))
+(defn escaped-name
+  "Return escaped version of database name suitable for use as a filename / database name / etc."
+  ^String [^DatabaseDefinition database-definition]
+  (s/replace (:database-name database-definition) #"\s+" "_"))
 
 
 (defprotocol IMetabaseInstance
@@ -106,8 +104,7 @@
   ^TableDefinition [^String table-name field-definition-maps rows]
   (map->TableDefinition {:table-name          table-name
                          :rows                rows
-                         :field-definitions   (mapv create-field-definition field-definition-maps)
-                         :database-definition (promise)}))
+                         :field-definitions   (mapv create-field-definition field-definition-maps)}))
 
 (defn create-database-definition
   "Convenience for creating a new `DatabaseDefinition`."
@@ -117,3 +114,11 @@
   (map->DatabaseDefinition {:database-name     database-name
                             :table-definitions (mapv (partial apply create-table-definition)
                                                      table-name+field-definition-maps+rows)}))
+
+(defmacro def-database-definition
+  "Convenience for creating a new `DatabaseDefinition` named by the symbol DATASET-NAME."
+  [^clojure.lang.Symbol dataset-name & table-name+field-definition-maps+rows]
+  {:pre [(symbol? dataset-name)]}
+  `(def ~(vary-meta dataset-name assoc :tag DatabaseDefinition)
+     (create-database-definition ~(name dataset-name)
+       ~@table-name+field-definition-maps+rows)))