Repository navigation
Conversation
|
The main motivation for this change is the lack of observability on filtered facts in a read only session. |
mrrodriguez
left a comment
mrrodriguez
left a comment
There was a problem hiding this comment.
I think this is making sense. There are a few nice improvements as side effects of doing this. I have a few comments to consider. I haven't looked closely at tests or the durability impl yet.
| ;; accumulated items on this node. | ||
| (get-accum-reduced-complete [memory node]) | ||
|
|
||
| ;; Returns the facts that passed the join filter of an accumulator node with a join filter. |
There was a problem hiding this comment.
Just an observation: defprotocol supports doc strings on its method declarations. We could/should be using them rather than ac hoc comment blocks.
| (if (some? new-converted) | ||
| (mem/add-accum-filtered! memory node (:matches token) bindings | ||
| (into (vec previous-filtered-facts) new-filtered-facts)) | ||
| (mem/remove-accum-filtered! memory node (:matches token) bindings)) |
There was a problem hiding this comment.
I don't think I understand this case. There is no nil new-converted. Why does that mean we have no facts passing the accumulator join filter though?
The idea is that this memory only stores filtered facts when the accumulator is able to be propagated?
| (let [stored (mem/get-accum-filtered memory node (:matches token) bindings)] | ||
| (if (not= stored ::mem/no-accum-filtered) | ||
| stored | ||
| (vec (filter-accum-facts node token candidate-facts bindings join-filter-fn accum-condition))))) |
There was a problem hiding this comment.
filter-accum-facts is one of the rare'ish places we use a lazy seq return. This now converts it to eager evaluation. Is there some rationale for this? I'd think that filter-accum-facts could use platform/eager-for instead of for, if we are just going to wrap it in vec here now instead.
It is actually a bit inconsistent in that we still use the lazy variant in the same AccumulateWithJoinFilterNode impl some of the time.
| accum-reduced (mem/get-accum-reduced memory this join-bindings (merge join-bindings fact-bindings))] | ||
| accum-bindings (merge join-bindings fact-bindings) | ||
| accum-reduced (mem/get-accum-reduced memory this join-bindings accum-bindings) | ||
| matches (:matches token) |
There was a problem hiding this comment.
| matches (:matches token) | |
| {:keys [matches]} token |
| ;; The activation methods above keep the filtered facts current with mem/add-accum-filtered!, and read them through stored-or-filtered. | ||
| ;; This lookup uses them first. The key is the :matches of the token that entered this node. | ||
| ;; That token's :matches is the prefix before this node's entry in the descendant token. | ||
| ;; Sessions serialized before the filtered facts were stored have none, so the join filter runs again. |
There was a problem hiding this comment.
How can the join filter run again if there is no join filter compiled to run? This comment contradicts itself for read-only sessions.
| :let [previous-facts (stored-or-filtered node memory token previous-candidates bindings join-filter-fn accum-condition) | ||
|
|
||
| new-facts (filter-accum-facts node token new-candidates bindings join-filter-fn accum-condition)] | ||
| ;; Equal facts give the same join filter result, so removing the retracted facts from the filtered facts |
There was a problem hiding this comment.
This is pretty nice. Looks like something that could have been done regardless of having the new storage here. Running the filter over again on the new candidates after right retract is not necessary.
This MR contains two changes: