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.