Skip to content
Snippets Groups Projects
Unverified Commit 4b278779 authored by Chris Truter's avatar Chris Truter Committed by GitHub
Browse files

Future-proof CSV uploads from next HoneySQL upgrade (#48220)

parent abe04bc2
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/driver/sql_jdbc.clj
+ 33
− 17
View file @ 4b278779
......@@ -127,15 +127,27 @@
[_driver _semantic_type expr]
(h2x/->timestamp expr))
(defmacro ^:private with-quoting [driver & body]
`(binding [sql/*dialect* (sql/get-dialect (sql.qp/quote-style ~driver))
sql/*quoted* true]
~@body))
(defn- quote-identifier [ref]
[:raw (sql/format-entity ref)])
(defn- create-table!-sql
[driver table-name column-definitions & {:keys [primary-key]}]
(first (sql/format {:create-table (keyword table-name)
:with-columns (cond-> (mapv (fn [[name type-spec]]
(vec (cons name type-spec)))
column-definitions)
primary-key (conj [(into [:primary-key] primary-key)]))}
:quoted true
:dialect (sql.qp/quote-style driver))))
(with-quoting driver
(first (sql/format {:create-table (keyword table-name)
:with-columns (cond-> (mapv (fn [[col-name type-spec]]
(vec (cons (quote-identifier col-name)
(if (string? type-spec)
[[:raw type-spec]]
type-spec))))
column-definitions)
primary-key (conj [(into [:primary-key] primary-key)]))}
:quoted true
:dialect (sql.qp/quote-style driver)))))
(defmethod driver/create-table! :sql-jdbc
[driver database-id table-name column-definitions & {:keys [primary-key]}]
......@@ -183,16 +195,20 @@
(defmethod driver/add-columns! :sql-jdbc
[driver db-id table-name column-definitions & {:keys [primary-key]}]
(mu/validate-throw [:maybe [:cat :keyword]] primary-key) ; we only support adding a single primary key column for now
(let [primary-key-column (first primary-key)
sql (first (sql/format {:alter-table (keyword table-name)
:add-column (map (fn [[column-name type-and-constraints]]
(cond-> (vec (cons column-name type-and-constraints))
(= primary-key-column column-name)
(conj :primary-key)))
column-definitions)}
:quoted true
:dialect (sql.qp/quote-style driver)))]
(qp.writeback/execute-write-sql! db-id sql)))
(with-quoting driver
(let [primary-key-column (first primary-key)
sql (first (sql/format {:alter-table (keyword table-name)
:add-column (map (fn [[column-name type-and-constraints]]
(cond-> (vec (cons (quote-identifier column-name)
(if (string? type-and-constraints)
[[:raw type-and-constraints]]
type-and-constraints)))
(= primary-key-column column-name)
(conj :primary-key)))
column-definitions)}
:quoted true
:dialect (sql.qp/quote-style driver)))]
(qp.writeback/execute-write-sql! db-id sql))))
(defmethod driver/alter-columns! :sql-jdbc
[driver db-id table-name column-definitions]
......
src/metabase/driver/sql_jdbc/sync/interface.clj
+ 18
− 6
View file @ 4b278779
......@@ -127,6 +127,14 @@
driver/dispatch-on-initialized-driver
:hierarchy #'driver/hierarchy)
(defmacro ^:private with-quoting [driver & body]
`(binding [sql/*dialect* (sql/get-dialect (sql.qp/quote-style ~driver))
sql/*quoted* true]
~@body))
(defn- quote-identifier [ref]
[:raw (sql/format-entity ref)])
(defmulti alter-columns-sql
"Generate the query to be used with [[driver/alter-columns!]]."
{:added "0.49.0", :arglists '([driver db-id table-name column-definitions])}
......@@ -135,9 +143,13 @@
(defmethod alter-columns-sql :sql-jdbc
[driver table-name column-definitions]
(first (sql/format {:alter-table (keyword table-name)
:alter-column (map (fn [[column-name type-and-constraints]]
(vec (cons column-name type-and-constraints)))
column-definitions)}
:quoted true
:dialect (sql.qp/quote-style driver))))
(with-quoting driver
(first (sql/format {:alter-table (keyword table-name)
:alter-column (map (fn [[column-name type-and-constraints]]
(vec (cons (quote-identifier column-name)
(if (string? type-and-constraints)
[[:raw type-and-constraints]]
type-and-constraints))))
column-definitions)}
:quoted true
:dialect (sql.qp/quote-style driver)))))
src/metabase/search/legacy.clj
+ 0
− 1
View file @ 4b278779
......@@ -314,7 +314,6 @@
(defmethod search-query-for-model "table"
[model {:keys [current-user-perms table-db-id], :as search-ctx}]
(when (seq current-user-perms)
(-> (base-query-for-model model search-ctx)
(add-table-db-id-clause table-db-id)
(sql.helpers/left-join :metabase_database [:= :table.db_id :metabase_database.id]))))
......
test/metabase/upload_test.clj
+ 17
− 15
View file @ 4b278779
......@@ -558,7 +558,7 @@
["number" {:base_type :type/Float}]
["date" {:base_type :type/Date}]
["datetime" {:base_type :type/DateTime}]]
(driver.u/supports? driver/*driver* :upload-with-auto-pk (mt/db))
(auto-pk-column?)
(cons ["_mb_row_id" {:semantic_type :type/PK
:base_type :type/BigInteger}]))
(->> (t2/select :model/Field :table_id (:id table))
......@@ -1502,7 +1502,7 @@
- extra_1")
["_mb_row_id,id, extra 2"]
(if (driver.u/supports? driver/*driver* :upload-with-auto-pk (mt/db))
(if (auto-pk-column?)
(trim-lines "The CSV file is missing columns that are in the table:
- name
......@@ -1616,7 +1616,7 @@
(deftest update-mb-row-id-csv-only-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(when (driver.u/supports? driver/*driver* :upload-with-auto-pk (mt/db))
(when (auto-pk-column?)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
(testing "If the table doesn't have _mb_row_id but the CSV does, ignore the CSV _mb_row_id but create the column anyway"
......@@ -1872,7 +1872,7 @@
(deftest update-mb-row-id-csv-and-table-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(when (driver.u/supports? driver/*driver* :upload-with-auto-pk (mt/db))
(when (auto-pk-column?)
(doseq [action (actions-to-test driver/*driver*)]
(testing (action-testing-str action)
(testing "Append succeeds if the table has _mb_row_id and the CSV does too"
......@@ -1966,7 +1966,8 @@
(with-uploads-enabled!
(testing "Append should handle new non-ascii columns being added in the latest CSV"
(with-upload-table! [table (create-upload-table!)]
(column-display-names-for-table table)
(is (= (header-with-auto-pk ["Name"])
(column-display-names-for-table table)))
;; Reorder as well for good measure
(let [csv-rows ["α,name"
"omega,Everything"]
......@@ -2106,6 +2107,15 @@
(rows-for-table table))))
(io/delete-file file))))))))))))
(defn- round-floats
"Round all floats to have n digits of precision."
[digits-precision rows]
(let [factor (Math/pow 10 digits-precision)]
(mapv (partial mapv #(if (float? %)
(/ (Math/round (* factor %)) factor)
%))
rows)))
(deftest update-type-coercion-test
(mt/test-drivers (mt/normal-drivers-with-feature :uploads)
(doseq [action (actions-to-test driver/*driver*)]
......@@ -2143,7 +2153,8 @@
(is (= {:row-count 1}
(update!))))
(is (= (rows-with-auto-pk [[coerced]])
(rows-for-table table))))
;; Deal with 32 bit floats for Clickhouse
(round-floats 6 (rows-for-table table)))))
(testing (format "\nUploading %s into a column of type %s should fail to coerce"
uncoerced (name upload-type))
(is (thrown-with-msg?
......@@ -2152,15 +2163,6 @@
(update!)))))
(io/delete-file file)))))))))))
(defn- round-floats
"Round all floats to have n digits of precision."
[digits-precision rows]
(let [factor (Math/pow 10 digits-precision)]
(mapv (partial mapv #(if (float? %)
(/ (Math/round (* factor %)) factor)
%))
rows)))
(deftest update-promotion-multiple-columns-test
(mt/test-drivers (disj (mt/normal-drivers-with-feature :uploads) :redshift) ; redshift doesn't support promotion
(doseq [action (actions-to-test driver/*driver*)]
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment