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 d6934639..8d9699cd 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -1136,20 +1136,33 @@ ;; 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 - (assoc! m node (min prev id)) - (assoc! m node id)))) - - node->id (-> (reduce update-node->id - (transient {}) - forward-edges) - persistent!) + 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 (get node->id node) + 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 %) + parent-ids-set) + %) + common-nodes)) (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)) + #{}))))