oracle-samples/clara-rules

Regression from optimization of #202

Closed this issue · 2 comments

The following should pass:

(def test-accum-needing-token-partitions-correctly-on-fact-binding
  (let [q (dsl/parse-query []
                           [[WindSpeed]
                            [?ts <- (acc/all) :from [Temperature (= ?loc location)]]])
        s (mk-session [q] :cache false)
        t1mci (->Temperature 1 "MCI")
        t2lax (->Temperature 2 "LAX")
        right-activates-before-left (-> s
                                        (insert t1mci
                                                t2lax)
                                        (insert (->WindSpeed 10 "MCI"))
                                        fire-rules
                                        (query q)
                                        frequencies)
        left-activates-before-right (-> s
                                        (insert (->WindSpeed 10 "MCI"))
                                        (insert t1mci
                                                t2lax)
                                        fire-rules
                                        (query q)
                                        frequencies)
        batched-inserts (-> s
                            (insert (->WindSpeed 10 "MCI")
                                    t1mci
                                    t2lax)
                            fire-rules
                            (query q)
                            frequencies)]

    (is (= (frequencies [{:?ts [t1mci] :?loc "MCI"}
                         {:?ts [t2lax] :?loc "LAX"}])
           right-activates-before-left
           left-activates-before-right
           batched-inserts))))

However, it fails in the right-activates-before-left case. The cause is due to a incorrect optimization made in the work done to the AccumulateNode in #202. This happens when facts are accumulated per fact-binding group prior to a token being inserted. The optimization was that the accumulation only had to be done once for all facts and could then be passed to all tokens in the join. This is not true. The accumulation must be done once per fact-binding group of facts. The result of that can be applied to every joined token thereafter.

I'm going with the simple fix here. It is possible that we could try to make use of the new-bindings information that is now available to accumulate nodes that is now being used and available as of #102. The trick may be that if the node does not have new-bindings that we could still do an optimization like this, but that'd take more thought. I'd like to just fix the regression in this issue. I don't think this optimization is providing significant performance benefits anyways since the number of fact-bindings groups is probably low in practice.

I'll have a PR to address this soon.

PR for this is #216

Pull request has been merged, so closing this one.