Merge release 0.36.6 into master

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

@@ -442,7 +442,7 @@
   "Unwrap a Field `clause`, if it's something that can be unwrapped (i.e. something that is, or wraps, a `:field-id` or
   `:field-literal`). Otherwise return `clause` as-is."
   [clause]
-  (if (is-clause? #{:field-id :fk-> :field-literal :datetime-field :binning-strategy} clause)
+  (if (is-clause? #{:field-id :fk-> :field-literal :datetime-field :binning-strategy :joined-field} clause)
     (unwrap-field-clause clause)
     clause))
 
@@ -453,10 +453,9 @@
     (field-clause->id-or-literal [:datetime-field [:field-id 100] ...]) ; -> 100
     (field-clause->id-or-literal [:field-id 100])                       ; -> 100
 
-  For expressions (or any other clauses) this returns the clause as-is, so as to facilitate the primary use case of
-  comparing Field clauses."
+  For expressions returns the expression name."
   [clause :- mbql.s/Field]
-  (second (unwrap-field-clause clause)))
+  (second (maybe-unwrap-field-clause clause)))
 
 (s/defn add-order-by-clause :- mbql.s/MBQLQuery
   "Add a new `:order-by` clause to an MBQL `inner-query`. If the new order-by clause references a Field that is

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

@@ -1059,3 +1059,15 @@
                                 :query    {:source-query {:expressions  {:two [:+ 1 1]}
                                                           :source-table 1}}}
                                "two"))
+
+(expect
+  1
+  (mbql.u/field-clause->id-or-literal [:field-id 1]))
+
+(expect
+  "foo"
+  (mbql.u/field-clause->id-or-literal [:field-literal "foo" :type/Integer]))
+
+(expect
+  "foo"
+  (mbql.u/field-clause->id-or-literal [:expression "foo"]))

src/metabase/cmd.clj

@@ -37,11 +37,14 @@
      ((resolve 'metabase.cmd.load-from-h2/load-from-h2!) h2-connection-string))))
 
 (defn ^:command dump-to-h2
-  "Transfer data from existing database to newly created H2 DB."
-  [h2-filename]
+  "Transfer data from existing database to newly created H2 DB with specified filename.
+
+  Target H2 file is deleted before dump, unless the --keep-existing flag is given."
+  [h2-filename & opts]
   (classloader/require 'metabase.cmd.dump-to-h2)
   (binding [mdb/*disable-data-migrations* true]
-    (let [return-code ((resolve 'metabase.cmd.dump-to-h2/dump-to-h2!) h2-filename)]
+    (let [keep-existing (boolean (some #{"--keep-existing"} opts))
+          return-code   ((resolve 'metabase.cmd.dump-to-h2/dump-to-h2!) h2-filename keep-existing)]
       (when (pos-int? return-code)
         (System/exit return-code)))))
 

src/metabase/cmd/dump_to_h2.clj

@@ -78,7 +78,7 @@
   (println-ok))
 
 (defn- load-data! [target-db-conn]
-  (println "Source db:" (mdb/jdbc-spec))
+  (println "Source db:" (dissoc (mdb/jdbc-spec) :password))
   (jdbc/with-db-connection [db-conn (mdb/jdbc-spec)]
     (doseq [{table-name :table, :as e} entities
             :let [rows (jdbc/query db-conn [(str "SELECT * FROM " (name table-name))])]
@@ -91,16 +91,19 @@
 ;;; --------------------------------------------------- Public Fns ---------------------------------------------------
 
 (defn dump-to-h2!
-  "Transfer data from existing database specified by connection string
-  to the H2 DB specified by env vars.  Intended as a tool for migrating
-  from one instance to another using H2 as serialization target.
+  "Transfer data from existing database specified by connection string to the H2 DB specified by env vars. Intended as a
+  tool for migrating from one instance to another using H2 as serialization target.
 
-  Defaults to using `@metabase.db/db-file` as the connection string."
-  [h2-filename]
+  Defaults to using `@metabase.db/db-file` as the connection string.
+
+  Target H2 DB will be deleted if it exists, unless `keep-existing` is truthy."
+  [h2-filename keep-existing]
   (let [h2-filename (or h2-filename "metabase_dump.h2")]
-    (println "Dumping to" h2-filename)
-    (doseq [filename [h2-filename (str h2-filename ".mv.db")]]
-      (when (.exists (io/file filename))
+    (println "Dumping to " h2-filename)
+    (doseq [filename [h2-filename
+                    (str h2-filename ".mv.db")]]
+      (when (and (.exists (io/file filename))
+                 (not keep-existing))
         (io/delete-file filename)
         (println (u/format-color 'red (trs "Output H2 database already exists: %s, removing.") filename))))
 

src/metabase/query_processor/middleware/add_dimension_projections.clj

@@ -87,19 +87,22 @@
   the newly remapped column. This should order by the text of the remapped column vs. the id of the source column
   before the remapping"
   [field->remapped-col :- {mbql.s/field-id, mbql.s/fk->}, order-by-clauses :- [mbql.s/OrderBy]]
-  (vec
-   (for [[direction field, :as order-by-clause] order-by-clauses]
-     (if-let [remapped-col (get field->remapped-col field)]
-       [direction remapped-col]
-       order-by-clause))))
+  (->> (for [[direction field, :as order-by-clause] order-by-clauses]
+         (if-let [remapped-col (get field->remapped-col field)]
+           [direction remapped-col]
+           order-by-clause))
+       distinct
+       vec))
 
 (defn- update-remapped-breakout
   [field->remapped-col breakout-clause]
-  (vec (mapcat (fn [field]
-                 (if-let [remapped-col (get field->remapped-col field)]
-                   [remapped-col field]
-                   [field]))
-               breakout-clause)))
+  (->> breakout-clause
+       (mapcat (fn [field]
+              (if-let [remapped-col (get field->remapped-col field)]
+                [remapped-col field]
+                [field])))
+       distinct
+       vec))
 
 (s/defn ^:private add-fk-remaps :- [(s/one (s/maybe [ExternalRemappingDimension]) "external remapping dimensions")
                                     (s/one mbql.s/Query "query")]
@@ -114,7 +117,11 @@
     ;; fetch remapping column pairs if any exist...
     (if-let [remap-col-tuples (seq (create-remap-col-tuples (concat fields breakout)))]
       ;; if they do, update `:fields`, `:order-by` and `:breakout` clauses accordingly and add to the query
-      (let [new-fields          (into fields (map second) remap-col-tuples)
+      (let [new-fields          (->> remap-col-tuples
+                                     (map second)
+                                     (concat fields)
+                                     distinct
+                                     vec)
             ;; make a map of field-id-clause -> fk-clause from the tuples
             field->remapped-col (into {} (for [[field-clause fk-clause] remap-col-tuples]
                                            [field-clause fk-clause]))

test/metabase/query_processor/middleware/add_dimension_projections_test.clj

@@ -50,9 +50,20 @@
      (testing "make sure FK remaps add an entry for the FK field to `:fields`, and returns a pair of [dimension-info updated-query]"
        (is (= [[remapped-field]
                (update-in example-query [:query :fields]
-                          conj [:fk-> [:field-id (mt/id :venues :category_id)] [:field-id (mt/id :categories :name)]])]
+                          conj [:fk-> [:field-id (mt/id :venues :category_id)]
+                                [:field-id (mt/id :categories :name)]])]
               (#'add-dim-projections/add-fk-remaps example-query))))
 
+     (testing "make sure we don't duplicate remappings"
+       (is (= [[remapped-field]
+               (update-in example-query [:query :fields]
+                          conj [:fk-> [:field-id (mt/id :venues :category_id)]
+                                [:field-id (mt/id :categories :name)]])]
+              (#'add-dim-projections/add-fk-remaps
+               (update-in example-query [:query :fields]
+                          conj [:fk-> [:field-id (mt/id :venues :category_id)]
+                                [:field-id (mt/id :categories :name)]])))))
+
      (testing "adding FK remaps should replace any existing order-bys for a field with order bys for the FK remapping Field"
        (is (= [[remapped-field]
                (-> example-query