From 76a77f9326eebeef839321785e8e45d52f3de43c Mon Sep 17 00:00:00 2001 From: Oleksandr Yakushev Date: Fri, 4 Oct 2024 18:35:14 +0300 Subject: [PATCH] [jdbc.read] Don't track realized keys in TransientRow --- src/toucan2/jdbc/result_set.clj | 44 ++++++++++++++++++--------------- src/toucan2/jdbc/row.clj | 25 +++++++++---------- test/toucan2/jdbc/row_test.clj | 15 ----------- 3 files changed, 36 insertions(+), 48 deletions(-) diff --git a/src/toucan2/jdbc/result_set.clj b/src/toucan2/jdbc/result_set.clj index a5384cd..bc5bc7d 100644 --- a/src/toucan2/jdbc/result_set.clj +++ b/src/toucan2/jdbc/result_set.clj @@ -67,31 +67,34 @@ (rs! [_this acc] (persistent! acc))) +(defn- column-name->keyword [column-name label-fn] + (when (or (string? column-name) + (instance? clojure.lang.Named column-name)) + (keyword + (when (instance? clojure.lang.Named column-name) + (when-let [col-ns (namespace column-name)] + (name (label-fn (name col-ns))))) + (name (label-fn (name column-name)))))) + (defn- make-column-name->index [cols label-fn] {:pre [(fn? label-fn)]} (if (empty? cols) (constantly nil) (memoize (fn [column-name] - (when (or (string? column-name) - (instance? clojure.lang.Named column-name)) - ;; TODO FIXME -- it seems like the column name we get here has already went thru the label fn/qualifying - ;; functions. The `(originally ...)` in the log message is wrong. Are we applying label function twice?! - (let [column-name' (keyword - (when (instance? clojure.lang.Named column-name) - (when-let [col-ns (namespace column-name)] - (label-fn (name col-ns)))) - (label-fn (name column-name))) - i (when column-name' - (first (keep-indexed - (fn [i col] - (when (= col column-name') - (inc i))) - cols)))] - (log/tracef "Index of column named %s (originally %s) is %s" column-name' column-name i) - (when-not i - (log/debugf "Could not determine index of column name %s. Found: %s" column-name cols)) - i)))))) + ;; TODO FIXME -- it seems like the column name we get here has already went thru the label fn/qualifying + ;; functions. The `(originally ...)` in the log message is wrong. Are we applying label function twice?! + (let [column-name' (column-name->keyword column-name label-fn) + i (when column-name' + (first (keep-indexed + (fn [i col] + (when (= col column-name') + (inc i))) + cols)))] + (log/tracef "Index of column named %s (originally %s) is %s" column-name' column-name i) + (when-not i + (log/debugf "Could not determine index of column name %s. Found: %s" column-name cols)) + i))))) (defn instance-builder-fn "Create a result set map builder function appropriate for passing as the `:builder-fn` option to `next.jdbc` that @@ -142,6 +145,7 @@ col-names (get builder :cols (next.jdbc.rs/get-modified-column-names (.getMetaData rset) combined-opts)) + col-names-kw (vec (keep #(column-name->keyword % label-fn) col-names)) col-name->index (make-column-name->index col-names label-fn)] (log/tracef "column name -> index = %s" col-name->index) (loop [acc init] @@ -154,7 +158,7 @@ :let [row-num (.getRow rset) _ (log/tracef "Fetch row %s" row-num) i->thunk (row-num->i->thunk row-num) - row (jdbc.row/row model rset builder i->thunk col-name->index) + row (jdbc.row/row model rset builder i->thunk col-name->index col-names-kw) acc' (rf acc row)] (reduced? acc') diff --git a/src/toucan2/jdbc/row.clj b/src/toucan2/jdbc/row.clj index 034d860..1d9133f 100644 --- a/src/toucan2/jdbc/row.clj +++ b/src/toucan2/jdbc/row.clj @@ -52,8 +52,8 @@ ;; a function that given a column name key will normalize it and return the ;; corresponding JDBC index. This should probably be memoized for the whole result set. column-name->index - ;; an atom with a set of realized column name keywords. - realized-keys + ;; a list of all column names + column-names ;; ATOM with map. Given a JDBC column index (starting at 1) return a thunk that can be ;; used to fetch the column. This usually comes ;; from [[toucan2.jdbc.read/make-cached-i->thunk]]. @@ -74,7 +74,6 @@ (if (realized? realized-row) (assoc @realized-row k v) (let [^clojure.lang.ITransientMap transient-row' (assoc! transient-row k v)] - (swap! realized-keys conj k) (assert (= (.valAt transient-row' k) v) (format "assoc! did not do what we expected. k = %s v = %s row = %s .valAt = %s" (pr-str k) @@ -90,7 +89,7 @@ rset builder column-name->index - realized-keys + column-names i->thunk transient-row' realized-row))))) @@ -118,7 +117,6 @@ (if (= index k-index) (constantly ::not-found) (i->thunk index)))))) - (swap! realized-keys disj k) ;; as in the `assoc` method above, we can optimize a bit and return `this` instead of creating a new object if ;; `assoc!` returned the original `transient-row` rather than a different object (if (and (identical? transient-row transient-row') @@ -128,7 +126,7 @@ rset builder column-name->index' - realized-keys + column-names i->thunk transient-row' realized-row))))) @@ -296,11 +294,13 @@ [^toucan2.jdbc.row.TransientRow row] (try (let [transient-row (.transient_row row) - realized-keys (.realized_keys row)] + column-names (.column_names row)] [(symbol (format "^%s " `TransientRow)) ;; (instance? pretty.core.PrettyPrintable transient-row) (pretty/pretty transient-row) - (zipmap @realized-keys - (map #(get transient-row %) @realized-keys))]) + (into {} (keep (fn [col] (let [v (get transient-row col ::not-found)] + (when-not (= v ::not-found) + [col (get transient-row col)])))) + column-names)]) (catch Exception _ ["unrealized result set {row} -- do you need to call toucan2.realize/realize ?"]))) @@ -357,18 +357,17 @@ (defn ^:no-doc row "Create a new `TransientRow`. Part of the low-level implementation of the JDBC query execution backend. You probably shouldn't be using this directly!" - [model ^ResultSet rset builder i->thunk col-name->index] + [model ^ResultSet rset builder i->thunk col-name->index column-names] (assert (not (.isClosed rset)) "ResultSet is already closed") (let [transient-row (next.jdbc.rs/->row builder) i->thunk (atom i->thunk) - realized-row-delay (make-realized-row-delay builder i->thunk transient-row) - realized-keys (atom #{})] + realized-row-delay (make-realized-row-delay builder i->thunk transient-row)] ;; this is a gross amount of positional args. But using `reify` makes debugging things too hard IMO. (->TransientRow model rset builder col-name->index - realized-keys + column-names i->thunk transient-row realized-row-delay))) diff --git a/test/toucan2/jdbc/row_test.clj b/test/toucan2/jdbc/row_test.clj index 21a3ab0..8a300bd 100644 --- a/test/toucan2/jdbc/row_test.clj +++ b/test/toucan2/jdbc/row_test.clj @@ -32,9 +32,6 @@ conj (select/reducible-select ::test/venues 1))))) -(defn- realized-keys [^TransientRow row] - @(.realized_keys row)) - (defn- already-realized? [^TransientRow row] (realized? (.realized_row row))) @@ -49,16 +46,12 @@ (get row :xyz))) (is (= ::not-found (get row :xyz ::not-found))) - (is (= #{:name} - (realized-keys row))) (is (not (already-realized? row)))))) (deftest ^:parallel assoc-test (do-with-row (fn [row] (is (transient-row? (assoc row :k 1000))) - (is (= #{:k} - (realized-keys row))) (is (contains? row :k)) (is (not (already-realized? row))) (is (= 1000 @@ -66,8 +59,6 @@ (:k row))) (testing "namespaced key" (is (transient-row? (assoc row :namespaced/k 500))) - (is (= #{:k :namespaced/k} - (realized-keys row))) (is (contains? row :k)) (is (contains? row :namespaced/k)) (is (not (already-realized? row))) @@ -93,8 +84,6 @@ (do-with-row (fn [row] (is (transient-row? (assoc row :k nil))) - (is (= #{:k} - (realized-keys row))) (is (not (already-realized? row))) (is (= nil (get row :k) @@ -107,8 +96,6 @@ (fn [row] (let [row' (merge row {:k 1000})] (doseq [row [row row']] - (is (= #{:k} - (realized-keys row))) (is (not (already-realized? row)))))))) (deftest ^:parallel contains?-test @@ -116,8 +103,6 @@ (fn [row] (is (contains? row :name)) (is (not (contains? row :xyz))) - (is (= #{} - (realized-keys row))) (is (not (already-realized? row)))))) (deftest ^:parallel deferrable-update-test