Skip to content

Commit

Permalink
[jdbc.read] Don't track realized keys in TransientRow
Browse files Browse the repository at this point in the history
  • Loading branch information
@alexander-yakushev
alexander-yakushev committed Oct 5, 2024
1 parent 86929ea commit 76a77f9
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 48 deletions.
44 changes: 24 additions & 20 deletions src/toucan2/jdbc/result_set.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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')
Expand Down
25 changes: 12 additions & 13 deletions src/toucan2/jdbc/row.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]].
Expand All @@ -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)
Expand All @@ -90,7 +89,7 @@
rset
builder
column-name->index
realized-keys
column-names

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

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/row.clj#L92 

Added line #L92 was not covered by tests
i->thunk
transient-row'
realized-row)))))
Expand Down Expand Up @@ -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')
Expand All @@ -128,7 +126,7 @@
rset
builder
column-name->index'
realized-keys
column-names
i->thunk
transient-row'
realized-row)))))
Expand Down Expand Up @@ -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)]))))

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

View check run for this annotation

Codecov / codecov/patch

src/toucan2/jdbc/row.clj#L302 

Added line #L302 was not covered by tests
column-names)])
(catch Exception _
["unrealized result set {row} -- do you need to call toucan2.realize/realize ?"])))

Expand Down Expand Up @@ -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)))
15 changes: 0 additions & 15 deletions test/toucan2/jdbc/row_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)))

Expand All @@ -49,25 +46,19 @@
(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
(get row :k)
(: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)))
Expand All @@ -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)
Expand All @@ -107,17 +96,13 @@
(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
(do-with-row
(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
Expand Down

0 comments on commit 76a77f9

Please sign in to comment.