Merge pull request #1467 from metabase/unit-tests-for-datetime-granularities

make sure QP "annotate" returns the correct version of a field (fixes #1257)

project.clj

@@ -9,7 +9,6 @@
             "generate-sample-dataset" ["with-profile" "+generate-sample-dataset" "run"]}
   :dependencies [[org.clojure/clojure "1.7.0"]
                  [org.clojure/core.async "0.1.346.0-17112a-alpha"]
-                 [org.clojure/core.logic "0.8.10"]
                  [org.clojure/core.match "0.3.0-alpha4"]              ; optimized pattern matching library for Clojure
                  [org.clojure/core.memoize "0.5.7"]                   ; needed by core.match; has useful FIFO, LRU, etc. caching mechanisms
                  [org.clojure/data.csv "0.1.3"]                       ; CSV parsing / generation
@@ -17,7 +16,6 @@
                  [org.clojure/java.jdbc "0.4.2"]                      ; basic jdbc access from clojure
                  [org.clojure/math.numeric-tower "0.0.4"]             ; math functions like `ceil`
                  [org.clojure/tools.logging "0.3.1"]                  ; logging framework
-                 [org.clojure/tools.macro "0.1.5"]                    ; tools for writing macros
                  [org.clojure/tools.namespace "0.2.10"]
                  [amalloy/ring-gzip-middleware "0.1.3"]               ; Ring middleware to GZIP responses if client can handle it
                  [cheshire "5.5.0"]                                   ; fast JSON encoding (used by Ring JSON middleware)

src/metabase/util.clj

@@ -3,10 +3,10 @@
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.pprint :refer [pprint]]
             [clojure.tools.logging :as log]
-            [colorize.core :as color]
-            [medley.core :as m]
+            [clj-time.coerce :as coerce]
             [clj-time.format :as time]
-            [clj-time.coerce :as coerce])
+            [colorize.core :as color]
+            [medley.core :as m])
   (:import (java.net Socket
                      InetSocketAddress
                      InetAddress)
@@ -237,10 +237,13 @@
 
 (defn format-color
   "Like `format`, but uses a function in `colorize.core` to colorize the output.
-   COLOR-SYMB should be a symbol like `green`.
+   COLOR-SYMB should be a quoted symbol like `green`, `red`, `yellow`, `blue`,
+   `cyan`, `magenta`, etc. See the entire list of avaliable colors
+   [here](https://github.com/ibdknox/colorize/blob/master/src/colorize/core.clj).
 
      (format-color 'red \"Fatal error: %s\" error-message)"
   [color-symb format-string & args]
+  {:pre [(symbol? color-symb)]}
   ((ns-resolve 'colorize.core color-symb) (apply format format-string args)))
 
 (defn pprint-to-str
@@ -267,31 +270,81 @@
   [^Throwable e]
   (when e
     (when-let [stacktrace (.getStackTrace e)]
-      (->> (map str (.getStackTrace e))
-           (filterv (partial re-find #"metabase"))))))
-
-(defmacro try-apply
-  "Call F with PARAMS inside a try-catch block and log exceptions caught."
-  [f & params]
-  `(try
-     (~f ~@params)
-     (catch java.sql.SQLException e#
-       (log/error (color/red ~(format "Caught exception in %s: " f)
-                             (with-out-str (jdbc/print-sql-exception-chain e#))
-                             (pprint-to-str (filtered-stacktrace e#)))))
-     (catch Throwable e#
-       (log/error (color/red ~(format "Caught exception in %s: " f)
-                             (or (.getMessage e#) e#)
-                             (pprint-to-str (filtered-stacktrace e#)))))))
+      (filterv (partial re-find #"metabase")
+               (map str (.getStackTrace e))))))
+
+(defn wrap-try-catch
+  "Returns a new function that wraps F in a `try-catch`. When an exception is caught, it is logged
+   with `log/error` and returns `nil`."
+  ([f]
+   (wrap-try-catch f nil))
+  ([f f-name]
+   (let [exception-message (if f-name
+                             (format "Caught exception in %s: " f-name)
+                             "Caught exception: ")]
+     (fn [& args]
+       (try
+         (apply f args)
+         (catch java.sql.SQLException e
+           (log/error (color/red exception-message "\n"
+                                 (with-out-str (jdbc/print-sql-exception-chain e)) "\n"
+                                 (pprint-to-str (filtered-stacktrace e)))))
+         (catch Throwable e
+           (log/error (color/red exception-message (or (.getMessage e) e) "\n"
+                                 (pprint-to-str (filtered-stacktrace e))))))))))
+
+(defn try-apply
+  "Like `apply`, but wraps F inside a `try-catch` block and logs exceptions caught."
+  [^clojure.lang.IFn f & args]
+  (apply (wrap-try-catch f) args))
+
+(defn wrap-try-catch!
+  "Re-intern FN-SYMB as a new fn that wraps the original with a `try-catch`. Intended for debugging.
+
+     (defn z [] (throw (Exception. \"!\")))
+     (z) ; -> exception
+
+     (wrap-try-catch! 'z)
+     (z) ; -> nil; exception logged with log/error"
+  [fn-symb]
+  {:pre [(symbol? fn-symb)
+         (fn? @(resolve fn-symb))]}
+  (let [varr                    (resolve fn-symb)
+        {nmspc :ns, symb :name} (meta varr)]
+    (println (format "wrap-try-catch! %s/%s" nmspc symb))
+    (intern nmspc symb (wrap-try-catch @varr fn-symb))))
+
+(defn ns-wrap-try-catch!
+  "Re-intern all functions in NAMESPACE as ones that wrap the originals with a `try-catch`.
+   Defaults to the current namespace. You may optionally exclude a set of symbols using the kwarg `:exclude`.
+
+     (ns-wrap-try-catch!)
+     (ns-wrap-try-catch! 'metabase.driver)
+     (ns-wrap-try-catch! 'metabase.driver :exclude 'query-complete)
+
+   Intended for debugging."
+  {:arglists '([namespace? :exclude & excluded-symbs])}
+  [& args]
+  (let [[nmspc args] (optional #(try-apply the-ns [%]) args *ns*)
+        excluded     (when (= (first args) :exclude)
+                       (set (rest args)))]
+    (doseq [[symb varr] (ns-interns nmspc)]
+      (when (fn? @varr)
+        (when-not (contains? excluded symb)
+          (wrap-try-catch! (symbol (str (ns-name nmspc) \/ symb))))))))
+
+(defn deref-with-timeout
+  "Call `deref` on a FUTURE and throw an exception if it takes more than TIMEOUT-MS."
+  [futur timeout-ms]
+  (let [result (deref futur timeout-ms ::timeout)]
+    (when (= result ::timeout)
+      (throw (Exception. (format "Timed out after %d milliseconds." timeout-ms))))
+    result))
 
 (defmacro with-timeout
   "Run BODY in a `future` and throw an exception if it fails to complete after TIMEOUT-MS."
   [timeout-ms & body]
-  `(let [future# (future ~@body)
-         result# (deref future# ~timeout-ms :timeout)]
-     (when (= result# :timeout)
-       (throw (Exception. (format "Timed out after %d milliseconds." ~timeout-ms))))
-     result#))
+  `(deref-with-timeout (future ~@body) ~timeout-ms))
 
 (defmacro cond-as->
   "Anaphoric version of `cond->`. Binds EXPR to NAME through a series

src/metabase/util/logic.clj

-(ns metabase.util.logic
-  "Useful relations for `core.logic`."
-  (:refer-clojure :exclude [==])
-  (:require [clojure.core.logic :refer :all]))
-
-(defna butlast°
-  "A relation such that BUSTLASTV is all items but the last item LASTV of list L."
-  [butlastv lastv l]
-  ([[]   ?x [?x]])
-  ([_    _  [?x . ?more]] (fresh [more-butlast]
-                            (butlast° more-butlast lastv ?more)
-                            (conso ?x more-butlast butlastv))))
-
-(defna split°
-  "A relation such that HALF1 and HALF2 are even divisions of list L.
-   If L has an odd number of items, HALF1 will have one more item than HALF2."
-  [half1 half2 l]
-  ([[]   []   []])
-  ([[?x] []   [?x]])
-  ([[?x] [?y] [?x ?y]])
-  ([[?x ?y . ?more-half1-butlast] [?more-half1-last . ?more-half2] [?x ?y . ?more]]
-   (fresh [more-half1]
-     (split° more-half1 ?more-half2 ?more)
-     (butlast° ?more-half1-butlast ?more-half1-last more-half1))))
-
-(defn sorted-into°
-  "A relation such that OUT is the list L with V sorted into it doing comparisons with PRED-F."
-  [pred-f l v out]
-  (matche [l]
-    ([[]]           (== out [v]))
-    ([[?x . ?more]] (conda
-                     ((pred-f v ?x) (conso v (lcons ?x ?more) out))
-                     (s#            (fresh [more]
-                                      (sorted-into° pred-f ?more v more)
-                                      (conso ?x more out)))))))
-
-(defna sorted-permutation°
-  "A relation such that OUT is a permutation of L where all items are sorted by PRED-F."
-  [pred-f l out]
-  ([_ [] []])
-  ([_ [?x . ?more] _] (fresh [more]
-                        (sorted-permutation° pred-f ?more more)
-                        (sorted-into° pred-f more ?x out))))
-
-(defn matches-seq-order°
-  "A relation such that V1 is present and comes before V2 in list L."
-  [v1 v2 l]
-  (conda
-    ;; This is just an optimization for cases where L isn't a logic var; it's much faster <3
-    ((nonlvaro l)  ((fn -ordered° [[item & more]]
-                      (conda
-                        ((== v1 item)         s#)
-                        ((== v2 item)         fail)
-                        ((when (seq more) s#) (-ordered° more))))
-                    l))
-    (s#            (conda
-                     ((firsto l v1))
-                     ((firsto l v2) fail)
-                     ((fresh [more]
-                        (resto l more)
-                        (matches-seq-order° v1 v2 more)))))))

test/metabase/driver/query_processor_test.clj

@@ -1304,3 +1304,39 @@
                      :aggregation  ["count"]
                      :filter       ["TIME_INTERVAL" (id :checkins :timestamp) "last" "week"]}})
         :data :rows first first)))
+
+;; Make sure that when referencing the same field multiple times with different units we return the one
+;; that actually reflects the units the results are in.
+;; eg when we breakout by one unit and filter by another, make sure the results and the col info
+;; use the unit used by breakout
+(defn- date-bucketing-unit-when-you [& {:keys [breakout-by filter-by]}]
+  (with-temp-db [_ (checkins:1-per-day)]
+    (let [results (driver/process-query
+                   {:database (db-id)
+                    :type     :query
+                    :query     {:source_table (id :checkins)
+                                :aggregation  ["count"]
+                                :breakout     [["datetime_field" (id :checkins :timestamp) "as" breakout-by]]
+                                :filter       ["TIME_INTERVAL" (id :checkins :timestamp) "current" filter-by]}})]
+      {:rows (-> results :row_count)
+       :unit (-> results :data :cols first :unit)})))
+
+(datasets/expect-with-datasets sql-engines
+  {:rows 1, :unit :day}
+  (date-bucketing-unit-when-you :breakout-by "day", :filter-by "day"))
+
+(datasets/expect-with-datasets sql-engines
+  {:rows 7, :unit :day}
+  (date-bucketing-unit-when-you :breakout-by "day", :filter-by "week"))
+
+(datasets/expect-with-datasets sql-engines
+  {:rows 1, :unit :week}
+  (date-bucketing-unit-when-you :breakout-by "week", :filter-by "day"))
+
+(datasets/expect-with-datasets sql-engines
+  {:rows 1, :unit :quarter}
+  (date-bucketing-unit-when-you :breakout-by "quarter", :filter-by "day"))
+
+(datasets/expect-with-datasets sql-engines
+  {:rows 1, :unit :hour}
+  (date-bucketing-unit-when-you :breakout-by "hour", :filter-by "day"))