diff --git a/modules/drivers/druid/test/metabase/driver/druid/client_test.clj b/modules/drivers/druid/test/metabase/driver/druid/client_test.clj index 8bc0765b56fbaed529e88b7a1d6a6e323fb189d0..b252e345c9a97682f2660f63379e0ef2a557df6b 100644 --- a/modules/drivers/druid/test/metabase/driver/druid/client_test.clj +++ b/modules/drivers/druid/test/metabase/driver/druid/client_test.clj @@ -32,25 +32,25 @@ (deftest ssh-tunnel-test (mt/test-driver :druid - (is (thrown? - com.jcraft.jsch.JSchException - (try - (let [engine :druid - details {:ssl false - :password "changeme" - :tunnel-host "localhost" - :tunnel-pass "BOGUS-BOGUS" - :port 5432 - :dbname "test" - :host "http://localhost" - :tunnel-enabled true - :tunnel-port 22 - :tunnel-user "bogus"}] + (let [engine :druid + details {:ssl false + :password "changeme" + :tunnel-host "localhost" + :tunnel-pass "BOGUS-BOGUS" + :port 5432 + :dbname "test" + :host "http://localhost" + :tunnel-enabled true + :tunnel-port 22 + :tunnel-user "bogus"}] + (is (thrown? + com.jcraft.jsch.JSchException + (try (tu.log/suppress-output - (driver.u/can-connect-with-details? engine details :throw-exceptions))) - (catch Throwable e - (loop [^Throwable e e] - (or (when (instance? com.jcraft.jsch.JSchException e) - (throw e) - e) - (some-> (.getCause e) recur))))))))) + (driver.u/can-connect-with-details? engine details :throw-exceptions)) + (catch Throwable e + (loop [^Throwable e e] + (or (when (instance? com.jcraft.jsch.JSchException e) + (throw e) + e) + (some-> (ex-cause e) recur)))))))))) diff --git a/modules/drivers/redshift/src/metabase/driver/redshift.clj b/modules/drivers/redshift/src/metabase/driver/redshift.clj index 18bc1060421359e695b7504dfb9f304ef844d64e..e1c789ca66288be896365d56416d0bf53a56e8d3 100644 --- a/modules/drivers/redshift/src/metabase/driver/redshift.clj +++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj @@ -99,7 +99,6 @@ [_] "SET TIMEZONE TO %s;") - (defn- splice-raw-string-value [driver s] (hsql/raw (str "'" (sql.qp/->honeysql driver s) "'"))) diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj index 97d5edb7d7be293bbb87509cdf65b8ec2f15bf1b..284d17f3afd651d0c009f0f68790d9aefd46d609 100644 --- a/src/metabase/driver.clj +++ b/src/metabase/driver.clj @@ -326,12 +326,13 @@ Example impl: + (defmethod reducible-query :my-driver - [_ query {:keys [canceled-chan], :as context} respond] + [_ query context respond] (with-open [results (run-query! query)] (respond {:cols [{:name \"my_col\"}]} - (qp.reducible/reducible-rows (get-row results) canceled-chan))))" + (qp.reducible/reducible-rows (get-row results) (context/canceled-chan context)))))" {:added "0.35.0", :arglists '([driver query context respond])} dispatch-on-initialized-driver :hierarchy #'hierarchy) diff --git a/src/metabase/models/database.clj b/src/metabase/models/database.clj index 0b2f68bf96a9184df5d32540ec092c08252db806..fdc5e5d64d388060eb9dc2a10d54dc6ecad5b2d9 100644 --- a/src/metabase/models/database.clj +++ b/src/metabase/models/database.clj @@ -61,7 +61,10 @@ (db/delete! 'Card :database_id id) (db/delete! 'Permissions :object [:like (str (perms/object-path id) "%")]) (db/delete! 'Table :db_id id) - (driver/notify-database-updated driver database)) + (try + (driver/notify-database-updated driver database) + (catch Throwable e + (log/error e (trs "Error sending database deletion notification"))))) ;; TODO - this logic would make more sense in post-update if such a method existed (defn- pre-update diff --git a/src/metabase/query_processor/middleware/cache.clj b/src/metabase/query_processor/middleware/cache.clj index 4feff17fd2682e2d35679594962e3cbd0c7fdb0a..7ef5f0a11b9b1217a9ecdff597b12099a07b6750 100644 --- a/src/metabase/query_processor/middleware/cache.clj +++ b/src/metabase/query_processor/middleware/cache.clj @@ -192,6 +192,8 @@ * The result *rows* of the query must be less than `query-caching-max-kb` when serialized (before compression)." [qp] (fn [query rff context] - (if (is-cacheable? query) - (run-query-with-cache qp query rff context) - (qp query rff context)))) + (let [cacheable? (is-cacheable? query)] + (log/tracef "Query is cacheable? %s" (boolean cacheable?)) + (if cacheable? + (run-query-with-cache qp query rff context) + (qp query rff context))))) diff --git a/src/metabase/query_processor/middleware/resolve_database_and_driver.clj b/src/metabase/query_processor/middleware/resolve_database_and_driver.clj index 0c67c75e0640beaf851eab3dd0b992ad2f09f625..3b0d3dbb8a6efee773ea7473aef30982b5ebf976 100644 --- a/src/metabase/query_processor/middleware/resolve_database_and_driver.clj +++ b/src/metabase/query_processor/middleware/resolve_database_and_driver.clj @@ -28,5 +28,6 @@ (driver/the-initialized-driver (driver.u/database->driver (qp.store/database))) (catch Throwable e (throw (ex-info (tru "Unable to resolve driver for query") - {:type error-type/invalid-query})))) + {:type error-type/invalid-query} + e)))) (qp query rff context)))) diff --git a/src/metabase/query_processor/reducible.clj b/src/metabase/query_processor/reducible.clj index fd9fc90a1698dc9e58a75be6d5e3b0c0c258e7c6..2a6f4fd209b6db420d437a2966f9f1a103fe8e1f 100644 --- a/src/metabase/query_processor/reducible.clj +++ b/src/metabase/query_processor/reducible.clj @@ -68,6 +68,12 @@ (async.u/promise-pipe out-chan out-chan*) out-chan*)) +(def ^:dynamic *run-on-separate-thread?* + "Whether to run the query on a separate thread. When running a query asynchronously (i.e., with `async-qp`), this is + normally `true`, meaning the `out-chan` is returned immediately. When running a query synchronously (i.e., with + `sync-qp`), this is normally `false`, becuase we are blocking while waiting for results." + true) + (defn async-qp "Wrap a QP function (middleware or a composition of middleware created with `combine-middleware`) with the signature: @@ -89,10 +95,14 @@ {:pre [(map? query) ((some-fn nil? map?) context)]} (let [context (merge (context.default/default-context) context)] (wire-up-context-channels! context) - (try - (qp query (context/rff context) context) - (catch Throwable e - (context/raisef e context))) + (let [thunk (fn [] (try + (qp query (context/rff context) context) + (catch Throwable e + (context/raisef e context))))] + (log/tracef "Running on separate thread? %s" *run-on-separate-thread?*) + (if *run-on-separate-thread?* + (future (thunk)) + (thunk))) (quittable-out-chan (context/out-chan context)))))) (defn- wait-for-async-result [out-chan] @@ -111,7 +121,14 @@ (qp query context)" [qp] {:pre [(fn? qp)]} - (comp wait-for-async-result qp)) + (fn qp* + ([query] + (wait-for-async-result (binding [*run-on-separate-thread?* false] + (qp query)))) + + ([query context] + (wait-for-async-result (binding [*run-on-separate-thread?* false] + (qp query context)))))) ;;; ------------------------------------------------- Other Util Fns ------------------------------------------------- diff --git a/test/metabase/async/streaming_response_test.clj b/test/metabase/async/streaming_response_test.clj index c64fb3c87519097daa08921d72a337efde3bf402..ce89f9b45816322939e881672bba1eb37f97661e 100644 --- a/test/metabase/async/streaming_response_test.clj +++ b/test/metabase/async/streaming_response_test.clj @@ -46,8 +46,12 @@ [_ {{{:keys [sleep]} :query} :native, database-id :database} context respond] {:pre [(integer? sleep) (integer? database-id)]} (let [futur (future - (Thread/sleep sleep) - (respond {:cols [{:name "Sleep", :base_type :type/Integer}]} [[sleep]]))] + (try + (Thread/sleep sleep) + (respond {:cols [{:name "Sleep", :base_type :type/Integer}]} [[sleep]]) + (catch InterruptedException e + (reset! canceled? true) + (throw e))))] (a/go (when (a/<! (context/canceled-chan context)) (reset! canceled? true) @@ -88,9 +92,10 @@ (is (= {:status "ok"} (test-client/client :get 200 "health"))) (testing "Health endpoint should complete before the first round of queries completes" (is (> @remaining (inc (- num-requests thread-pool-size))))) - (testing "Health endpoint should complete in under 100ms regardless of how many queries are running" - (let [elapsed-ms (- (System/currentTimeMillis) start-time-ms)] - (is (< elapsed-ms 100)))))))))) + (testing "Health endpoint should complete in under 500ms regardless of how many queries are running" + (testing "(Usually this is under 100ms but might be a little over if CircleCI is being slow)" + (let [elapsed-ms (- (System/currentTimeMillis) start-time-ms)] + (is (< elapsed-ms 500))))))))))) (deftest newlines-test (testing "Keepalive newlines should be written while waiting for a response." diff --git a/test/metabase/query_processor/middleware/cache_test.clj b/test/metabase/query_processor/middleware/cache_test.clj index 54e012a040b4e50cd9317defb62959911df35b4e..dc17f089fee30d676674f86546c244d1c6eb9dd1 100644 --- a/test/metabase/query_processor/middleware/cache_test.clj +++ b/test/metabase/query_processor/middleware/cache_test.clj @@ -66,6 +66,7 @@ (boolean (#'cache/is-cacheable? {:cache-ttl cache-ttl}))))))))) (defn- cached? [result] + (assert (some? result)) (if (:cached result) :cached :not-cached)) diff --git a/test/metabase/query_processor/middleware/format_rows_test.clj b/test/metabase/query_processor/middleware/format_rows_test.clj index 65941e2b7137437ff02f2259708182aec16f2ae5..15c62cf18ddad9f864b9324659bfe0a9be80c974 100644 --- a/test/metabase/query_processor/middleware/format_rows_test.clj +++ b/test/metabase/query_processor/middleware/format_rows_test.clj @@ -19,48 +19,49 @@ (deftest format-rows-test (mt/test-drivers (mt/normal-drivers-except dbs-exempt-from-format-rows-tests) - (testing "without report timezone" - (is (= (if (= driver/*driver* :sqlite) - ;; TIMEZONE FIXME - [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00"] - [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00"] - [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00"] - [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00"] - [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00"]] - [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00Z"] - [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00Z"] - [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00Z"] - [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00Z"] - [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00Z"]]) - (mt/rows - (mt/dataset test-data-with-time - (mt/run-mbql-query users - {:order-by [[:asc $id]] - :limit 5})))))) - (testing "with report timezone" - (mt/with-report-timezone-id "America/Los_Angeles" - (is (= (cond - (= driver/*driver* :sqlite) + (mt/dataset test-data-with-time + (testing "without report timezone" + (is (= (if (= driver/*driver* :sqlite) + ;; TIMEZONE FIXME [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00"] [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00"] [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00"] [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00"] [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00"]] - - (qp.test/supports-report-timezone? driver/*driver*) - [[1 "Plato Yeshua" "2014-04-01T00:00:00-07:00" "08:30:00-08:00"] - [2 "Felipinho Asklepios" "2014-12-05T00:00:00-08:00" "15:15:00-08:00"] - [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00-08:00" "16:15:00-08:00"] - [4 "Simcha Yan" "2014-01-01T00:00:00-08:00" "08:30:00-08:00"] - [5 "Quentin Sören" "2014-10-03T00:00:00-07:00" "17:30:00-08:00"]] - - :else [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00Z"] [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00Z"] [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00Z"] [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00Z"] [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00Z"]]) - (mt/dataset test-data-with-time + (mt/rows + (mt/run-mbql-query users + {:order-by [[:asc $id]] + :limit 5}))))) + (testing "with report timezone" + (mt/with-report-timezone-id "America/Los_Angeles" + (is (= (cond + (= driver/*driver* :sqlite) + [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00"] + [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00"] + [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00"] + [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00"] + [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00"]] + + ;; TIMEZONE FIXME -- the value of this changes based on whether we are in DST. This is B R O K E N + (qp.test/supports-report-timezone? driver/*driver*) + (let [offset (t/zone-offset (t/zoned-date-time (t/local-date) (t/local-time) (t/zone-id "America/Los_Angeles")))] + [[1 "Plato Yeshua" "2014-04-01T00:00:00-07:00" (str "08:30:00" offset)] + [2 "Felipinho Asklepios" "2014-12-05T00:00:00-08:00" (str "15:15:00" offset)] + [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00-08:00" (str "16:15:00" offset)] + [4 "Simcha Yan" "2014-01-01T00:00:00-08:00" (str "08:30:00" offset)] + [5 "Quentin Sören" "2014-10-03T00:00:00-07:00" (str "17:30:00" offset)]]) + + :else + [[1 "Plato Yeshua" "2014-04-01T00:00:00Z" "08:30:00Z"] + [2 "Felipinho Asklepios" "2014-12-05T00:00:00Z" "15:15:00Z"] + [3 "Kaneonuskatew Eiran" "2014-11-06T00:00:00Z" "16:15:00Z"] + [4 "Simcha Yan" "2014-01-01T00:00:00Z" "08:30:00Z"] + [5 "Quentin Sören" "2014-10-03T00:00:00Z" "17:30:00Z"]]) (mt/rows (mt/run-mbql-query users {:order-by [[:asc $id]] @@ -159,10 +160,11 @@ (mt/with-clock (t/mock-clock (t/instant clock-instant) clock-zone) (is (= expected (format-rows/format-value t (t/zone-id zone))) - (format "format %s '%s' with results timezone ID '%s'" (.getName (class t)) t zone)))))) + (format "format %s '%s' with results timezone ID '%s'" (.getName (class t)) t zone))))))) - (deftest results-timezone-test - (testing "Make sure ISO-8601 timestamps are written correctly based on the report-timezone" +(deftest results-timezone-test + (testing "Make sure ISO-8601 timestamps are written correctly based on the report-timezone" + (driver/with-driver ::timezone-driver (doseq [[timezone-id expected-rows] {"UTC" [["2011-04-18T10:12:47.232Z" "2011-04-18T00:00:00Z" "2011-04-18T10:12:47.232Z"]] @@ -171,12 +173,10 @@ "2011-04-18T19:12:47.232+09:00"]]}] (mt/with-results-timezone-id timezone-id (testing (format "timezone ID '%s'" timezone-id) - (let [results (driver/with-driver ::timezone-driver - ((format-rows/format-rows - (constantly - {:rows [[(t/instant "2011-04-18T10:12:47.232Z") - (t/local-date 2011 4 18) - (t/offset-date-time "2011-04-18T10:12:47.232Z")]]})) - {}))] - (is (= {:rows expected-rows} - results))))))))) + (let [query {} + rows [[(t/instant "2011-04-18T10:12:47.232Z") + (t/local-date 2011 4 18) + (t/offset-date-time "2011-04-18T10:12:47.232Z")]] + results (mt/test-qp-middleware format-rows/format-rows query rows)] + (is (= expected-rows + (:post results)))))))))) diff --git a/test/metabase/query_processor_test/string_extracts_test.clj b/test/metabase/query_processor_test/string_extracts_test.clj index 43c421242abcc62dded3f49ea422ad3e6ee76731..9af9b56e45a4b46e1c094e055087fec954cb02b3 100644 --- a/test/metabase/query_processor_test/string_extracts_test.clj +++ b/test/metabase/query_processor_test/string_extracts_test.clj @@ -73,5 +73,5 @@ :aggregation [[:count]] :limit 1} (mt/run-mbql-query venues) - rows + (mt/formatted-rows [identity int]) first))))) diff --git a/test/metabase/query_processor_test/time_field_test.clj b/test/metabase/query_processor_test/time_field_test.clj index 9c8805973c24d846e40e6b99c2d792012457b2f4..2885a05b12b765eadfd9d26615fb0ed0c18da25a 100644 --- a/test/metabase/query_processor_test/time_field_test.clj +++ b/test/metabase/query_processor_test/time_field_test.clj @@ -1,17 +1,16 @@ (ns metabase.query-processor-test.time-field-test (:require [clojure.test :refer :all] + [java-time :as t] [metabase [driver :as driver] - [query-processor-test :as qpt]] - [metabase.test - [data :as data] - [util :as tu]] - [metabase.test.data.datasets :as datasets])) + [query-processor-test :as qp.test] + [test :as mt]] + [metabase.test.util :as tu])) (defn- time-query [filter-type & filter-args] - (qpt/formatted-rows [int identity identity] - (data/dataset test-data-with-time - (data/run-mbql-query users + (mt/formatted-rows [int identity identity] + (mt/dataset test-data-with-time + (mt/run-mbql-query users {:fields [$id $name $last_login_time] :order-by [[:asc $id]] :filter (into [filter-type $last_login_time] filter-args)})))) @@ -21,7 +20,7 @@ #{:oracle :mongo :redshift :sparksql}) (deftest basic-test - (datasets/test-drivers (qpt/normal-drivers-except skip-time-test-drivers) + (mt/test-drivers (mt/normal-drivers-except skip-time-test-drivers) (doseq [[message [start end]] {"Basic between query on a time field" ["08:00:00" "09:00:00"] @@ -37,7 +36,7 @@ (time-query :between start end))))))) (deftest greater-than-test - (datasets/test-drivers (qpt/normal-drivers-except skip-time-test-drivers) + (mt/test-drivers (mt/normal-drivers-except skip-time-test-drivers) (is (= (if (= :sqlite driver/*driver*) [[3 "Kaneonuskatew Eiran" "16:15:00"] [5 "Quentin Sören" "17:30:00"] @@ -49,36 +48,37 @@ (time-query :> "16:00:00Z"))))) (deftest equals-test - (datasets/test-drivers (qpt/normal-drivers-except skip-time-test-drivers) + (mt/test-drivers (mt/normal-drivers-except skip-time-test-drivers) (is (= (if (= :sqlite driver/*driver*) [[3 "Kaneonuskatew Eiran" "16:15:00"]] [[3 "Kaneonuskatew Eiran" "16:15:00Z"]]) (time-query := "16:15:00Z"))))) (deftest report-timezone-test - (datasets/test-drivers (qpt/normal-drivers-except skip-time-test-drivers) - (is (= (cond - (= :sqlite driver/*driver*) - [[1 "Plato Yeshua" "08:30:00"] - [4 "Simcha Yan" "08:30:00"]] + (mt/test-drivers (mt/normal-drivers-except skip-time-test-drivers) + (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"] + ;; TIMEZONE FIXME — the value of some of these change based on DST. This is B R O K E N + (let [offset (t/zone-offset (t/zoned-date-time (t/local-date) (t/local-time) (t/zone-id "America/Los_Angeles")))] + (is (= (cond + (= :sqlite driver/*driver*) + [[1 "Plato Yeshua" "08:30:00"] + [4 "Simcha Yan" "08:30:00"]] - ;; TIMEZONE FIXME — Wack answer - (= :presto driver/*driver*) - [[3 "Kaneonuskatew Eiran" "08:15:00-08:00"]] + ;; TIMEZONE FIXME — Wack answer + (= :presto driver/*driver*) + [[3 "Kaneonuskatew Eiran" (str "08:15:00" offset)]] - ;; Databases like PostgreSQL ignore timezone information when - ;; using a time field, the result below is what happens when the - ;; 08:00 time is interpreted as UTC, then not adjusted to Pacific - ;; time by the DB - (qpt/supports-report-timezone? driver/*driver*) - ;; TIMEZONE FIXME — the value of this changes based on whether we are in DST. This is B R O K E N - [[1 "Plato Yeshua" "08:30:00-08:00"] - [4 "Simcha Yan" "08:30:00-08:00"]] + ;; Databases like PostgreSQL ignore timezone information when + ;; using a time field, the result below is what happens when the + ;; 08:00 time is interpreted as UTC, then not adjusted to Pacific + ;; time by the DB + (qp.test/supports-report-timezone? driver/*driver*) + [[1 "Plato Yeshua" (str "08:30:00" offset)] + [4 "Simcha Yan" (str "08:30:00" offset)]] - :else - [[1 "Plato Yeshua" "08:30:00Z"] - [4 "Simcha Yan" "08:30:00Z"]]) - (tu/with-temporary-setting-values [report-timezone "America/Los_Angeles"] - (apply time-query :between (if (qpt/supports-report-timezone? driver/*driver*) - ["08:00:00" "09:00:00"] - ["08:00:00-00:00" "09:00:00-00:00"]))))))) + :else + [[1 "Plato Yeshua" "08:30:00Z"] + [4 "Simcha Yan" "08:30:00Z"]]) + (apply time-query :between (if (qp.test/supports-report-timezone? driver/*driver*) + ["08:00:00" "09:00:00"] + ["08:00:00-00:00" "09:00:00-00:00"])))))))) diff --git a/test/metabase/test.clj b/test/metabase/test.clj index a552dfaf0ffeea8fda42f8e3fab743f8c1b8c568..74057813ce86f1d580aeb3607b5da4fb11901ef3 100644 --- a/test/metabase/test.clj +++ b/test/metabase/test.clj @@ -226,9 +226,10 @@ context)] (if async? (async-qp query context) - (let [qp (qp.reducible/sync-qp async-qp) - result (qp query context)] - {:result (m/dissoc-in result [:data :pre]) - :pre (-> result :data :pre) - :post (-> result :data :rows) - :metadata (update result :data #(dissoc % :pre :rows))}))))) + (binding [qp.reducible/*run-on-separate-thread?* true] + (let [qp (qp.reducible/sync-qp async-qp) + result (qp query context)] + {:result (m/dissoc-in result [:data :pre]) + :pre (-> result :data :pre) + :post (-> result :data :rows) + :metadata (update result :data #(dissoc % :pre :rows))}))))))