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

[jdbc] Refactor make-cached-row-num->i->thunk #179

Closed
wants to merge 2 commits into from
Closed

[jdbc] Refactor make-cached-row-num->i->thunk #179

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8295b6a
[jdbc] Fix cached values being ignored in make-cached-row-num->i->thunk
alexander-yakushev Sep 13, 2024
bf301c2
[jdbc] Get rid of extra thunking
alexander-yakushev Sep 13, 2024
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
36 changes: 15 additions & 21 deletions src/toucan2/jdbc/read.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,12 @@
[_conn _model rset _rsmeta i]
(get-object-of-class-thunk rset i java.time.OffsetTime))

(defn- make-column-thunk [conn model ^ResultSet rset i]
(log/tracef "Building thunk to read column %s" i)
(fn column-thunk []
(let [rsmeta (.getMetaData rset)
thunk (read-column-thunk conn model rset rsmeta i)
v (thunk)]
(next.jdbc.rs/read-column-by-index v rsmeta i))))

(defn- make-i->thunk [conn model rset]
(comp (memoize (fn [i]
(make-column-thunk conn model rset i)))
int))
(defn- read-column-value [conn model ^ResultSet rset i]
(let [i (int i)
rsmeta (.getMetaData rset)
thunk (read-column-thunk conn model rset rsmeta i)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it is actually somewhat important here to calculate these thunks just once for the entire result set instead of once per value. The overhead of calling the read-column-thunk multimethod on every single value in the results can get pretty high.

This whole read-column-thunk idea came about when I did something similar in Metabase. Some of these read-column-thunk methods have to look at ResultSetMetaData to determine how to read a column out or make other decisions like that that only need to happen once per ResultSet rather than once per row. Here's an example with Postgres in Metabase:

https://github.com/metabase/metabase/blob/f49407fcbdf18f40e247437f41b3bbefd1e227bf/src/metabase/driver/postgres.clj#L795-L803

Both timestamp and timestamp with time zone come back as java.sql.Types/TIMESTAMP so you need to look at the ResultSetMetaData to determine what the actual column type is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your intent and agree that it would be beneficial. The problem is, if I'm reading the code correctly, the process that you describe is not actually cached right now. Obtaining metadata and calling read-column-thunk happens here https://github.com/camsaul/toucan2/blob/master/src/toucan2/jdbc/read.clj#L134, inside the cached lambda, and so it happens for each row.

I can try to rewrite this code so that it is properly cached and reused.

v (thunk)]
(next.jdbc.rs/read-column-by-index v rsmeta i)))

(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
Expand All @@ -163,8 +157,7 @@
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)
(let [cached-row-num (atom -1)
cached-values (atom {})]
(fn row-num->i->thunk* [current-row-num]
(when-not (= current-row-num @cached-row-num)
Expand All @@ -173,13 +166,14 @@
(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)))))))
(if (= cached-value ::not-found)
;; miss
(let [v (read-column-value conn model rset i)]
(swap! cached-values assoc i v)
v)
;; hit
(do (log/tracef "Using cached value for column %s: %s" i cached-value)
cached-value))))))))

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

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/read.clj#L175-L176 

Added lines #L175 - L176 were not covered by tests

(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
Expand Down
Loading