Sorted coll Fressian handler fixes #228
Conversation
mrrodriguez
force-pushed
the
sorted-coll-durability-fix
branch
from
89e354e
to
c64b612
Compare
|
|
||
| (is (= (-> rb :id-to-node keys set) | ||
| (-> deserialized1 :id-to-node keys set) | ||
| (-> deserialized2 :id-to-node keys set)))))) |
There was a problem hiding this comment.
Perhaps you could factor out the parts of durability-test that are just normal rule operations and assertions from the serialization parts and assert on the former here too? This doesn't seem to do much to validate functional correctness after the second pass through serialization rather than just validating that the second deserialization doesn't cause an exception. Functional correctness depends on the node structure itself; I don't even think the id-to-node map is used in the engine.
There was a problem hiding this comment.
Yeah I just did this to get a quick test. It wasn't meant to be much more than asserting that something structurally coherent can serialize and deserialize. I know the :id-to-node doesn't really prove much of anything in terms of the correctness of a rulebase.
Prior to these changes there were exceptions before even getting this far. I can try to work in a test about inserting facts to these rulebases and showing they all have the same queried results.
|
I noticed #230 while reviewing this, but it looks like we have the same issue before and after these changes. |
|
I'm not very knowledgeable about Fressian, but the write and read handler changes look appropriate. |
|
I'll work in #230 to this I believe. |
mrrodriguez
force-pushed
the
sorted-coll-durability-fix
branch
from
c64b612
to
9965007
Compare
|
I pushed changes to this. More testing was added and clean up of the support of the This also should be addressing #230 which just came as a side-effect of fixing up this function in its various use-cases. |
| (@#'com/create-get-alphas-fn type ancestors rulebase)) | ||
| (def ^:private create-get-alphas-fn @#'com/create-get-alphas-fn) | ||
|
|
||
| (defn- opts->get-alphas-fn [rulebase opts] |
There was a problem hiding this comment.
I don't think this is quite right either, and is unnecessarily complex. How about
(defn opts->get-alphas-fn [rulebase opts]
(let [fact-type-fn (or (:fact-type-fn opts) type)
ancestors-fn (or (:ancestors-fn opts) ancestors)]
(create-get-alphas-fn fact-type-fn ancestors-fn rulebase)))
Currently this doesn't allow the user to specify the ancestors-fn without overriding the fact-type-fn as well. More generally, I don't think we need to intermingle the "default or override" selection logic with the call to actually create the get-alphas-fn; doing so just creates unnecessary complexity.
There was a problem hiding this comment.
Yeah, you're right. I think it is unlikely to override one and not the other, but obviously possible.
Either way, the implementation like what you are saying here makes more sense. I just didn't put enough thought into this part since it wasn't a usage I was concerned with.
I'll push the change soon.
There was a problem hiding this comment.
They are pushed now.
| init-qresults (:query-results (session-test s)) | ||
| restored-qresults1 (:query-results (session-test restored1)) | ||
| restored-qresults2 (:query-results (session-test restored2))] | ||
|
|
There was a problem hiding this comment.
So test-durability-fressian-serde tests the functional correctness via queries and assertions on their results, and this just does the same thing as test-durability-fressian-serde, adds another iteration of SerDe, and validates that the query results didn't change, correct? That should work to test this.
There was a problem hiding this comment.
Yes. Obviously it doesn't test total correctness of the session or anything, but gives reasonable confidence since the results end up coming out the same after inserting the test facts, firing, and querying.
The working memory style test uses this same test, but takes it a step further and checks the object-identity found within the query results etc.
- Improve clara.rules.durability/assemble-restored-session to -- properly support session options and -- optionanlly be called with no init memory
mrrodriguez
force-pushed
the
sorted-coll-durability-fix
branch
from
9965007
to
2ec6c61
Compare
|
+1 |
1 similar comment
|
+1 |
|
Merged! |
The sorted collections Fressian SerDe handlers didn't preserve metadata upon deserialization. This means a deserialized sorted collection could potentially fail if it were re-serialized again afterwards. This only applies to sorted collections that have a custom sort comparator fn, which must have its name specified in the metadata under the key
:clara.rules.durability/comparator-name.I added the start of some explicit testing of the behavior of the Fressian handlers in
clara.test-fressianto try to be sure to catch more issues like this in the future.I also added a test in
clara.test-durabilitydemonstrating the use-case that triggered this issue from the Serialize+Deserialize+Re-serialize a rulebase perspective.clara.rules.durability/assemble-restored-sessionhad a 2-arity added as well. This is only used in tests here, but we have experienced places where it'd be helpful for our use-cases as well. If you deserialize a rulebase and want a brand new working memory, rather than having an existing one, it is convenient to have this assembly function that behaves sort of likeclara.rules/mk-session, but with a rulebase given as an argument - along with the same sort of options.