From e260a65963baeb654e2beb408fcb5e64892ca9f3 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Mon, 7 Oct 2024 21:15:32 +0000 Subject: [PATCH] Make sure fetching column from transient row persists changes to transient --- src/toucan2/jdbc/row.clj | 113 +++++++++++++++------------------ test/toucan2/jdbc/row_test.clj | 31 +++++++++ 2 files changed, 82 insertions(+), 62 deletions(-) diff --git a/src/toucan2/jdbc/row.clj b/src/toucan2/jdbc/row.clj index 034d860..8aea53a 100644 --- a/src/toucan2/jdbc/row.clj +++ b/src/toucan2/jdbc/row.clj @@ -58,8 +58,8 @@ ;; used to fetch the column. This usually comes ;; from [[toucan2.jdbc.read/make-cached-i->thunk]]. i->thunk - ;; underlying transient map representing this row. - ^clojure.lang.ITransientMap transient-row + ;; a Volatile that contains the underlying transient map representing this row. + ^clojure.lang.Volatile volatile-transient-row ;; a delay that should return a persistent map for the current row. Once this is called ;; we should return the realized row directly and work with that going forward. realized-row] @@ -73,27 +73,18 @@ (log/tracef ".assoc %s %s" k v) (if (realized? realized-row) (assoc @realized-row k v) - (let [^clojure.lang.ITransientMap transient-row' (assoc! transient-row k v)] + (do + (vswap! volatile-transient-row (fn [^clojure.lang.ITransientMap transient-row] + (let [^clojure.lang.ITransientMap transient-row' (assoc! transient-row k v)] + (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) + (pr-str v) + (pr-str transient-row') + (pr-str (.valAt transient-row' k)))) + transient-row'))) (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) - (pr-str v) - (pr-str transient-row') - (pr-str (.valAt transient-row' k)))) - ;; `assoc!` might return a different object. In practice, I think it usually doesn't; optimize for that case - ;; and return `this` rather than creating a new instance of `TransientRow`. - (if (identical? transient-row transient-row') - this - ;; If `assoc!` did return a different object, then we need to create a new `TransientRow` with the new value - (TransientRow. model - rset - builder - column-name->index - realized-keys - i->thunk - transient-row' - realized-row))))) + this))) ;; TODO -- can we `assocEx` the transient row? (assocEx [_this k v] @@ -104,34 +95,32 @@ (log/tracef ".without %s" k) (if (realized? realized-row) (dissoc @realized-row k) - (let [transient-row' (dissoc! transient-row k) - k-index (column-name->index k) - column-name->index' (if-not k-index - column-name->index - (fn [column-name] - (let [index (column-name->index column-name)] - (when-not (= index k-index) - index))))] - (when k-index - (swap! i->thunk (fn [i->thunk] - (fn [index] - (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') - (identical? column-name->index column-name->index')) - this - (TransientRow. model - rset - builder - column-name->index' - realized-keys - i->thunk - transient-row' - realized-row))))) + (do + (vswap! volatile-transient-row dissoc! k) + (let [k-index (column-name->index k) + column-name->index' (if-not k-index + column-name->index + (fn [column-name] + (let [index (column-name->index column-name)] + (when-not (= index k-index) + index))))] + (when k-index + (swap! i->thunk (fn [i->thunk] + (fn [index] + (if (= index k-index) + (constantly ::not-found) + (i->thunk index)))))) + (swap! realized-keys disj k) + (if (identical? column-name->index column-name->index') + this + (TransientRow. model + rset + builder + column-name->index' + realized-keys + i->thunk + volatile-transient-row + realized-row)))))) ;; Java 7 compatible: no forEach / spliterator ;; @@ -146,7 +135,7 @@ (log/tracef ".containsKey %s" k) (if (realized? realized-row) (contains? @realized-row k) - (or (contains? transient-row k) + (or (contains? @volatile-transient-row k) (boolean (column-name->index k))))) (entryAt [this k] @@ -155,7 +144,7 @@ (when-not (= v ::not-found) (clojure.lang.MapEntry. k v)))) -;;; TODO -- this should probably also include any extra keys added with `assoc` or whatever + ;; TODO -- this should probably also include any extra keys added with `assoc` or whatever clojure.lang.Counted (count [_this] (log/tracef ".count") @@ -206,7 +195,7 @@ ;; non-number column name :else - (let [existing-value (.valAt transient-row k ::not-found)] + (let [existing-value (.valAt ^clojure.lang.ITransientMap @volatile-transient-row k ::not-found)] (if-not (= existing-value ::not-found) existing-value (let [fetched-value (fetch-column-with-name column-name->index @i->thunk k ::not-found)] @@ -259,7 +248,7 @@ (realized? realized-row) (update @realized-row k f) - :let [existing-value (.valAt transient-row k ::not-found)] + :let [existing-value (.valAt ^clojure.lang.ITransientMap @volatile-transient-row k ::not-found)] ;; value already exists: update the value in the transient row and call it a day (not= existing-value ::not-found) @@ -295,7 +284,7 @@ 'transient' mode." [^toucan2.jdbc.row.TransientRow row] (try - (let [transient-row (.transient_row row) + (let [transient-row @(.volatile_transient_row row) realized-keys (.realized_keys row)] [(symbol (format "^%s " `TransientRow)) ;; (instance? pretty.core.PrettyPrintable transient-row) (pretty/pretty transient-row) @@ -347,10 +336,10 @@ transient-row (range 1 (inc (next.jdbc.rs/column-count builder))))) -(defn- make-realized-row-delay [builder i->thunk transient-row] +(defn- make-realized-row-delay [builder i->thunk ^clojure.lang.Volatile volatile-transient-row] (delay (log/tracef "Fully realizing row. *fetch-all-columns* = %s" *fetch-all-columns*) - (let [row (cond->> transient-row + (let [row (cond->> @volatile-transient-row *fetch-all-columns* (fetch-all-columns! builder i->thunk))] (next.jdbc.rs/row! builder row)))) @@ -359,10 +348,10 @@ shouldn't be using this directly!" [model ^ResultSet rset builder i->thunk col-name->index] (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 #{})] + (let [volatile-transient-row (volatile! (next.jdbc.rs/->row builder)) + i->thunk (atom i->thunk) + realized-row-delay (make-realized-row-delay builder i->thunk volatile-transient-row) + realized-keys (atom #{})] ;; this is a gross amount of positional args. But using `reify` makes debugging things too hard IMO. (->TransientRow model rset @@ -370,5 +359,5 @@ col-name->index realized-keys i->thunk - transient-row + volatile-transient-row realized-row-delay))) diff --git a/test/toucan2/jdbc/row_test.clj b/test/toucan2/jdbc/row_test.clj index 21a3ab0..613e279 100644 --- a/test/toucan2/jdbc/row_test.clj +++ b/test/toucan2/jdbc/row_test.clj @@ -53,6 +53,37 @@ (realized-keys row))) (is (not (already-realized? row)))))) +(def ^:private array-map-max-num-keys-before-switching-to-hash-map + "Max number of keys in a `clojure.lang.PersistentArrayMap` before it switches to `clojure.lang.PersistentHashMap`. At + the time of this writing it is 8, but we're looking it up here in case it changes in the future." + ;; HASHTABLE_THRESHOLD is for the size of the underlying array which stores both keys AND values + (let [field (doto (.getDeclaredField clojure.lang.PersistentArrayMap "HASHTABLE_THRESHOLD") + (.setAccessible true)) + int-val (.getInt field clojure.lang.PersistentArrayMap)] + (int (/ int-val 2)))) + +(deftest ^:parallel get-persists-changes-test + (testing ".valAt implementation must persist changes to underlying transient row (#187)" + (do-with-row + (fn [^TransientRow row] + ;; first we need to load up the row with exactly enough values that the next value added to it will cause the + ;; underlying transient row to change from an array map to a hash map. See + ;; https://github.com/clojure/clojure/blob/56d37996b18df811c20f391c840e7fd26ed2f58d/src/jvm/clojure/lang/PersistentArrayMap.java#L516-L517 + (let [^TransientRow row (reduce + (fn [row n] + (assoc row n n)) + row + (range array-map-max-num-keys-before-switching-to-hash-map)) + underlying-row-before @(.volatile_transient_row row)] + ;; now accessing an actual value should update the underlying transient row and put it over the threshold that + ;; converts it from an array map to a hash map + (is (= "Tempest" + (:name row))) + (let [underlying-row-after @(.volatile_transient_row row)] + (testing (format "\nbefore = %s\nafter = %s" (pr-str underlying-row-before) (pr-str underlying-row-after)) + (is (not (identical? underlying-row-before + underlying-row-after)))))))))) + (deftest ^:parallel assoc-test (do-with-row (fn [row]