Add retract-fn to accumulator #410
Conversation
|
Be sure to check out the https://github.com/cerner/clara-rules/blob/master/CONTRIBUTING.md too |
sunilgunisetty
force-pushed
the
retract-fn-in-accumulator
branch
from
f3cef77
to
be77e88
Compare
mrrodriguez
requested review from
WilliamParker,
mrrodriguez and
EthanEChristian
| @@ -14,7 +14,7 @@ | |||
| ;; The accumulator is a Rete extension to run an accumulation (such as sum, average, or similar operation) | |||
| ;; over a collection of values passing through the Rete network. This object defines the behavior | |||
| ;; of an accumulator. See the AccumulateNode for the actual node implementation in the network. | |||
| (defrecord Accumulator [initial-value reduce-fn combine-fn convert-return-fn]) | |||
| (defrecord Accumulator [initial-value retract-fn reduce-fn combine-fn convert-return-fn]) | |||
There was a problem hiding this comment.
Adding this may add clarity to someone reading it. Right now we just set the key on the record map via the factory fn map->Accumulator. Having this as a field is not strictly necessary and wouldn't really be of any real performance benefit (this field does not appear to be accessed enough to have significant performance implications).
I think it seems fine to add it here for readability. However, I was wondering if anyone else had any thoughts on if this could cause any sort of breakage anywhere (I don't think so, and I'd hope not).
Also, I think the retract-fn itself is poorly documented. I don't see many details on defining your own accumulators at all in our docs, so that'd be a good place to see improvements.
In particular, it recently reoccurred to me that the retract-fn isn't even always used, even if provided. It is more of an "optimization hint" that may be used if applicable. Examples of it not being used, would be the AccumulateWithJoinFilterNode that reaccumulates from the candidate facts each time.
There was a problem hiding this comment.
Not having the retract-fn in the record seems unclear to me so I agree with adding it. As far as breaking anything, I think we're fine. Since records take additional keys as you say the only way I think we'd break anything is with durability. However, the Fressian record handler just writes the record out as a map with information to call its map-> constructor on deserialization. That said, it is conceivable that someone has created a durability implementation that has optimizations that would be broken by this change - for an idea of the things that could be done see issue 260, but I don't think it is likely. I'd suggest just making a note in the changelog that any users with their own durability implementation for the rulebase should look at this PR. @mrrodriguez
| @@ -14,7 +14,7 @@ | |||
| ;; The accumulator is a Rete extension to run an accumulation (such as sum, average, or similar operation) | |||
| ;; over a collection of values passing through the Rete network. This object defines the behavior | |||
| ;; of an accumulator. See the AccumulateNode for the actual node implementation in the network. | |||
| (defrecord Accumulator [initial-value reduce-fn combine-fn convert-return-fn]) | |||
| (defrecord Accumulator [initial-value retract-fn reduce-fn combine-fn convert-return-fn]) | |||
There was a problem hiding this comment.
Not having the retract-fn in the record seems unclear to me so I agree with adding it. As far as breaking anything, I think we're fine. Since records take additional keys as you say the only way I think we'd break anything is with durability. However, the Fressian record handler just writes the record out as a map with information to call its map-> constructor on deserialization. That said, it is conceivable that someone has created a durability implementation that has optimizations that would be broken by this change - for an idea of the things that could be done see issue 260, but I don't think it is likely. I'd suggest just making a note in the changelog that any users with their own durability implementation for the rulebase should look at this PR. @mrrodriguez
There was a problem hiding this comment.
Other than the comment made by @WilliamParker, in regards to the changelog, i think this looks fine
|
I merged this since it seems we're in agreement that the change is OK. I then made the changelog entry in 22d2df4 and then corrected my mistake of what version we're on in 9cbcc8a#diff-4ac32a78649ca5bdd8e0ba38b7006a1e cc @mrrodriguez @EthanEChristian @sunilgunisetty thanks for noticing this and submitting the PR. |
Its not obvious
retract-fnis part of accumulator. Adding it make it more explicit.