Optimize relative datetime filters (metabase#14835)

1. Rename optimize-datetime-filters middleware -> optimize-temporal-filters (it's more accurate, because this also
optimizes date or time filter clauses)

2. optimize-temporal-filters middleware now optimizes relative-datetime clauses (which represent a moment in time
relative to when the query is ran, e.g. "last month") in addition to absolute-datetime clauses (which represent an
absolute moment in time, e.g. 2021-02-15T14:40:00-08:00) . This middleware rewrites queries so we filter against
specific temporal ranges without casting the column itself, meaning we can leverage indexes on that column. See metabase#11837
for more details

3. Added new validate-temporal-bucketing middleware that throws an Exception if you try to do something that makes no
sense, e.g. bucket a DATE field by :time or a TIME field by :month. This is a better situation then running the query
and waiting for the DB to complain. (In practice, I don't think the FE client would let you generate a query like this
in the first place)

4. Fix random test failures for MySQL in task-history-cleanup-test

Author: camsaul

.circleci/config.yml

@@ -492,7 +492,7 @@ jobs:
       # .BACKEND-CHECKSUMS is every Clojure source file as well as dependency files like deps.edn and plugin manifests
       - create-checksum-file:
           filename: .BACKEND-CHECKSUMS
-          find-args: ". -type f -name '*.clj' -or -name '*.java' -or -name deps.edn -or -name metabase-plugin.yaml"
+          find-args: ". -type f -name '*.clj' -or -name '*.java' -or -name '*.edn' -or -name '*.yaml' -or -name sample-dataset.db.mv.db"
       # .FRONTEND-CHECKSUMS is every JavaScript source file as well as dependency files like yarn.lock
       - create-checksum-file:
           filename: .FRONTEND-CHECKSUMS

.dir-locals.el

@@ -17,9 +17,8 @@
                             ;; Define custom indentation for functions inside metabase.
                             ;; This list isn't complete; add more forms as we come across them.
                             (define-clojure-indent
-                              (let-404 1)
+                              (let-404)
                               (match 1)
-                              (merge-with 1)
                               (l/matche '(1 (:defn)))
                               (l/matcha '(1 (:defn)))
                               (p/defprotocol+ '(1 (:defn)))

backend/mbql/src/metabase/mbql/schema.clj

@@ -558,7 +558,7 @@
    relative-datetime
    Field))
 
-(def ^:private EqualityComparible
+(def ^:private EqualityComparable
   "Schema for things things that make sense in a `=` or `!=` filter, i.e. things that can be compared for equality."
   (s/maybe
    (s/cond-pre
@@ -570,7 +570,7 @@
     ExpressionArg
     value)))
 
-(def ^:private OrderComparible
+(def ^:private OrderComparable
   "Schema for things that make sense in a filter like `>` or `<`, i.e. things that can be sorted."
   (s/if (partial is-clause? :value)
     value
@@ -594,24 +594,25 @@
 ;;
 ;;    [:!= [:field 1 nil] 2 3] --[DESUGAR]--> [:and [:!= [:field 1 nil] 2] [:!= [:field 1 nil] 3]]
 
-(defclause =,  field EqualityComparible, value-or-field EqualityComparible, more-values-or-fields (rest EqualityComparible))
-(defclause !=, field EqualityComparible, value-or-field EqualityComparible, more-values-or-fields (rest EqualityComparible))
+(defclause =,  field EqualityComparable, value-or-field EqualityComparable, more-values-or-fields (rest EqualityComparable))
+(defclause !=, field EqualityComparable, value-or-field EqualityComparable, more-values-or-fields (rest EqualityComparable))
 
-(defclause <,  field OrderComparible, value-or-field OrderComparible)
-(defclause >,  field OrderComparible, value-or-field OrderComparible)
-(defclause <=, field OrderComparible, value-or-field OrderComparible)
-(defclause >=, field OrderComparible, value-or-field OrderComparible)
+(defclause <,  field OrderComparable, value-or-field OrderComparable)
+(defclause >,  field OrderComparable, value-or-field OrderComparable)
+(defclause <=, field OrderComparable, value-or-field OrderComparable)
+(defclause >=, field OrderComparable, value-or-field OrderComparable)
 
-(defclause between field OrderComparible, min OrderComparible, max OrderComparible)
+;; :between is INCLUSIVE just like SQL !!!
+(defclause between field OrderComparable, min OrderComparable, max OrderComparable)
 
 ;; SUGAR CLAUSE: This is automatically written as a pair of `:between` clauses by the `:desugar` middleware.
 (defclause ^:sugar inside
-  lat-field OrderComparible
-  lon-field OrderComparible
-  lat-max   OrderComparible
-  lon-min   OrderComparible
-  lat-min   OrderComparible
-  lon-max   OrderComparible)
+  lat-field OrderComparable
+  lon-field OrderComparable
+  lat-max   OrderComparable
+  lon-min   OrderComparable
+  lat-min   OrderComparable
+  lon-max   OrderComparable)
 
 ;; SUGAR CLAUSES: These are rewritten as `[:= <field> nil]` and `[:not= <field> nil]` respectively
 (defclause ^:sugar is-null,  field Field)

backend/mbql/src/metabase/mbql/util.clj

@@ -2,10 +2,9 @@
   "Utilitiy functions for working with MBQL queries."
   (:refer-clojure :exclude [replace])
   (:require [clojure.string :as str]
-            [java-time.amount :as t.amount]
-            [java-time.core :as t.core]
             [metabase.mbql.schema :as mbql.s]
             [metabase.mbql.util.match :as mbql.match]
+            [metabase.util.date-2 :as u.date]
             [metabase.util.i18n :refer [tru]]
             [metabase.util.schema :as su]
             [schema.core :as s]))
@@ -460,43 +459,25 @@
       ;; otherwise add new clause at the end
       (update inner-query :order-by (comp vec distinct conj) order-by-clause))))
 
-(defn relative-date
-  "Return a new Temporal value relative to `t` using a relative date `unit`.
-
-    (relative-date :year -1 (t/zoned-date-time \"2019-11-04T10:57:00-08:00[America/Los_Angeles]\"))
-    ;; ->
-    (t/zoned-date-time \"2020-11-04T10:57-08:00[America/Los_Angeles]\")"
-  ^java.time.temporal.Temporal [unit amount t]
-  (if (zero? amount)
-    t
-    (t.core/plus t (case unit
-                     :millisecond (t.amount/millis amount)
-                     :second      (t.amount/seconds amount)
-                     :minute      (t.amount/minutes amount)
-                     :hour        (t.amount/hours amount)
-                     :day         (t.amount/days amount)
-                     :week        (t.amount/days (* amount 7))
-                     :month       (t.amount/months amount)
-                     :quarter     (t.amount/months (* amount 3))
-                     :year        (t.amount/years amount)))))
-
 (s/defn add-datetime-units :- mbql.s/DateTimeValue
   "Return a `relative-datetime` clause with `n` units added to it."
   [absolute-or-relative-datetime :- mbql.s/DateTimeValue
    n                             :- s/Num]
   (if (is-clause? :relative-datetime absolute-or-relative-datetime)
     (let [[_ original-n unit] absolute-or-relative-datetime]
       [:relative-datetime (+ n original-n) unit])
-    (let [[_ timestamp unit] absolute-or-relative-datetime]
-      [:absolute-datetime (relative-date unit n timestamp) unit])))
+    (let [[_ t unit] absolute-or-relative-datetime]
+      [:absolute-datetime (u.date/add t unit n) unit])))
 
 (defn dispatch-by-clause-name-or-class
   "Dispatch function perfect for use with multimethods that dispatch off elements of an MBQL query. If `x` is an MBQL
   clause, dispatches off the clause name; otherwise dispatches off `x`'s class."
-  [x]
-  (if (mbql-clause? x)
-    (first x)
-    (class x)))
+  ([x]
+   (if (mbql-clause? x)
+     (first x)
+     (class x)))
+  ([x _]
+   (dispatch-by-clause-name-or-class x)))
 
 (s/defn expression-with-name :- mbql.s/FieldOrExpressionDef
   "Return the `Expression` referenced by a given `expression-name`."
@@ -728,9 +709,24 @@
      joins
      unique-aliases)))
 
+(defn- remove-empty [x]
+  (cond
+    (map? x)
+    (not-empty (into {} (for [[k v] x
+                              :let  [v (remove-empty v)]
+                              :when (some? v)]
+                          [k v])))
+
+    (sequential? x)
+    (not-empty (into (empty x) (filter some? (map remove-empty x))))
+
+    :else
+    x))
+
 (s/defn update-field-options :- mbql.s/field
   "Like `clojure.core/update`, but for the options in a `:field` clause."
   [[_ id-or-name opts] :- mbql.s/field f & args]
+  ;; TODO -- this should canonicalize the clause afterwards
   [:field id-or-name (not-empty (apply f opts args))])
 
 (defn assoc-field-options

backend/mbql/test/metabase/mbql/util_test.clj

@@ -1,27 +1,11 @@
 (ns metabase.mbql.util-test
   (:require [clojure.test :refer :all]
-            [java-time :as t]
             [metabase.mbql.util :as mbql.u]
             metabase.types))
 
 ;; fool cljr-refactor/the linter so it doesn't try to remove the unused dep on `metabase.types`
 (comment metabase.types/keep-me)
 
-(deftest relative-date-test
-  (let [t (t/zoned-date-time "2019-06-14T00:00:00.000Z[UTC]")]
-    (doseq [[unit n expected] [[:second  5 "2019-06-14T00:00:05Z[UTC]"]
-                               [:minute  5 "2019-06-14T00:05:00Z[UTC]"]
-                               [:hour    5 "2019-06-14T05:00:00Z[UTC]"]
-                               [:day     5 "2019-06-19T00:00:00Z[UTC]"]
-                               [:week    5 "2019-07-19T00:00:00Z[UTC]"]
-                               [:month   5 "2019-11-14T00:00:00Z[UTC]"]
-                               [:quarter 5 "2020-09-14T00:00:00Z[UTC]"]
-                               [:year    5 "2024-06-14T00:00:00Z[UTC]"]]]
-      (is (= (t/zoned-date-time expected)
-             (mbql.u/relative-date unit n t))
-          (format "%s plus %d %ss should be %s" t n unit expected)))))
-
-
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                     match                                                      |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -991,4 +975,8 @@
 
   (testing "Should remove empty options"
     (is (= [:field 1 nil]
-           (mbql.u/update-field-options [:field 1 {:a 1}] dissoc :a)))))
+           (mbql.u/update-field-options [:field 1 {:a 1}] dissoc :a))))
+
+  (testing "Should normalize the clause"
+    [:field 1 nil]
+    (mbql.u/update-field-options [:field 1 {:a {:b 1}}] assoc-in [:a :b] nil)))

bin/lint-migrations-file/src/change/strict.clj

@@ -31,3 +31,18 @@
 
 (s/def ::change
   (s/keys :opt-un [::addColumn ::createTable]))
+
+(s/def :change.strict.dbms-qualified-sql-change.sql/dbms
+  string?)
+
+(s/def :change.strict.dbms-qualified-sql-change.sql/sql
+  string?)
+
+(s/def :change.strict.dbms-qualified-sql-change/sql
+  (s/keys :req-un [:change.strict.dbms-qualified-sql-change.sql/dbms
+                   :change.strict.dbms-qualified-sql-change.sql/sql]))
+
+(s/def ::dbms-qualified-sql-change
+  (s/merge
+   ::change
+   (s/keys :req-un [:change.strict.dbms-qualified-sql-change/sql])))

bin/lint-migrations-file/src/change_set/strict.clj

@@ -1,7 +1,8 @@
 (ns change-set.strict
   (:require change-set.common
             change.strict
-            [clojure.spec.alpha :as s]))
+            [clojure.spec.alpha :as s]
+            [clojure.string :as str]))
 
 (comment change-set.common/keep-me
          change.strict/keep-me)
@@ -12,9 +13,20 @@
    string?
    (partial re-find #"Added [\d.x]+")))
 
-;; only one change allowed per change set for the strict schema.
 (s/def ::changes
-  (s/alt :change :change.strict/change))
+  (s/or
+   ;; only one change allowed per change set for the strict schema.
+   :one-change
+   (s/alt :change :change.strict/change)
+
+   ;; unless it's SQL changes, in which case we'll let you specify more than one as long as they are qualified with
+   ;; different DBMSes
+   :sql-changes-for-different-
+   (s/and
+    (s/+ :change.strict/dbms-qualified-sql-change)
+    (fn [changes]
+      (apply distinct? (mapcat #(str/split (-> % :sql :dbms) #",")
+                               changes))))))
 
 (s/def ::change-set
   (s/merge

bin/lint-migrations-file/test/lint_migrations_file_test.clj

@@ -119,3 +119,36 @@
           (mock-change-set
            :id 200
            :changes [(update (mock-create-table-changes) :createTable dissoc :remarks)]))))))
+
+(deftest allow-multiple-sql-changes-if-dbmses-are-different
+  (testing "Allow multiple SQL changes if DBMSes are different"
+    (is (= :ok
+           (validate
+            (mock-change-set
+             :id 200
+             :changes
+             [{:sql {:dbms "h2", :sql "1"}}
+              {:sql {:dbms "postgresql", :sql "2"}}
+              {:sql {:dbms "mysql,mariadb", :sql "3"}}])))))
+
+  (testing "should fail if *any* change is missing dbms"
+    (is (thrown-with-msg?
+         clojure.lang.ExceptionInfo
+         #":dbms"
+         (validate
+          (mock-change-set
+           :id 200
+           :changes
+           [{:sql {:dbms "h2", :sql "1"}}
+            {:sql {:sql "2"}}])))))
+
+  (testing "should fail if a DBMS is repeated"
+    (is (thrown-with-msg?
+         clojure.lang.ExceptionInfo
+         #":changes"
+         (validate
+          (mock-change-set
+           :id 200
+           :changes
+           [{:sql {:dbms "h2", :sql "1"}}
+            {:sql {:dbms "postgresql,h2", :sql "2"}}]))))))

jsconfig.json

@@ -1 +1,19 @@
-{"include": ["frontend/src/**", "enterprise/frontent/src/**"]}
+{
+  "include": [
+    "frontend/src/**/*",
+    "enterprise/frontend/src/**/*",
+    "frontend/test/**/*",
+    "enterprise/frontend/test/**/*"
+  ],
+  "compilerOptions": {
+    "baseUrl": ".",
+    "paths": {
+      "*": [
+        "./frontend/src/*",
+        "./frontend/test/*",
+        "./enterprise/frontend/src/*",
+        "./enterprise/frontend/test/*"
+      ]
+    }
+  }
+}

modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj

@@ -604,6 +604,9 @@
                                                                         [:field (:id f) {:temporal-unit unit}]
                                                                         [:relative-datetime -1 unit]])}))))))))))
 
+;; This is a table of different BigQuery column types -> temporal units we should be able to bucket them by for
+;; filtering purposes against RELATIVE-DATETIMES. `relative-datetime` only supports the unit below -- a subset of all
+;; the MBQL date bucketing units.
 (def ^:private filter-test-table
   [[nil          :minute :hour :day  :week :month :quarter :year]
    [:time        true    true  false false false  false    false]
@@ -613,6 +616,8 @@
 
 (defn- test-table-with-fn [table f]
   (let [units (rest (first table))]
+    ;; this is done in parallel because there are a lot of combinations and doing them one at a time would take the
+    ;; rest of our lives
     (dorun (pmap (fn [[field & vs]]
                    (testing (format "\nfield = %s" field)
                      (dorun (pmap (fn [[unit expected]]
@@ -631,6 +636,8 @@
       (mt/dataset attempted-murders
         (test-table-with-fn filter-test-table can-we-filter-against-relative-datetime?)))))
 
+;; This is a table of different BigQuery column types -> temporal units we should be able to bucket them by for
+;; breakout purposes.
 (def ^:private breakout-test-table
   [[nil          :default :minute :hour :day  :week :month :quarter :year :minute-of-hour :hour-of-day :day-of-week :day-of-month :day-of-year :week-of-year :month-of-year :quarter-of-year]
    [:time        true     true    true  false false false  false    false true            true         false        false         false        false         false          false]