From 3cd64fb4d8b9109fc6349a2da0cf4f56b58370e2 Mon Sep 17 00:00:00 2001
From: Nick Alexander <nalexander@mozilla.com>
Date: Tue, 27 Sep 2016 17:32:09 -0700
Subject: [PATCH] Review comments.

---
 src/common/datomish/db.cljc            |  6 +++---
 src/common/datomish/sqlite_schema.cljc | 10 +++++++---
 src/common/datomish/transact.cljc      |  3 +++
 test/datomish/db_test.cljc             | 13 +++++++++++++
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/src/common/datomish/db.cljc b/src/common/datomish/db.cljc
index fd926685..c41381a8 100644
--- a/src/common/datomish/db.cljc
+++ b/src/common/datomish/db.cljc
@@ -702,14 +702,14 @@
 
             ;; Map searchid -> e.  There's a generic reduce that takes [pair-chan] lurking in here.
             searchid->e
-            (loop [coll {}
+            (loop [coll (transient {})
                    qs qs]
               (let [[q & qs] qs]
                 (if q
                   (let [rs (<? (s/all-rows (:sqlite-connection db) q))
-                        coll* (into coll (map (juxt :searchid :e)) rs)]
+                        coll* (reduce conj! coll (map (juxt :searchid :e) rs))]
                     (recur coll* qs))
-                  coll)))
+                  (persistent! coll))))
             ]
         (util/mapvals (partial get searchid->e) av->searchid))))
 
diff --git a/src/common/datomish/sqlite_schema.cljc b/src/common/datomish/sqlite_schema.cljc
index bdd8e409..50943a44 100644
--- a/src/common/datomish/sqlite_schema.cljc
+++ b/src/common/datomish/sqlite_schema.cljc
@@ -100,6 +100,7 @@
 (defn create-temp-tx-lookup-statement [table-name]
   ;; n.b., v0/value_type_tag0 can be NULL, in which case we look up v from datoms;
   ;; and the datom columns are NULL into the LEFT JOIN fills them in.
+  ;; The table-name is not escaped in any way, in order to allow "temp.dotted" names.
   ;; TODO: update comment about sv.
   [(str "CREATE TABLE IF NOT EXISTS " table-name
         " (e0 INTEGER NOT NULL, a0 SMALLINT NOT NULL, v0 BLOB NOT NULL, tx0 INTEGER NOT NULL, added0 TINYINT NOT NULL,
@@ -113,9 +114,12 @@
            e INTEGER, a SMALLINT, v BLOB, tx INTEGER, value_type_tag SMALLINT)")])
 
 (defn create-temp-tx-lookup-eavt-statement [idx-name table-name]
-  ;; Note that `id_tx_lookup_added` is created and dropped
-  ;; after insertion, which makes insertion slightly faster.
-  ;; Prevent overlapping transactions.  TODO: drop added0?
+  ;; Note that the consuming code creates and drops the indexes
+  ;; manually, which makes insertion slightly faster.
+  ;; This index prevents overlapping transactions.
+  ;; The idx-name and table-name are not escaped in any way, in order
+  ;; to allow "temp.dotted" names.
+  ;; TODO: drop added0?
   [(str "CREATE UNIQUE INDEX IF NOT EXISTS "
         idx-name
         " ON "
diff --git a/src/common/datomish/transact.cljc b/src/common/datomish/transact.cljc
index af963a45..f4ef7e10 100644
--- a/src/common/datomish/transact.cljc
+++ b/src/common/datomish/transact.cljc
@@ -254,9 +254,12 @@
   {:pre [(db/db? db) (report? report)]}
 
   (let [unique-identity? (memoize (partial ds/unique-identity? (db/schema db)))
+        ;; Map lookup-ref -> entities containing lookup-ref, like {[[:a :v] [[[:a :v] :b :w] ...]] ...}.
         groups           (group-by (partial keep db/lookup-ref?) (:entities report))
+        ;; Entities with no lookup-ref are grouped under the key (lazy-seq).
         entities         (get groups (lazy-seq)) ;; No lookup-refs? Pass through.
         to-resolve       (dissoc groups (lazy-seq)) ;; The ones with lookup-refs.
+        ;; List [[:a :v] ...] to lookup.
         avs              (set (map (juxt :a :v) (apply concat (keys to-resolve))))
         ->av             (fn [r] ;; Conditional (juxt :a :v) that passes through nil.
                            (when r [(:a r) (:v r)]))]
diff --git a/test/datomish/db_test.cljc b/test/datomish/db_test.cljc
index c1ca84cb..b6f5ee7e 100644
--- a/test/datomish/db_test.cljc
+++ b/test/datomish/db_test.cljc
@@ -887,6 +887,17 @@
                  [1 :friends 2]}
                (<? (<datoms>= (d/db conn) tx))))))
 
+    (testing "Looks up refs when there are more than 999 refs (all present)"
+      (let
+          [bound (* 999 2)
+           make-add #(vector :db/add (+ 1000 %) :name (str "Ivan-" %))
+           make-ref #(-> {:db/id (d/lookup-ref :name (str "Ivan-" %)) :email (str "Ivan-" % "@" %)})
+           {tx-data1 :tx-data} (<? (d/<transact! conn (map make-add (range bound))))
+           {tx-data2 :tx-data} (<? (d/<transact! conn (map make-ref (range bound))))]
+        (is (= bound (dec (count tx-data1)))) ;; Each :name is new; dec to account for :db/tx.
+        (is (= bound (dec (count tx-data2)))) ;; Each lookup-ref exists, each :email is new; dec for :db/tx.
+        ))
+
     (testing "Fails for missing entities"
       (is (thrown-with-msg?
             ExceptionInfo #"No entity found for lookup-ref"
@@ -904,3 +915,5 @@
             (<? (d/<transact! conn [[:db/add 1 :friends (d/lookup-ref :aka "The Magician")]])))))))
 
 #_ (time (t/run-tests))
+
+#_ (time (clojure.test/test-vars [#'test-lookup-refs]))