MBQL: handle nils in SQL filters more intuitively (#13477)

* MBQL: handle nils in SQL filters more intuitively. #13332 [ci drivers]

* Refactor [ci drivers]

* Correctly handle value literals [ci drivers]

* Make `correct-null-behaviour` work with `Field` instances [ci drivers]

* Alphabet is hard

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

@@ -284,11 +284,17 @@
       ;; `hx/identifier`s to SQL
       (binding [sql.qp/*table-alias* "ABC"
                 *print-meta*         true]
-        (let [fields {:date      date-field
-                      :datetime  datetime-field
-                      :timestamp timestamp-field}]
+        (let [fields                     {:date      date-field
+                                          :datetime  datetime-field
+                                          :timestamp timestamp-field}
+              build-honeysql-clause-head (fn [{:keys [honeysql]} field-arg args]
+                                           (if (fn? honeysql)
+                                             (honeysql field-arg args)
+                                             (into [honeysql field-arg] args)))]
           (doseq [clause [{:args 2, :mbql :=, :honeysql :=}
-                          {:args 2, :mbql :!=, :honeysql :not=}
+                          {:args 2, :mbql :!=, :honeysql (fn [identifier args]
+                                                           [:or (into [:not= identifier] args)
+                                                            [:= identifier nil]])}
                           {:args 2, :mbql :>, :honeysql :>}
                           {:args 2, :mbql :>=, :honeysql :>=}
                           {:args 2, :mbql :<, :honeysql :<}
@@ -313,8 +319,9 @@
                                                       (repeat (dec (:args clause)) filter-value))
                             expected-identifier (hx/identifier :field "ABC" (name temporal-type))
                             expected-value      (get-in value [:as temporal-type] (:value value))
-                            expected-clause     (into [(:honeysql clause) expected-identifier]
-                                                      (repeat (dec (:args clause)) expected-value))]
+                            expected-clause     (build-honeysql-clause-head clause
+                                                                            expected-identifier
+                                                                            (repeat (dec (:args clause)) expected-value))]
                         (testing (format "\nreconcile %s -> %s"
                                          (into [(:mbql clause) temporal-type] (repeat (dec (:args clause)) (:type value)))
                                          (into [(:mbql clause) temporal-type] (repeat (dec (:args clause)) temporal-type)))

src/metabase/driver.clj

@@ -615,6 +615,6 @@
 
 (defmulti db-start-of-week
   "Return start of week for given database"
-  {:added "0.37.0" :arlists '([driver database])}
+  {:added "0.37.0" :arglists '([driver])}
   dispatch-on-initialized-driver
   :hierarchy #'hierarchy)

src/metabase/driver/sql/query_processor.clj

@@ -19,7 +19,9 @@
             [metabase.query-processor
              [interface :as i]
              [store :as qp.store]]
-            [metabase.query-processor.middleware.annotate :as annotate]
+            [metabase.query-processor.middleware
+             [annotate :as annotate]
+             [wrap-value-literals :as value-literal]]
             [metabase.util
              [honeysql-extensions :as hx]
              [i18n :refer [deferred-tru]]
@@ -27,7 +29,8 @@
             [potemkin.types :as p.types]
             [pretty.core :refer [PrettyPrintable]]
             [schema.core :as s])
-  (:import metabase.util.honeysql_extensions.Identifier))
+  (:import metabase.models.field.FieldInstance
+           metabase.util.honeysql_extensions.Identifier))
 
 ;; TODO - yet another `*query*` dynamic var. We should really consolidate them all so we only need a single one.
 (def ^:dynamic *query*
@@ -634,9 +637,21 @@
   [driver [_ field value]]
   [:= (->honeysql driver field) (->honeysql driver value)])
 
+(defn- correct-null-behaviour
+  [driver [op & args]]
+  (let [field-arg (mbql.u/match-one args
+                    FieldInstance               &match
+                    #{:field-id :field-literal} &match)]
+    ;; We must not transform the head again else we'll have an infinite loop
+    ;; (and we can't do it at the call-site as then it will be harder to fish out field references)
+    [:or (into [op] (map (partial ->honeysql driver)) args)
+     [:= (->honeysql driver field-arg) nil]]))
+
 (defmethod ->honeysql [:sql :!=]
   [driver [_ field value]]
-  [:not= (->honeysql driver field) (->honeysql driver value)])
+  (if (nil? (value-literal/unwrap-value-literal value))
+    [:not= (->honeysql driver field) (->honeysql driver value)]
+    (correct-null-behaviour driver [:not= field value])))
 
 (defmethod ->honeysql [:sql :and]
   [driver [_ & subclauses]]
@@ -646,9 +661,14 @@
   [driver [_ & subclauses]]
   (apply vector :or (map (partial ->honeysql driver) subclauses)))
 
+(def ^:private clause-needs-null-behaviour-correction?
+  (comp #{:contains :starts-with :ends-with} first))
+
 (defmethod ->honeysql [:sql :not]
   [driver [_ subclause]]
-  [:not (->honeysql driver subclause)])
+  (if (clause-needs-null-behaviour-correction? subclause)
+    (correct-null-behaviour driver [:not subclause])
+    [:not (->honeysql driver subclause)]))
 
 (defmethod apply-top-level-clause [:sql :filter]
   [driver _ honeysql-form {clause :filter}]

src/metabase/query_processor/middleware/wrap_value_literals.clj

@@ -119,6 +119,13 @@
     (let [s (add-type-info s (type-info field), :parse-datetime-strings? false)]
       (into [clause field s] more))))
 
+(defn unwrap-value-literal
+  "Extract value literal from `:value` form or returns form as is if not a `:value` form."
+  [maybe-value-form]
+  (mbql.u/match-one maybe-value-form
+    [:value x & _] x
+    _              &match))
+
 (defn ^:private wrap-value-literals-in-mbql-query
   [{:keys [source-query], :as inner-query} options]
   (let [inner-query (cond-> inner-query

test/metabase/driver/sql/query_processor_test.clj

@@ -3,6 +3,7 @@
             [honeysql.core :as hsql]
             [metabase
              [driver :as driver]
+             [query-processor :as qp]
              [test :as mt]]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.models.setting :as setting]
@@ -159,7 +160,7 @@
   (testing "params from source queries should get passed in to the top-level. Semicolons should be removed"
     (mt/with-everything-store
       (driver/with-driver :h2
-        (is (= {:query  "SELECT \"source\".* FROM (SELECT * FROM some_table WHERE name = ?) \"source\" WHERE \"source\".\"name\" <> ?"
+        (is (= {:query  "SELECT \"source\".* FROM (SELECT * FROM some_table WHERE name = ?) \"source\" WHERE (\"source\".\"name\" <> ? OR \"source\".\"name\" IS NULL)"
                 :params ["Cam" "Lucky Pigeon"]}
                (sql.qp/mbql->native :h2
                  (mt/mbql-query venues
@@ -209,3 +210,24 @@
                     setting/get-keyword     (constantly :monday)]
         (is (= (hsql/call :week :created_at)
                (sql.qp/adjust-start-of-week :h2 (partial hsql/call :week) :created_at)))))))
+
+(defn- query-on-dataset-with-nils
+  [query]
+  (mt/rows
+    (qp/process-query {:database (mt/id)
+                       :type     :query
+                       :query    (merge
+                                  {:source-query {:native "select 'foo' as a union select null as a union select 'bar' as a"}
+                                   :order-by     [[:asc [:field-literal "A" :type/Text]]]}
+                                  query)})))
+
+(deftest correct-for-null-behaviour
+  (testing "NULLs should be treated intuitively in filters (SQL has somewhat unintuitive semantics where NULLs get propagated out of expressions)."
+    (is (= [[nil] ["bar"]]
+           (query-on-dataset-with-nils {:filter [:not [:starts-with [:field-literal "A" :type/Text] "f"]]})))
+    (is (= [[nil] ["bar"]]
+           (query-on-dataset-with-nils {:filter [:not [:ends-with [:field-literal "A" :type/Text] "o"]]})))
+    (is (= [[nil] ["bar"]]
+           (query-on-dataset-with-nils {:filter [:not [:contains [:field-literal "A" :type/Text] "f"]]})))
+    (is (= [[nil] ["bar"]]
+           (query-on-dataset-with-nils {:filter [:!= [:field-literal "A" :type/Text] "foo"]})))))

test/metabase/query_processor_test/nested_queries_test.clj

@@ -324,7 +324,8 @@
                       [:source.LONGITUDE :LONGITUDE]
                       [:source.PRICE :PRICE]]
              :from   [[venues-source-honeysql :source]]
-             :where  [:not= :source.text "Coo"]
+             :where  [:or [:not= :source.text "Coo"]
+                      [:= :source.text nil]]
              :limit  10})
            (qp/query->native
             (mt/mbql-query nil