ISSUE-433: Behavior of rules sharing conditions within :or is incorrect #434
Conversation
| 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)) |
There was a problem hiding this comment.
I'm trying to rationalize this.
If we had a case like:
a b
| |
c
| |
d e
where c was a common condition with, a and b as parents and d and e as children, it isn't safe to reuse the same c to propagate to the children d and e in all cases. Instead c needs to propagate to d, eg. only when it is receiving propagation from it's parent a, and similarly, perhaps e should only be propagated to when we are going from b > c > e.
I think this makes sense. c can't be shared in the propagation - or even working memory structure, due to propagation semantics - when it is coming from 2 logically separate parent paths.
So far I'm onboard with this change.
|
The initial approach added 1 second onto the compilation performance test. This test was running in 2.3 seconds, so the addition of another second is probably not acceptable. With the new changes the time is pretty much unchanged. |
There was a problem hiding this comment.
I'm fairly confident this doesn't create regressions and solves the issue at hand. It has the downside of effectively preventing node sharing across productions but I don't see any straightforward way of solving the issue otherwise. In the somewhat longer term we should probably have a namespace with more extensive testing of node sharing both for correctness as here and that nodes are in fact shared in specific circumstances - I don't think we have the latter at present. I can probably work on that in the next few weeks unless someone else wants to, in the meantime +1 to merge of this and we can discuss a release if that would be useful.
| 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 %) |
There was a problem hiding this comment.
The (set parent-ids) call could be at the time of the creation of the function object rather than each time it is called. Probably not a significant issue but I haven't tested it.
| @@ -194,3 +194,31 @@ | |||
| (is (has-fact? (:cold @side-effect-holder) (->Temperature -10 "MCI") )) | |||
|
|
|||
| (is (has-fact? (:subzero @side-effect-holder) (->Temperature -10 "MCI"))))) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
We could probably use a more extensive namespace for testing node sharing, both for when it is done and in scenarios like this, but this is an OK home to this test for now.
There was a problem hiding this comment.
I presume this test fails without the change? On mobile at the moment so I can't test.
There was a problem hiding this comment.
yes, the e-query assertion fails prior to the changes in the PR
For #433, i think this is the most straight forward approach.
cc. @WilliamParker @mrrodriguez