Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO-NOT-MERGE][DROOLS-7635] ansible-rulebook : Raise an error when a condition comp… #112

Closed
wants to merge 1 commit into from
Closed

[DO-NOT-MERGE][DROOLS-7635] ansible-rulebook : Raise an error when a condition comp… #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...c/main/java/org/drools/ansible/rulebook/integration/api/domain/RuleGenerationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class RuleGenerationContext {

private final StackedContext<String, PrototypeDSL.PrototypePatternDef> patterns = new StackedContext<>();

private String ruleSetName;

private String ruleName;

private int bindingsCounter = 0;
Expand Down Expand Up @@ -209,11 +211,20 @@ public void setRuleName(String ruleName) {
this.ruleName = ruleName;
}

public String getRuleSetName() {
return ruleSetName;
}

public void setRuleSetName(String ruleSetName) {
this.ruleSetName = ruleSetName;
}

List<Rule> toExecModelRules(RulesSet rulesSet, org.drools.ansible.rulebook.integration.api.domain.Rule ansibleRule, RulesExecutionController rulesExecutionController, AtomicInteger ruleCounter) {
updateContextFromRule(ansibleRule);
if (getRuleName() == null) {
setRuleName("r_" + ruleCounter.getAndIncrement());
}
setRuleSetName(rulesSet.getName());

List<org.drools.model.Rule> rules = createRules(rulesExecutionController);
if (hasTemporalConstraint(ansibleRule)) {
Expand Down
13 changes: 7 additions & 6 deletions ...main/java/org/drools/ansible/rulebook/integration/api/domain/conditions/MapCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ private ParsedCondition parseSingle(RuleGenerationContext ruleContext, Map.Entry

private ParsedCondition parseExpression(RuleGenerationContext ruleContext, String expressionName, Map<?, ?> expression) {
RulebookOperator operator = decodeOperation(ruleContext, expressionName);
operator.setConditionContext(ruleContext, expression);

if (operator instanceof ConditionFactory) {
if (expression.get("lhs") != null) {
Expand Down Expand Up @@ -261,17 +262,17 @@ private static void throwExceptionIfCannotInverse(RulebookOperator operator, Rul
private static RulebookOperator decodeOperation(RuleGenerationContext ruleContext, String expressionName) {
switch (expressionName) {
case "EqualsExpression":
return RulebookOperator.EQUAL;
return RulebookOperator.newEqual();
case "NotEqualsExpression":
return RulebookOperator.NOT_EQUAL;
return RulebookOperator.newNotEqual();
case "GreaterThanExpression":
return RulebookOperator.GREATER_THAN;
return RulebookOperator.newGreaterThan();
case "GreaterThanOrEqualToExpression":
return RulebookOperator.GREATER_OR_EQUAL;
return RulebookOperator.newGreaterOrEqual();
case "LessThanExpression":
return RulebookOperator.LESS_THAN;
return RulebookOperator.newLessThan();
case "LessThanOrEqualToExpression":
return RulebookOperator.LESS_OR_EQUAL;
return RulebookOperator.newLessOrEqual();
case ExistsField.EXPRESSION_NAME:
return ExistsField.INSTANCE;
case ExistsField.NEGATED_EXPRESSION_NAME:
Expand Down
179 changes: 179 additions & 0 deletions ...rools/ansible/rulebook/integration/api/domain/constraints/RulebookConstraintOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
package org.drools.ansible.rulebook.integration.api.domain.constraints;

import java.util.Map;
import java.util.function.BiPredicate;

import org.drools.ansible.rulebook.integration.api.domain.RuleGenerationContext;
import org.drools.model.ConstraintOperator;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static org.drools.model.util.OperatorUtils.areEqual;
import static org.drools.model.util.OperatorUtils.compare;

public class RulebookConstraintOperator implements ConstraintOperator {

enum RulebookConstraintOperatorType {
EQUAL,
NOT_EQUAL,
GREATER_THAN,
GREATER_OR_EQUAL,
LESS_THAN,
LESS_OR_EQUAL,
UNKNOWN;
}

private static final Logger LOG = LoggerFactory.getLogger(RulebookConstraintOperator.class);

private RulebookConstraintOperatorType type;
private ConditionContext conditionContext;
private boolean typeCheckLogged = false;

public RulebookConstraintOperator(RulebookConstraintOperatorType type) {
this.type = type;
}

public void setConditionContext(RuleGenerationContext ruleContext, Map<?, ?> expression) {
this.conditionContext = new ConditionContext(ruleContext.getRuleSetName(), ruleContext.getRuleName(), expression.toString());
}

public RulebookConstraintOperator negate() {
switch (this.type) {
case EQUAL:
this.type = RulebookConstraintOperatorType.NOT_EQUAL;
return this;
case NOT_EQUAL:
this.type = RulebookConstraintOperatorType.EQUAL;
return this;
case GREATER_THAN:
this.type = RulebookConstraintOperatorType.LESS_OR_EQUAL;
return this;
case GREATER_OR_EQUAL:
this.type = RulebookConstraintOperatorType.LESS_THAN;
return this;
case LESS_OR_EQUAL:
this.type = RulebookConstraintOperatorType.GREATER_THAN;
return this;
case LESS_THAN:
this.type = RulebookConstraintOperatorType.GREATER_OR_EQUAL;
return this;
}
this.type = RulebookConstraintOperatorType.UNKNOWN;
return this;
}

public boolean canInverse() {
switch (this.type) {
case EQUAL:
case NOT_EQUAL:
case GREATER_THAN:
case GREATER_OR_EQUAL:
case LESS_THAN:
case LESS_OR_EQUAL:
return true;
default:
return false;
}
}

@Override
public <T, V> BiPredicate<T, V> asPredicate() {
final BiPredicate<T, V> predicate;
switch (this.type) {
case EQUAL:
predicate = (t, v) -> areEqual(t, v);
break;
case NOT_EQUAL:
predicate = (t, v) -> !areEqual(t, v);
break;
case GREATER_THAN:
predicate = (t,v) -> t != null && compare(t, v) > 0;
break;
case GREATER_OR_EQUAL:
predicate = (t,v) -> t != null && compare(t, v) >= 0;
break;
case LESS_THAN:
predicate = (t,v) -> t != null && compare(t, v) < 0;
break;
case LESS_OR_EQUAL:
predicate = (t,v) -> t != null && compare(t, v) <= 0;
break;
default:
throw new UnsupportedOperationException("Cannot convert " + this + " into a predicate");
}
return (t, v) -> predicateWithTypeCheck(t, v, predicate);
}

private <T, V> boolean predicateWithTypeCheck(T t, V v, BiPredicate<T, V> predicate) {
if (t == null
|| v == null
|| t instanceof Number && v instanceof Number
|| t.getClass() == v.getClass()) {
return predicate.test(t, v);
} else {
if (!typeCheckLogged) {
// TODO: rewrite the message to be python friendly
LOG.error("Cannot compare values of different types: {} and {}. RuleSet: {}. RuleName: {}. Condition: {}",
t.getClass(), v.getClass(), conditionContext.getRuleSet(), conditionContext.getRuleName(), conditionContext.getConditionExpression());
typeCheckLogged = true; // Log only once per constraint
}
return false; // Different types are never equal
}
}

public RulebookConstraintOperator inverse() {
switch (this.type) {
case GREATER_THAN:
this.type = RulebookConstraintOperatorType.LESS_THAN;
return this;
case GREATER_OR_EQUAL:
this.type = RulebookConstraintOperatorType.LESS_OR_EQUAL;
return this;
case LESS_THAN:
this.type = RulebookConstraintOperatorType.GREATER_THAN;
return this;
case LESS_OR_EQUAL:
this.type = RulebookConstraintOperatorType.GREATER_OR_EQUAL;
return this;
default:
return this;
}
}

public boolean isComparison() {
return isAscending() || isDescending();
}

public boolean isAscending() {
return this.type == RulebookConstraintOperatorType.GREATER_THAN || this.type == RulebookConstraintOperatorType.GREATER_OR_EQUAL;
}

public boolean isDescending() {
return this.type == RulebookConstraintOperatorType.LESS_THAN || this.type == RulebookConstraintOperatorType.LESS_OR_EQUAL;
}

private class ConditionContext {

private String ruleSet;
private String ruleName;
private String conditionExpression;

public ConditionContext(String ruleSet, String ruleName, String conditionExpression) {
this.ruleSet = ruleSet;
this.ruleName = ruleName;
this.conditionExpression = conditionExpression;
}

public String getRuleSet() {
return ruleSet;
}

public String getRuleName() {
return ruleName;
}

public String getConditionExpression() {
return conditionExpression;
}
}
}
49 changes: 40 additions & 9 deletions ...java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.drools.ansible.rulebook.integration.api.domain.constraints;

import org.drools.ansible.rulebook.integration.api.domain.RuleGenerationContext;
import org.drools.model.ConstraintOperator;
import org.drools.model.Index;

import java.util.Map;
import java.util.function.BiPredicate;

public interface RulebookOperator extends ConstraintOperator {
Expand All @@ -19,17 +20,43 @@ default ConstraintOperator asConstraintOperator() {
return this;
}

RulebookOperator EQUAL = new OperatorWrapper(Index.ConstraintType.EQUAL);
RulebookOperator NOT_EQUAL = new OperatorWrapper(Index.ConstraintType.NOT_EQUAL);
RulebookOperator GREATER_THAN = new OperatorWrapper(Index.ConstraintType.GREATER_THAN);
RulebookOperator GREATER_OR_EQUAL = new OperatorWrapper(Index.ConstraintType.GREATER_OR_EQUAL);
RulebookOperator LESS_THAN = new OperatorWrapper(Index.ConstraintType.LESS_THAN);
RulebookOperator LESS_OR_EQUAL = new OperatorWrapper(Index.ConstraintType.LESS_OR_EQUAL);
static RulebookOperator newEqual() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.EQUAL));
}
Comment on lines +23 to +25
Copy link
Collaborator Author

@tkobayas tkobayas Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@tkobayas tkobayas Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


static RulebookOperator newNotEqual() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.NOT_EQUAL));
}

static RulebookOperator newGreaterThan() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.GREATER_THAN));
}

static RulebookOperator newGreaterOrEqual() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.GREATER_OR_EQUAL));
}

static RulebookOperator newLessThan() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.LESS_THAN));
}

static RulebookOperator newLessOrEqual() {
return new OperatorWrapper(new RulebookConstraintOperator(RulebookConstraintOperator.RulebookConstraintOperatorType.LESS_OR_EQUAL));
}

default void setConditionContext(RuleGenerationContext ruleContext, Map<?, ?> expression) {
if (this instanceof OperatorWrapper operatorWrapper) {
operatorWrapper.setConditionContext(ruleContext, expression);
} else {
// do nothing
return;
}
}

class OperatorWrapper implements RulebookOperator {
private final Index.ConstraintType delegate;
private final RulebookConstraintOperator delegate;

public OperatorWrapper(Index.ConstraintType delegate) {
public OperatorWrapper(RulebookConstraintOperator delegate) {
this.delegate = delegate;
}

Expand All @@ -52,5 +79,9 @@ public RulebookOperator inverse() {
public ConstraintOperator asConstraintOperator() {
return delegate;
}

public void setConditionContext(RuleGenerationContext ruleContext, Map<?, ?> expression) {
delegate.setConditionContext(ruleContext, expression);
}
}
}
Loading
Loading