Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make sure fetching column from transient row persists changes where needed #188

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Make sure fetching column from transient row persists changes where needed #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 51 additions & 62 deletions src/toucan2/jdbc/row.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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))))

Check warning on line 84 in src/toucan2/jdbc/row.clj

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/row.clj#L80-L84 

Added lines #L80 - L84 were not covered by tests
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]
Expand All @@ -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
;;
Expand All @@ -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]
Expand All @@ -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")
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))))

Expand All @@ -359,16 +348,16 @@
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
builder
col-name->index
realized-keys
i->thunk
transient-row
volatile-transient-row
realized-row-delay)))
31 changes: 31 additions & 0 deletions test/toucan2/jdbc/row_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Loading