Retracting facts that were never inserted

[mrrodriguez]

I think this issue may be specific to AccumulateNode. The rest of the nodes seem ok, however, I think NegationWithJoinFilterNode could have a subtle inefficiency around it. NegationWithJoinFilterNode still performs correctly though it looks like.

I plan to fix the specific issue with AccumulateNode with #190, but I want to address the topic as a whole, at least for understanding of the motivation behind it.

Firstly, the RootJoinNode right-activate has a comment at [1], but inlining it for discussion

;; Add elements to the working memory to support analysis tools.
(mem/add-elements! memory node join-bindings elements)

This suggest that storing the added elements in memory is for nothing more than tooling. I do not think this is the whole story. Storing elements in memory is important because when right-retract happens [2], we need to be sure at some level that the elements being retracted were ever even added in the first place.

Since the RootJoinNode does this in right-activate, the right-retract always ensures that the elements being retracted, were actually added in the first place. In this way, the RootJoinNode is the safeguard against the caller attempting to retract facts, that were never added in the first place.

---

The issue I have noticed is that when compiling the rulebase, the RootJoinNode is not always the direct child of AlphaNodes, even when there are no beta nodes to connect to the left side of these children of the AlphaNodes nodes.

I'll just use a concrete example with AccumulateNode to show how things can go wrong currently (prior to the work I'll do with #190 on AccumulateNode).

(let [p (dsl/parse-query [] [[?a <- (acc/sum :temperature) :from [Temperature]]])
      s (mk-session [p])]
  (-> s
      (retract (->Temperature 10 "MCI")) ; retracted, but never inserted in the first place
      fire-rules
      (query p)))
;;= ({:?a -10})

Looking at the rulebase and cutting out the irrelevant parts:

(.rulebase s)

(comment
  {:alpha-roots
   {clara.rules.testfacts.Temperature
    (#clara.rules.engine.AlphaNode
     {:env nil,
      :children
      (#clara.rules.engine.AccumulateNode
       {:id 1,
        :accum-condition {:accumulator (clara.rules.accumulators/sum :temperature), :from {:type clara.rules.testfacts.Temperature, :constraints []}},
        <...the rest truncated...>
        })})}})

The rulebase here ends up not having a RootJoinNode at all. The AlphaNode connects directly to an AccumulateNode. The important part in this particular situation is the AccumulateNode does not have any safeguard that facts being retracted via right-retract were ever actually inserted in the first place. Again, I can fix this specific issue since my change to AccumulateNode will retain some memory of facts seen before via insertions so that the retraction can be careful to only retract what has been inserted before.

My greater concern and question is if it makes sense to have beta roots of the rulebase other than the RootJoinNode. Perhaps this does help in cutting out an unnecessary node when we can work without it. However, each node then does need to be sure to properly "defend" against right-retracts of elements that may have never been inserted in the first place.

---

So the point of this topic isn't to directly address the AccumulateNode issue. That'll be addressed with #190. The point of this is, for me at least, to gain a better understanding of Clara's usage of the RootJoinNode and when and why we use it or do not use it in the compiler.
I also intend to improve the "tooling" comment in right-activate of the RootJoinNode because I think it is actually needed for correctness of token retractions propagated in some situations. That is minor.

[1] https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/engine.cljc#L369-L370
[2] https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/engine.cljc#L390

[rbrush]

I introduced the RootJoinNode as a performance optimization, not something needed for semantic correctness. When doing early profiling I noticed a lot of time was being spent in the join nodes when the nature of the rules meant they would always be activated...so RootJoinNode is a special case of join node that takes advantage of this. But we should be correct with any beta node at the root...barring bugs like what you're addressing in #190.

[mrrodriguez]

Ok, that is fair and fine. I was making sure there was no oversight here and that things were working as expected. +1 to all nodes that may potentially be a beta root properly handling cases like this. AccumulateNode will soon (my pull request round 1 isn't too far away now).

[WilliamParker]

@mrrodriguez is there anything left to do here?

[mrrodriguez]

@WilliamParker

This can be closed.