Test condition on equality of variables bound in previous accumulator conditions is ignored
WilliamParker opened this issue · 2 comments
@sparkofreason reported that the following rule does not behave as expected; specifically, it ignores the :test condition.
(defrule lost-game
[?ships <- (acc/count) :from [Ship]]
[?destroyed <- (acc/count) :from [Destroyed]]
[:test (= ?ships ?destroyed)]
=>
(insert! (->Loser)))
The rules network built from this rule doesn't have the :test condition as a descendant of the alpha roots; since facts are provided to the alpha roots and then propagate downward toward rule (ProductionNode) and query (QueryNode) nodes the :test condition will be ignored.
clara.test-rules> (clara.rules/defrule lost-game
[?ships <- (acc/count) :from [Ship]]
[?destroyed <- (acc/count) :from [Destroyed]]
[:test (= ?ships ?destroyed)]
=>
(insert! (->Loser)))
#'clara.test-rules/lost-game
clara.test-rules> (-> (mk-session [lost-game]) .rulebase :alpha-roots (get Ship))
({:env nil,
:children
({:id 2,
:accum-condition {:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Ship, :constraints []}},
:accumulator
{:initial-value 0,
:reduce-fn #function[clara.rules.accumulators/count/fn--16091],
:combine-fn #function[clojure.core/+],
:convert-return-fn #function[clojure.core/identity],
:retract-fn #function[clara.rules.accumulators/count/fn--16093]},
:result-binding :?ships,
:children
({:id 3,
:accum-condition {:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Destroyed, :constraints []}},
:accumulator
{:initial-value 0,
:reduce-fn #function[clara.rules.accumulators/count/fn--16091],
:combine-fn #function[clojure.core/+],
:convert-return-fn #function[clojure.core/identity],
:retract-fn #function[clara.rules.accumulators/count/fn--16093]},
:result-binding :?destroyed,
:children
({:id 4,
:production
{:ns-name clara.test-rules,
:lhs
[{:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Ship, :constraints []}, :result-binding :?ships}
{:accumulator (clara.rules.accumulators/count), :from {:type clara.test_rules.Destroyed, :constraints []}, :result-binding :?destroyed}
{:constraints [(= ?ships ?destroyed)]}],
:rhs (do (insert! (->Loser))),
:name "clara.test-rules/lost-game"},
:rhs #function[clara.test-rules/eval17812/fn--17813]}),
:binding-keys #{},
:new-bindings #{}}),
:binding-keys #{},
:new-bindings #{}}),
:activation #function[clara.test-rules/eval17828/fn--17829]})
@Zylox looked into this and it seems that sort-conditions places the :test condition above the accumulator conditions. It seems that sort-conditions assumes that all equality join variables in a condition can be created by that condition, but that breaks down in this case since we're relating equality join variables to each other, not to fields on a fact, constants, etc. One possible route to solve this would be to replace the usage of analyze-condition with a function that specifically targeted whether a condition can create a binding, not merely whether the binding is an equality join binding.
For anyone experiencing this issue, it can be avoided by using indirection on =. Since Clara special-cases the circumstance in which the first symbol is = (see the equality-expression? function in the compiler something like the following will work:
(defn equals-indirection [& args] (apply = args))
clara.test-rules> (clara.rules/defrule lost-game
[?ships <- (acc/count) :from [Ship]]
[?destroyed <- (acc/count) :from [Destroyed]]
[:test (equals-indirection ?ships ?destroyed)]
=>
(insert! (->Loser)))
Obviously this is confusing from a user's point of view though and it should be fixed; the example directly above should be considered a temporary workaround.
I have submitted a PR for this at #407. The fix is to simply consider any binding variable used a :test node to be an unbound variable. Since TestNode simply passes on a subset of the tokens it receives I don't see any way for it actually create bindings. If such behavior is desired in future I think :let bindings, discussed in #375, would be a better way to approach the problem than a "sneaky" approach like a filter (which is essentially what a :test node is) having a side effect of creating derived data.
There are other issues around condition sorting and binding analysis discussed at #373 but the issues there are much more extensive and their solutions ambiguous. The fix here is much quicker and the desirable behavior unambiguous so I think we can apply this targeted fix for the moment.
Closing since I have merged the PR.