From 4b534edd7cf58b608eab0ef35ea4012c11642a96 Mon Sep 17 00:00:00 2001 From: ec027900 Date: Mon, 9 Sep 2019 10:30:34 -0500 Subject: [PATCH 1/3] ISSUE-433: Behavior of rules sharing conditions within :or is incorrect --- src/main/clojure/clara/rules/compiler.clj | 13 ++++++--- src/test/common/clara/test_simple_rules.cljc | 28 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index d6934639..07ba5f79 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -1124,6 +1124,8 @@ id-to-condition-nodes (:id-to-condition-node beta-graph) + backward-edges (:backward-edges beta-graph) + ;; Since we require that id-to-condition-nodes have an equal value to "node" under the ID ;; for this to be used. In any possible edge cases where there are equal nodes under different IDs, ;; maintaining the lowest node id will add determinism. @@ -1138,10 +1140,13 @@ ;; will out perform equivalence checks. update-node->id (fn [m id] (let [node (get id-to-condition-nodes id) - prev (get m node)] + ;; appending parents to allow for identical conditions that do not share the same + ;; parents, see Issue 433 for more information + node-with-parents (assoc node :parents (get backward-edges id)) + prev (get m node-with-parents)] (if prev - (assoc! m node (min prev id)) - (assoc! m node id)))) + (assoc! m node-with-parents (min prev id)) + (assoc! m node-with-parents id)))) node->id (-> (reduce update-node->id (transient {}) @@ -1149,7 +1154,7 @@ persistent!) ;; Use the existing id or create a new one. - node-id (or (get node->id node) + node-id (or (get node->id (assoc node :parents (set parent-ids))) (create-id-fn)) graph-with-node (add-node beta-graph parent-ids node-id node)] diff --git a/src/test/common/clara/test_simple_rules.cljc b/src/test/common/clara/test_simple_rules.cljc index 8a18f2e5..4c2f3157 100644 --- a/src/test/common/clara/test_simple_rules.cljc +++ b/src/test/common/clara/test_simple_rules.cljc @@ -194,3 +194,31 @@ (is (has-fact? (:cold @side-effect-holder) (->Temperature -10 "MCI") )) (is (has-fact? (:subzero @side-effect-holder) (->Temperature -10 "MCI"))))) + + +;; A test to validate that nodes of different parents will not be shared. +;; See issue 433 for more information +(def-rules-test test-or-sharing-same-condition + {:rules [or-rule [[[:or + [::a] + [::b]] + [::d]] + (insert! {:fact-type ::c})] + + other-rule [[[::a] + [::d]] + (insert! {:fact-type ::e})]] + :queries [c-query [[] [[?c <- ::c]]] + e-query [[] [[?e <- ::e]]]] + + :sessions [empty-session [or-rule other-rule c-query e-query] {:fact-type-fn :fact-type}]} + (let [session (-> empty-session + (insert {:fact-type ::b}) + (insert {:fact-type ::d}) + fire-rules)] + + (is (= (set (query session c-query)) + #{{:?c {:fact-type ::c}}})) + + (is (= (set (query session e-query)) + #{})))) From 70976589671e3bb3d495bcbb7b2c6ffcd6f713d9 Mon Sep 17 00:00:00 2001 From: ec027900 Date: Thu, 12 Sep 2019 09:18:37 -0500 Subject: [PATCH 2/3] Add Changelog entry, fix perf degredation --- CHANGELOG.md | 1 + src/main/clojure/clara/rules/compiler.clj | 32 ++++++++++++++--------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ba7a77b..513d20d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ This is a history of changes to clara-rules. # 0.20.0 SNAPSHOT * Add a flag to omit compilation context (used by the durability layer) after Session compilation to save space when not needed. Defaults to true. [issue 422](https://github.com/cerner/clara-rules/issues/422) * Correct duplicate bindings within the same condition. See [issue 417](https://github.com/cerner/clara-rules/issues/417) +* Correct sharing of nodes with different parents. See [issue 433](https://github.com/cerner/clara-rules/issues/433) # 0.19.1 * Added a new field to the clara.rules.engine/Accumulator record. This could be a breaking change for any user durability implementations with low-level performance optimizations. See [PR 410](https://github.com/cerner/clara-rules/pull/410) for details. diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index 07ba5f79..6b46e4a6 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -1124,8 +1124,6 @@ id-to-condition-nodes (:id-to-condition-node beta-graph) - backward-edges (:backward-edges beta-graph) - ;; Since we require that id-to-condition-nodes have an equal value to "node" under the ID ;; for this to be used. In any possible edge cases where there are equal nodes under different IDs, ;; maintaining the lowest node id will add determinism. @@ -1140,21 +1138,29 @@ ;; will out perform equivalence checks. update-node->id (fn [m id] (let [node (get id-to-condition-nodes id) - ;; appending parents to allow for identical conditions that do not share the same - ;; parents, see Issue 433 for more information - node-with-parents (assoc node :parents (get backward-edges id)) - prev (get m node-with-parents)] + prev (get m node)] (if prev - (assoc! m node-with-parents (min prev id)) - (assoc! m node-with-parents id)))) + ;; There might be nodes with multiple representation under the same parent, this would + ;; likely be due to nodes that have the same condition but do not share all the same + ;; parents, see Issue 433 for more information + (assoc! m node (conj prev id)) + (assoc! m node [id])))) - node->id (-> (reduce update-node->id - (transient {}) - forward-edges) - persistent!) + node->ids (-> (reduce update-node->id + (transient {}) + forward-edges) + persistent!) + + backward-edges (:backward-edges beta-graph) ;; Use the existing id or create a new one. - node-id (or (get node->id (assoc node :parents (set parent-ids))) + node-id (or (when-let [common-nodes (get node->ids node)] + ;; We need to validate that the node we intend on sharing shares the same parents as the + ;; current node we are creating. See Issue 433 for more information + (some #(when (= (get backward-edges %) + (set parent-ids)) + %) + common-nodes)) (create-id-fn)) graph-with-node (add-node beta-graph parent-ids node-id node)] From 57aab0c9f62c929c9935989b9e78c963dc46e0a1 Mon Sep 17 00:00:00 2001 From: ec027900 Date: Mon, 30 Sep 2019 08:11:38 -0500 Subject: [PATCH 3/3] Extract set call, Update fn name --- src/main/clojure/clara/rules/compiler.clj | 26 ++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index 6b46e4a6..8d9699cd 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -1136,29 +1136,31 @@ ;; all of the old children for a prior match, for each additional child added we incur the assoc but the hash code ;; has already been cached by prior iterations of add-conjunctions. In these cases we have seen that the hashing ;; will out perform equivalence checks. - update-node->id (fn [m id] - (let [node (get id-to-condition-nodes id) - prev (get m node)] - (if prev - ;; There might be nodes with multiple representation under the same parent, this would - ;; likely be due to nodes that have the same condition but do not share all the same - ;; parents, see Issue 433 for more information - (assoc! m node (conj prev id)) - (assoc! m node [id])))) - - node->ids (-> (reduce update-node->id + update-node->ids (fn [m id] + (let [node (get id-to-condition-nodes id) + prev (get m node)] + (if prev + ;; There might be nodes with multiple representation under the same parent, this would + ;; likely be due to nodes that have the same condition but do not share all the same + ;; parents, see Issue 433 for more information + (assoc! m node (conj prev id)) + (assoc! m node [id])))) + + node->ids (-> (reduce update-node->ids (transient {}) forward-edges) persistent!) backward-edges (:backward-edges beta-graph) + parent-ids-set (set parent-ids) + ;; Use the existing id or create a new one. node-id (or (when-let [common-nodes (get node->ids node)] ;; We need to validate that the node we intend on sharing shares the same parents as the ;; current node we are creating. See Issue 433 for more information (some #(when (= (get backward-edges %) - (set parent-ids)) + parent-ids-set) %) common-nodes)) (create-id-fn))