From 1ad67a03ebb4445146869e3a11db10bfe42791d1 Mon Sep 17 00:00:00 2001
From: Richard Newman <rnewman@twinql.com>
Date: Tue, 26 Jul 2016 11:19:51 -0700
Subject: [PATCH] Add tests and comments for clause ordering.

---
 src/datomish/query/clauses.cljc |  7 +++--
 test/datomish/test/query.cljc   | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/src/datomish/query/clauses.cljc b/src/datomish/query/clauses.cljc
index 9a4826e4..9d8e4d50 100644
--- a/src/datomish/query/clauses.cljc
+++ b/src/datomish/query/clauses.cljc
@@ -101,8 +101,11 @@
    once will result in duplicate clauses."
   [cc]
   (impose-external-bindings
-    (assoc cc :wheres (concat (bindings->where (:bindings cc))
-                              (:wheres cc)))))
+    (assoc cc :wheres
+           ;; Note that the order of clauses here means that cross-pattern var bindings
+           ;; come first. That's OK: the SQL engine considers these altogether.
+           (concat (bindings->where (:bindings cc))
+                   (:wheres cc)))))
 
 ;; Pattern building is recursive, so we need forward declarations.
 (declare Not->NotJoinClause not-join->where-fragment)
diff --git a/test/datomish/test/query.cljc b/test/datomish/test/query.cljc
index 4b21cc59..1fa40f7d 100644
--- a/test/datomish/test/query.cljc
+++ b/test/datomish/test/query.cljc
@@ -80,3 +80,48 @@
              [?page :page/starred true ?t]
              [?t :db/txInstant ?timestampMicros]
              (not [?page :foo/bar _])]))))
+
+;; Note that clause ordering is not directly correlated to the output: cross-bindings end up
+;; at the front. The SQL engine will do its own analysis. See `clauses/expand-where-from-bindings`.
+(deftest test-not-clause-ordering-preserved
+  (is (= {:select '([:datoms1.v :timestampMicros] [:datoms0.e :page]),
+          :modifiers [:distinct],
+          :from '[[:datoms datoms0]
+                  [:datoms datoms1]],
+          :where (list
+                   :and
+                   [:= :datoms1.e :datoms0.tx]
+                   [:= :datoms0.a "page/starred"]
+                   [:= :datoms0.v 1]
+                   [:not
+                    (list :and (list :> :datoms0.tx (sql/param :latest)))]
+                   [:= :datoms1.a "db/txInstant"])}
+         (expand
+           '[:find ?timestampMicros ?page :in $ ?latest :where
+             [?page :page/starred true ?t]
+             (not [(> ?t ?latest)])
+             [?t :db/txInstant ?timestampMicros]]))))
+
+(deftest test-pattern-not-join-ordering-preserved
+  (is (= '{:select ([:datoms2.v :timestampMicros] [:datoms0.e :page]),
+           :modifiers [:distinct],
+           :from [[:datoms datoms0]
+                  [:datoms datoms2]],
+           :where (:and
+                     [:= :datoms2.e :datoms0.tx]
+                     [:= :datoms0.a "page/starred"]
+                     [:= :datoms0.v 1]
+                     [:not
+                      [:exists
+                       {:select [1],
+                        :from [[:datoms datoms1]],
+                        :where (:and
+                                  [:= :datoms1.a "foo/bar"]
+                                  [:= :datoms0.e :datoms1.e])}]]
+                     [:= :datoms2.a "db/txInstant"]
+                     )}
+         (expand
+           '[:find ?timestampMicros ?page :in $ ?latest :where
+             [?page :page/starred true ?t]
+             (not [?page :foo/bar _])
+             [?t :db/txInstant ?timestampMicros]]))))