Issue 298: Additional facts added to an :exists accumulator condition…

[WilliamParker]

… should not cause Rete operations

I added an explicit public exists accumulator - we'll want to support the :exists syntax in the DSL for backwards compatibility in any case, but are there thoughts on allowing users to use the accumulator directly? Good or bad? My instinct is that doing this without a DSL special case would have been preferable originally, and that having an accumulator that the docs can point to, even if using :exists, makes the behavior more transparent to users. For example, since :exists is backed by an accumulator, it has all the characteristics of an accumulator condition around binding groups. It would perhaps be possible to create an ExistsNode that is even more efficient than an accumulator if needed later, but I think we could detect that such a node could be used even if a Clara-supplied accumulator was given by the user via other checks - e.g. equality checks on the accumulator, metadata checks, etc.

@eraserhd this might be of interest to you from recent activity in the Slack channel as concerns the documentation aspects.

[EthanEChristian]

Implementation seems fine, but i cannot seem to find any documentation on the :exists functionality.
I feel like it might deserve some doc, maybe on the expressions page.

[eraserhd]

@EthanEChristian I volunteer to do that as a separate pull request, today or tomorrow. (I'd already put it on my todo list for this week.)

[EthanEChristian]

Thanks @eraserhd

[mrrodriguez]

You may want a small impl-detail comment here that mentions why this needs to return true or nil instead of just being :convert-return-fn pos? (returning false sometimes).

[mrrodriguez]

This made me think, if I wrote a similar accumulator, but returned an actual fact - instead of a fairly not useful "true", like this:

(defn one-fact
  ([] (assoc (all) :convert-return-fn first))
  ([field] (assoc (all field) :convert-return-fn first))

Wouldn't this still not cause the RHS to re-evaluate when there were multiple matches?
I believe accumulators now check that the result of the accumulation isn't the same as it was previously, prior to propagating to downstream nodes.

I bring this up because

1. Curious on general accumulator behavior vs what :exists offers
2. I've found :exists to be less useful since you can't actually do a fact-level variable binding on a fact that matches the condition.

[WilliamParker]

You could use an actual fact as well - but it will be quicker to do the underlying equality check on true than an actual fact in the downstream results.

It is possible to have bindings in the constrains of an exists condition the same as for any other accumulator as I noted in my recent review on oracle-samples/clara-site#33 but that could be confusing as it would end up firing more than once. As far as wanting to perform checks on the actual fact, my thinking here would be that it seems odd to need to dispatch on the value of the fact if you're only asserting in the rule that it exists - do you have a use case in mind?

[mrrodriguez]

@WilliamParker Don't know a specific scenario, but know that it has been helpful many times before to want to find and use a single match, ignoring any possible duplicate matches. Similar to what you'd want a acc/all for, but when you don't want to work with a collection and are instead happy with just using the first.

However, this wasn't what exists currently does, I just was questioning if it'd be more useful. I think a name like one-fact would be needed in this case though, not exists.

As far as perf goes, I think matching the whole fact would probably be pretty fast if the :convert-return-fn was always returning the same identical? fact each time the accumulator was ran on subsequent matches - which it should be with a deterministically ordered collection of matches.

It isn't a blocker to this work here either way. I just think a one-fact is likely to be an accumulator I would find more use for than something that doesn't return a resulting match.

[WilliamParker]

I can see a use-case for that sort of "just one" accumulator, albeit with the risk of users coming to rely on it returning a particular fact when they shouldn't. For example, the first fact inserted if the implementation happened to consistently returned it. However, that would also be an argument against the acc/all accumulator so it may not be a big deal. Perhaps we could add this later or as an additional accumulator. Intuitively to me it is a bit odd to have something called "exists" return a fact - "exists" to me seems like a boolean predicate. In any case not a blocker as you say.