[DO-NOT-MERGE][DROOLS-7635] ansible-rulebook : Raise an error when a condition comp… #112
Conversation
…ares incompatible types - WIP: This doesn't support alpha index. Some tests fail
| static RulebookOperator newEqual() { | ||
| return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.EQUAL)); | ||
| } |
There was a problem hiding this comment.
I introduced RulebookConstraintOperator instead of Index.ConstraintType. RulebookConstraintOperator is not enum/singleton, because each instance holds CondtionContext which is used for error logging.
Problem: This doesn't create alpha index, because PrototypeDSL requires Index.ConstraintType, but we cannot extends Index.ConstraintType because it's enum.
https://github.com/apache/incubator-kie-drools/blob/main/drools-model/drools-model-prototype/src/main/java/org/drools/model/prototype/PrototypeDSL.java#L139
Indeed, AlphaTest fails.
Other concerns:
- This approach also breaks node sharing? (I haven't looked into it yet)
- This requires lots of memory? We may reduce the size of
CondtionContext.
There was a problem hiding this comment.
This is a quite hard problem and I believe that this is something that we cannot fix in a reasonable way (and without losing features that for sure we don't want to lose, like indexing and node sharing) without also changing the Drools code base. In essence I believe that this will require also some changes in Drools, that's practically unavoidable, so let's do it.
Regarding the way on how to proceed to fix this, the main problem now is that for those indexed constraint the existing enumeration implies a tight coupling in Drools between the kind of the constraint (this is an equal, so we have some sort of index based on that fact that it is an equal) and the comparison function using to evaluate that constraint. As a first thing we need in some way to be able to avoid this coupling (at least in the prototypes part) and have a way to say "this is an equal, so it performs as an equal for indexing and node sharing matters, but the I want to evaluate it with this custom predicate that I'm providing".
Please let me know if my suggestion is clear or you have any remarks or see any other showstopper.
There was a problem hiding this comment.
Thank you for the suggestion. Thinking...
- If we store the error message information (rulesetname, rulename, condition expression) in a custom predicate, when node sharing happens, the information will be lost, so we will need to merge the information.
- Alpha index skips unmatched AlphaNodes and jumps to the right AlphaNode. That means, when an event doesn't match any AlphaNode because of type mismatch , the custom predicate would not be executed. This is actually the case that users want to read the error message ... "No rule fires, I don't know if constraints are wrong". I think we would need to add the check logic in
CompositeObjectSinkAdapter.propagateAssertObject.
|
Replaced by |
…ares incompatible types