fixes

src/metabase/driver/generic_sql/interface.clj

@@ -11,7 +11,7 @@
   (connection-details->connection-spec [this connection-details])
   (database->connection-details        [this database])
 
-  (unix-timestamp->date [this ^Keyword seconds-or-milliseconds field-or-value]
+  (unix-timestamp->timestamp [this ^Keyword seconds-or-milliseconds field-or-value]
     "Return a korma form appropriate for converting a Unix timestamp integer field or value to an proper SQL `Timestamp`.
      SECONDS-OR-MILLISECONDS refers to the resolution of the int in question and with be either `:seconds` or `:milliseconds`.")
 

src/metabase/driver/generic_sql/query_processor.clj

@@ -14,7 +14,9 @@
                                          [native :as native]
                                          [util :refer :all])
             [metabase.util :as u])
-  (:import (metabase.driver.query_processor.expand Field
+  (:import java.sql.Timestamp
+           java.util.Date
+           (metabase.driver.query_processor.expand Field
                                                    OrderByAggregateField
                                                    Value)))
 
@@ -86,6 +88,10 @@
 
 (defmethod apply-form :default [form]) ;; nothing
 
+(defn- cast-as-date
+  "Generate a korma form to cast FIELD-OR-VALUE to a `DATE`."
+  [field-or-value]
+  (utils/func "CAST(%s AS DATE)" [field-or-value]))
 
 (defprotocol IGenericSQLFormattable
   (formatted [this] [this include-as?]))
@@ -95,8 +101,15 @@
   (formatted
     ([this]
      (formatted this false))
-    ([{:keys [table-name field-name]} _]
-     (keyword (format "%s.%s" table-name field-name))))
+    ([{:keys [table-name base-type special-type field-name], :as field} include-as?]
+     (let [kw-name (keyword (str table-name \. field-name))
+           field   (cond
+                     (contains? #{:DateField :DateTimeField} base-type) (cast-as-date kw-name)
+                     (= special-type :timestamp_seconds)                (cast-as-date (i/unix-timestamp->timestamp (:driver *query*) kw-name :seconds))
+                     (= special-type :timestamp_milliseconds)           (cast-as-date (i/unix-timestamp->timestamp (:driver *query*) kw-name :milliseconds))
+                     :else                                              kw-name)]
+       (if include-as? [field (keyword field-name)]
+           field))))
 
   ;; e.g. the ["aggregation" 0] fields we allow in order-by
   OrderByAggregateField
@@ -118,23 +131,9 @@
      (formatted this false))
     ([{:keys [value base-type]} _]
      (cond
-       (instance? java.util.Date value) `(raw ~(format "CAST('%s' AS DATE)" (.toString ^java.util.Date value)))
-       (= base-type :UUIDField)         (do (assert (string? value))
-                                            (java.util.UUID/fromString value))
-       :else                            value))))
-
-  DateTimeField
-  (formatted
-    ([this]
-     (formatted this false))
-    ([{:keys [unit], {:keys [special-type field-name], :as field} :field} include-as?]
-     (let [f     (partial i/date (:driver *query*) unit)
-           field (cond
-                   (= special-type :timestamp_seconds)      (i/unix-timestamp->date (:driver *query*) (formatted field) :seconds)
-                   (= special-type :timestamp_milliseconds) (i/unix-timestamp->date (:driver *query*) (formatted field) :milliseconds)
-                   :else                                    (formatted field))]
-       (cond-> (f field)
-         include-as? (vector (keyword field-name)))))))
+       (instance? Timestamp value) (cast-as-date (u/date->yyyy-mm-dd value))
+       (= base-type :UUIDField)    (java.util.UUID/fromString value)
+       :else                       value))))
 
 
 (defmethod apply-form :aggregation [[_ {:keys [aggregation-type field]}]]

src/metabase/driver/h2.clj

@@ -111,10 +111,10 @@
 (defn- database->connection-details [_ {:keys [details]}]
   details)
 
-(defn- unix-timestamp->date [_ field-or-value seconds-or-milliseconds]
-  (utils/func (format "parseDateTime(%%s, '%s', 'en', 'GMT')" (case seconds-or-milliseconds
-                                                                :seconds      "s"
-                                                                :milliseconds "S"))
+(defn- unix-timestamp->timestamp [_ field-or-value seconds-or-milliseconds]
+  (utils/func (format "TIMESTAMPADD('%s', %%s, TIMESTAMP '1970-01-01T00:00:00Z')" (case seconds-or-milliseconds
+                                                                                    :seconds      "SECOND"
+                                                                                    :milliseconds "MILLISECOND"))
               [field-or-value]))
 
 (defn- wrap-process-query-middleware [_ qp]
@@ -137,7 +137,7 @@
 (extend H2Driver
   ISqlDriverDatabaseSpecific  {:connection-details->connection-spec connection-details->connection-spec
                                :database->connection-details        database->connection-details
-                               :unix-timestamp->date                unix-timestamp->date}
+                               :unix-timestamp->timestamp           unix-timestamp->timestamp}
   ;; Override the generic SQL implementation of wrap-process-query-middleware so we can block unsafe native queries (see above)
   IDriver                     (assoc GenericSQLIDriverMixin :wrap-process-query-middleware wrap-process-query-middleware)
   ISyncDriverTableFKs         GenericSQLISyncDriverTableFKsMixin

src/metabase/driver/mysql.clj

@@ -69,10 +69,10 @@
 (defn- database->connection-details [_ {:keys [details]}]
   details)
 
-(defn- unix-timestamp->date [_ field-or-value seconds-or-milliseconds]
+(defn- unix-timestamp->timestamp [_ field-or-value seconds-or-milliseconds]
   (utils/func (case seconds-or-milliseconds
                 :seconds      "FROM_UNIXTIME(%s)"
-                :milliseconds "FROM_UNIXTIME(%s * 1000)")
+                :milliseconds "FROM_UNIXTIME(%s / 1000)")
               [field-or-value]))
 
 (defn- timezone->set-timezone-sql [_ timezone]
@@ -86,7 +86,7 @@
 (extend MySQLDriver
   ISqlDriverDatabaseSpecific  {:connection-details->connection-spec connection-details->connection-spec
                                :database->connection-details        database->connection-details
-                               :unix-timestamp->date                unix-timestamp->date}
+                               :unix-timestamp->timestamp           unix-timestamp->timestamp}
   IDriver                     GenericSQLIDriverMixin
   ISyncDriverTableFKs         GenericSQLISyncDriverTableFKsMixin
   ISyncDriverFieldAvgLength   GenericSQLISyncDriverFieldAvgLengthMixin

src/metabase/driver/postgres.clj

@@ -102,10 +102,10 @@
                          port))
         (rename-keys {:dbname :db}))))
 
-(defn- unix-timestamp->date [_ field-or-value seconds-or-milliseconds]
+(defn- unix-timestamp->timestamp [_ field-or-value seconds-or-milliseconds]
   (utils/func (case seconds-or-milliseconds
                 :seconds      "TO_TIMESTAMP(%s)"
-                :milliseconds "TO_TIMESTAMP(%s * 1000)")
+                :milliseconds "TO_TIMESTAMP(%s / 1000)")
               [field-or-value]))
 
 (defn- timezone->set-timezone-sql [_ timezone]
@@ -124,7 +124,7 @@
 (extend PostgresDriver
   ISqlDriverDatabaseSpecific   {:connection-details->connection-spec connection-details->connection-spec
                                 :database->connection-details        database->connection-details
-                                :unix-timestamp->date                unix-timestamp->date
+                                :unix-timestamp->timestamp           unix-timestamp->timestamp
                                 :timezone->set-timezone-sql          timezone->set-timezone-sql}
   ISyncDriverSpecificSyncField {:driver-specific-sync-field!         driver-specific-sync-field!}
   IDriver                      GenericSQLIDriverMixin

src/metabase/driver/query_processor/expand.clj

@@ -227,7 +227,7 @@
             [table-name])
           field-name)))
 
-(defn- Field?
+(defn- unexpanded-Field?
   "Is this a valid value for a `Field` ID in an unexpanded query? (i.e. an integer or `fk->` form)."
   ;; ["aggregation" 0] "back-reference" form not included here since its specific to the order_by clause
   [field]
@@ -270,7 +270,7 @@
          :value (or (when (and (string? value)
                                (or (contains? #{:DateField :DateTimeField} base-type)
                                    (contains? #{:timestamp_seconds :timestamp_milliseconds} special-type)))
-                      (u/parse-date-yyyy-mm-dd value))
+                      (u/parse-iso8601 value))
                     value)))
 
 ;; Replace values with these during first pass over Query.
@@ -321,15 +321,15 @@
                         ^Field field])
 
 (defparser parse-aggregation
-  ["rows"]                              (->Aggregation :rows nil)
-  ["count"]                             (->Aggregation :count nil)
-  ["avg" (field-id :guard Field?)]      (->Aggregation :avg (ph field-id))
-  ["count" (field-id :guard Field?)]    (->Aggregation :count (ph field-id))
-  ["distinct" (field-id :guard Field?)] (->Aggregation :distinct (ph field-id))
-  ["stddev" (field-id :guard Field?)]   (do (assert-driver-supports :standard-deviation-aggregations)
-                                            (->Aggregation :stddev (ph field-id)))
-  ["sum" (field-id :guard Field?)]      (->Aggregation :sum (ph field-id))
-  ["cum_sum" (field-id :guard Field?)]  (->Aggregation :cumulative-sum (ph field-id)))
+  ["rows"]                                         (->Aggregation :rows nil)
+  ["count"]                                        (->Aggregation :count nil)
+  ["avg" (field-id :guard unexpanded-Field?)]      (->Aggregation :avg (ph field-id))
+  ["count" (field-id :guard unexpanded-Field?)]    (->Aggregation :count (ph field-id))
+  ["distinct" (field-id :guard unexpanded-Field?)] (->Aggregation :distinct (ph field-id))
+  ["stddev" (field-id :guard unexpanded-Field?)]   (do (assert-driver-supports :standard-deviation-aggregations)
+                                                       (->Aggregation :stddev (ph field-id)))
+  ["sum" (field-id :guard unexpanded-Field?)]      (->Aggregation :sum (ph field-id))
+  ["cum_sum" (field-id :guard unexpanded-Field?)]  (->Aggregation :cumulative-sum (ph field-id)))
 
 
 ;; ## -------------------- Breakout --------------------
@@ -380,7 +380,7 @@
   [v]
   (match v
     (_ :guard number?)         true
-    (_ :guard datetime-value?) true
+    (_ :guard u/date-string?)  true
     _                          false))
 
 (defn- Value?
@@ -392,7 +392,7 @@
       (orderable-Value? v)))
 
 (defparser parse-filter-subclause
-  ["INSIDE" (lat-field :guard Field?) (lon-field :guard Field?) (lat-max :guard number?) (lon-min :guard number?) (lat-min :guard number?) (lon-max :guard number?)]
+   ["INSIDE" (lat-field :guard unexpanded-Field?) (lon-field :guard unexpanded-Field?) (lat-max :guard number?) (lon-min :guard number?) (lat-min :guard number?) (lon-max :guard number?)]
   (map->Filter:Inside {:filter-type :inside
                        :lat         {:field (ph lat-field)
                                      :min   (ph lat-field lat-min)

src/metabase/util.clj

@@ -8,7 +8,11 @@
             [clj-time.coerce :as coerce])
   (:import (java.net Socket
                      InetSocketAddress
-                     InetAddress)))
+                     InetAddress)
+           java.sql.Timestamp
+           javax.xml.bind.DatatypeConverter))
+
+(set! *warn-on-reflection* true)
 
 (defmacro -assoc*
   "Internal. Don't use this directly; use `assoc*` instead."
@@ -33,45 +37,33 @@
   "`java.sql.Date` doesn't have an empty constructor so this is a convenience that lets you make one with the current date.
    (Some DBs like Postgres will get snippy if you don't use a `java.sql.Timestamp`)."
   []
-  (java.sql.Timestamp. (System/currentTimeMillis)))
+  (Timestamp. (System/currentTimeMillis)))
 
+;; Actually this only supports [RFC 3339](https://tools.ietf.org/html/rfc3339), which is basically a subset of ISO 8601
 (defn parse-iso8601
-  "Parse a string value expected in the iso8601 format into a `java.sql.Date`."
-  ^java.sql.Date
+  "Parse a string value expected in the iso8601 format into a `java.sql.Timestamp`.
+   NOTE: `YYYY-MM-DD` dates *are* valid iso8601 dates."
+  ^java.sql.Timestamp
   [^String datetime]
   (some->> datetime
-           (time/parse (time/formatters :date-time))
-           (coerce/to-long)
-           (java.sql.Date.)))
+           DatatypeConverter/parseDateTime
+           .getTime     ; Calendar -> Date
+           .getTime     ; Date -> ms
+           Timestamp.))
 
 (def ^:private ^java.text.SimpleDateFormat yyyy-mm-dd-simple-date-format
   (java.text.SimpleDateFormat. "yyyy-MM-dd"))
 
-(defn parse-date-yyyy-mm-dd
-  "Parse a date in the `yyyy-mm-dd` format and return a `java.sql.Date`."
-  ^java.sql.Date [^String date]
-  (-> (.parse yyyy-mm-dd-simple-date-format date)
-      .getTime
-      java.sql.Date.))
-
 (defn date->yyyy-mm-dd
   "Convert a date to a `YYYY-MM-DD` string."
   ^String [^java.util.Date date]
   (.format yyyy-mm-dd-simple-date-format 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 date-string?
-  "Is S a valid [RFC 3339](https://tools.ietf.org/html/rfc3339) date string?"
+  "Is S a valid ISO 8601 date string?"
   [s]
   (boolean (when (string? s)
-             (try (parse-rfc-3339 s)
+             (try (parse-iso8601 s)
                   (catch Throwable e)))))
 
 (defn now-iso8601

test/metabase/driver/query_processor_test.clj

@@ -770,41 +770,24 @@
 
 
 ;;; Unix timestamp breakouts -- SQL only
-(let [do-query (fn [] (->> (Q dataset sad-toucan-incidents
-                              aggregate count of incidents
-                              breakout timestamp
-                              limit 10
-                              return rows)
-                           (map (fn [[^java.util.Date date count]]
-                                  [(.toString date) (int count)]))))]
-
-  (datasets/expect-with-dataset :h2
-    [["2015-06-01" 6]
-     ["2015-06-02" 9]
-     ["2015-06-03" 5]
-     ["2015-06-04" 9]
-     ["2015-06-05" 8]
-     ["2015-06-06" 9]
-     ["2015-06-07" 8]
-     ["2015-06-08" 9]
-     ["2015-06-09" 7]
-     ["2015-06-10" 8]]
-    (do-query))
-
-  ;; postgres gives us *slightly* different answers because I think it's actually handling UNIX timezones properly (with timezone = UTC)
-  ;; as opposed to H2 which is giving us the wrong timezome. TODO - verify this
-  (datasets/expect-with-dataset :postgres
-    [["2015-06-01" 8]
-     ["2015-06-02" 9]
-     ["2015-06-03" 9]
-     ["2015-06-04" 4]
-     ["2015-06-05" 11]
-     ["2015-06-06" 8]
-     ["2015-06-07" 6]
-     ["2015-06-08" 10]
-     ["2015-06-09" 6]
-     ["2015-06-10" 10]]
-    (do-query)))
+(datasets/expect-with-datasets #{:h2 :postgres :mysql}
+  [["2015-06-01" 8]
+   ["2015-06-02" 9]
+   ["2015-06-03" 9]
+   ["2015-06-04" 4]
+   ["2015-06-05" 11]
+   ["2015-06-06" 8]
+   ["2015-06-07" 6]
+   ["2015-06-08" 10]
+   ["2015-06-09" 6]
+   ["2015-06-10" 10]]
+  (->> (Q dataset sad-toucan-incidents
+          aggregate count of incidents
+          breakout timestamp
+          limit 10
+          return rows)
+       (map (fn [[^java.util.Date date count]]
+              [(.toString date) (int count)]))))
 
 
 ;; +------------------------------------------------------------------------------------------------------------------------+