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.

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

[jdbc.read] Remove value caching from JDBC reading machinery #183

Merged
merged 1 commit into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
74 changes: 15 additions & 59 deletions src/toucan2/jdbc/read.clj
Original file line number Diff line number Diff line change
Expand Up @@ -136,54 +136,17 @@
v (thunk)]
(next.jdbc.rs/read-column-by-index v rsmeta i))))

(defn- make-i->thunk [conn model rset]
(defn ^:no-doc make-i->thunk
"Given a connection `conn`, a `model` and a result set `rset`, return a function which given a column number `i` returns
a thunk that retrieves the column value of the current row from the result set."
[conn model rset]
(comp (memoize (fn [i]
(make-column-thunk conn model rset i)))
int))

(defn ^:no-doc make-cached-row-num->i->thunk
"Returns a function that, given the current row number, returns a function that, given a column number, returns a cached
thunk to fetch values of that column. Confusing, huh? Here's a chart to make it a bit easier to visualize:

```clj
(make-cached-row-num->i->thunk conn model rset)
=>
(f current-row-number)
=>
(f column-index)
=>
(column-value-thunk)
```

What's the point of this? The main point is to cache values that come out of the database, so we only fetch them once.
This is used to implement our transient rows in [[toucan2.jdbc.result-set]] -- accessing the value of a transient row
twice should not result in two calls to `.getObject`.

The row number is used for cache-busting purposes, so we can reset the cache after each row is returned (so we don't
accidentally cache values from the first row and return them for all the rows). The row number passed in here doesn't
need to correspond to the actual row number from a JDBC standpoint; it's used only for cache-busting purposes."
[conn model ^ResultSet rset]
(let [i->thunk (make-i->thunk conn model rset)
cached-row-num (atom -1)
cached-values (atom {})]
(fn row-num->i->thunk* [current-row-num]
(when-not (= current-row-num @cached-row-num)
(reset! cached-row-num current-row-num)
(reset! cached-values {}))
(fn i->thunk* [i]
(fn cached-column-thunk []
(let [cached-value (get @cached-values i ::not-found)]
(if-not (= cached-value ::not-found)
(log/tracef "Using cached value for column %s: %s" i cached-value)
cached-value)
(let [thunk (i->thunk i)
v (thunk)]
(swap! cached-values assoc i v)
v)))))))

(defn ^:no-doc read-column-by-index-fn
"Given a `java.sql.Connection` `conn`, a `model`, and a `java.sql.ResultSet` `rset`, return a function that can be used
with [[next.jdbc.result-set/builder-adapter]]. The function has the signature
"Given a `i->thunk` function, return a function that can be used with [[next.jdbc.result-set/builder-adapter]]. The
function has the signature

```clj
(f builder rset i) => result
Expand All @@ -193,19 +156,12 @@
index `i`, it will return the value of that column for the current row.

The function used to fetch the column is built by combining [[read-column-thunk]]
and [[next.jdbc.result-set/read-column-by-index]]; the function is built once and used repeatedly for each new row.

Values are cached for the current row -- fetching the same column twice for a given row will only result in fetching
it from the database once."
([conn model ^ResultSet rset]
(read-column-by-index-fn (make-cached-row-num->i->thunk conn model rset)))

([row-num->i->thunk]
(fn read-column-by-index-fn* [_builder ^ResultSet rset ^Integer i]
(assert (not (.isClosed ^ResultSet rset))
"ResultSet is already closed. Do you need call toucan2.realize/realize on the row before the Connection is closed?")
(let [i->thunk (row-num->i->thunk (.getRow rset))
thunk (i->thunk i)
result (thunk)]
(log/tracef "col %s => %s" i result)
result))))
and [[next.jdbc.result-set/read-column-by-index]]; the function is built once and used repeatedly for each new row."
[i->thunk]
(fn read-column-by-index-fn* [_builder ^ResultSet rset ^Integer i]
(assert (not (.isClosed ^ResultSet rset))

Check warning on line 162 in src/toucan2/jdbc/read.clj

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/read.clj#L162

Added line #L162 was not covered by tests
"ResultSet is already closed. Do you need call toucan2.realize/realize on the row before the Connection is closed?")
(let [thunk (i->thunk i)
result (thunk)]

Check warning on line 165 in src/toucan2/jdbc/read.clj

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/read.clj#L164-L165

Added lines #L164 - L165 were not covered by tests
(log/tracef "col %s => %s" i result)
result)))

Check warning on line 167 in src/toucan2/jdbc/read.clj

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/read.clj#L167

Added line #L167 was not covered by tests
8 changes: 3 additions & 5 deletions src/toucan2/jdbc/result_set.clj
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@
directly."
[rf init conn model ^ResultSet rset opts]
(log/debugf "Reduce JDBC result set for model %s with rf %s and init %s" model rf init)
(let [row-num->i->thunk (jdbc.read/make-cached-row-num->i->thunk conn model rset)
(let [i->thunk (jdbc.read/make-i->thunk conn model rset)
builder-fn* (next.jdbc.rs/builder-adapter
(builder-fn conn model rset opts)
(jdbc.read/read-column-by-index-fn row-num->i->thunk))
(jdbc.read/read-column-by-index-fn i->thunk))
builder (builder-fn* rset opts)
combined-opts (jdbc.options/merge-options (merge (:opts builder) opts))
label-fn (get combined-opts :label-fn)
Expand All @@ -151,9 +151,7 @@
(log/tracef "Result set has no more rows.")
acc)

:let [row-num (.getRow rset)
_ (log/tracef "Fetch row %s" row-num)
i->thunk (row-num->i->thunk row-num)
:let [_ (log/tracef "Fetch row %s" (.getRow rset))
row (jdbc.row/row model rset builder i->thunk col-name->index)
acc' (rf acc row)]

Expand Down
Loading