Binding to values derived from previous variables

[mrrodriguez]

The use-case for this one is a little difficult to explain, but let me try.

I'll start with some motivating examples where I think the resulting error is at least confusing.

Example (1) This first example compiles, but not into something meaningful.

(def compiles-query (dsl/parse-query []
                                     [[Temperature (= ?x temperature)]
                                      [ColdAndWindy (= ?t (+ temperature ?x))]]))


(-> (mk-session [compiles-query] :cache false)
    (insert (->Temperature 10 "MCI")
            (->ColdAndWindy 5 50))
    fire-rules
    (query compiles-query))

;= ()

The issue here lies in the extracted TestNode that is generated to try and deal with the non-hash-based join condition.
This TestNode fn looks something like [1]:

(clojure.core/fn [?__token__]

  (clojure.core/let [?x (get-in ?__token__ [:bindings :?x])

                     ?__gen__28586 (get-in ?__token__ [:bindings :?__gen__28586])

                     ?t (get-in ?__token__ [:bindings :?t])]

    (clojure.core/and (= ?t (+ ?__gen__28586 ?x)))))

Where ?__gen__28586 is bound to temperature of the :original-condition.
This doesn't make sense since ?t doesn't have a binding - it just comes out nil.

Example (2) This doesn't compile into a Rete tree, but the error reported may be less-than-ideal.

(def does-not-compile-query (dsl/parse-query []
                                             [[Temperature (= ?x temperature)]
                                              [ColdAndWindy (= ?t (inc ?x))]]))

(-> (mk-session [does-not-compile-query] :cache false)
                      (insert (->Temperature 10 "MCI")
                              (->ColdAndWindy 5 50))
                      fire-rules
                      (query does-not-compile-query))

;; CompilerException java.lang.RuntimeException: Unable to resolve symbol: ?x in this context, compiling:(etc)

To the caller, it looks like ?x should be resolvable.

Given these examples, I can see how the immediate response would be that it doesn't make sense to set up a join binding like this since it isn't really a "join".
This usage of a variable binding is more to create what I'd consider a "local binding" so that you can reuse some computation elsewhere in the rule.
Example (1) can be extended to demonstrate this idea (although there is no reason to do this particular example this way) :

(def compiles-query-revised (dsl/parse-query []
                                             [[Temperature (= ?x temperature)]
                                              [ColdAndWindy (= ?t (+ temperature ?x))]
                                              [:test (< 5 ?t)]]))

(-> (mk-session [compiles-query-revised] :cache false)
    (insert (->Temperature 10 "MCI")
            (->ColdAndWindy 5 50))
    fire-rules
    (query compiles-query-revised))

;= ()

The result of this is not intuitive from my perspective and I've seen other be confused by it.

Overall, I think this may be using the join bindings for an unintended use. If that is the case, I'd vote in favor of putting some guards against these things in the form of throwing meaningful exceptions during Rete compilation.

On the other hand, there are times when it is "convenient" to store a derived calculation made in a LHS condition as a variable binding so that it can be tested in other subsequent conditions and used in the RHS. It could aid performance to not repeat the same calculation in the LHS and RHS for example.

This works (and doesn't make much sense, but keeping it short):

(let [r (dsl/parse-rule [[Temperature (= ?one-more (inc temperature))]
                         [:test (< ?one-more 30)]]
                        (insert! (->Cold ?one-more)))
      q (dsl/parse-query []
                         [[?c <- Cold]])]
  (-> (mk-session [r q])
      (insert (->Temperature 10 "MCI"))
      fire-rules
      (query q)))

;= ({:?c #clara.rules.testfacts.Cold{:temperature 11}})

This can probably be abused as well as an aid to writing poorly designed rules.

I have been able to track down the internals in clara.rules.compiler that contribute to this issue. I am just not sure on the direction we want to take with usages like this.

[1] Separate issue I'd probably like to think about: I had to add to the TestNode structure on the Rete tree in order to visually see what a TestNode condition looks like prior to eval. I think it would aid in debugging and visualizing the Rete tree if the un-eval'ed form was preserved on a field of the TestNode. Same goes for other join filter fn's etc.

[rbrush]

I think we should support this...or at least I can't think of any reason not to that would make sense to an end user. It isn't a typical join/test scenario but I don't want to force users to be aware of that distinction.

I think the key pieces here are going to be:

• Test nodes will need to be able to create new bindings, since in the first example we don't have all of the input necessary to compute the binding until we do the extracted test
• To make that happen we'll probably have to fix issue Use a topological sort of variable bindings when building the beta tree #133, since tests would be able to create new bindings that could potentially be used by other rules.
• We may have to enhance the extract-tests functionality from join nodes to make sure these use case are covered. (It might be working already but obviously this flow isn't unit tested.)

The trickiest algorithmic part is probably going to be sorting out the order we need to place the nodes so all of the bound variables are visible. But this is something we should solve anyway as described in issue #133.

I'll poke around this a bit more tomorrow.

[rbrush]

Looking at this a bit more closely, I think the best approach will be to refactor the extract-tests type of functionality into a specialized join node that runs arbitrary tests and can create new bindings. So we might rename the current JoinNode to "HashJoinNode" and create an "ExpressionJoinNode" that can run arbitrary logic on the beta side of the network. HashJoinNode is more efficient, but a new ExpressionJoinNode will be used when needed to handle arbitrary expressions and relationships.

I think this simplifies things in the long run, since we will no longer need to artificially transform our network into join and test nodes, and it also simplifies ordering of the nodes as well.

This change won't be trivial, but it does drop some existing complexity and ensures our nodes can express any expression you could put in a rule.

[mrrodriguez]

I think that approach makes sense to me when going the direction of allowing bindings to be introduced in test nodes. I agree it'd likely be simpler to reason about with less transformations.

Thanks for looking into this and taking the time to post your design thoughts out here along the way.

[rbrush]

The above commit should fix this (at least, the examples included in the bug report now work and are included in a unit test.)

Most of the work done was in the refactoring for issue #133. The change in the above commit simply reuses the same binding logic used for fact matching to run arbitrary cross-fact expressions.

I'll plan on merging this to master soon unless some flaw presents itself. Mike, please let me know if you run into any problems with this logic.

[mrrodriguez]

Thanks! I am working on testing this out and seeing how it works out.

[mrrodriguez]

I ran this against my existing codebase and it looks good. It caught one misused bindings in a condition, but I don't consider that a bad thing. There were some odd cases before where a condition was able to refer to a binding in one constraint that it introduced in a previous constraint. I think it was just related to the test extraction before, but I never dug into it. It was confusing that it worked then though and I generally advised against it.
So other than that, this looks good so far. I scanned through the changes and it looks like it probably is a bit clearer what is going on now. The only thing I haven't really looked to hard at is this topo sort, but I am going to check that out since it looks interesting.

This looks like quite a bit of a change. Thanks for getting to it so fast!

[rbrush]

Merged into master! I'm going to go ahead and close this now, but we can create new issues if anything related pops up.