You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "Rikkola (via GitHub)" <gi...@apache.org> on 2026/10/02 16:52:08 UTC
[PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Rikkola opened a new pull request, #7160:
URL: https://github.com/apache/incubator-kie/pull/7160
Key features:
- not()/nor() absence guard folded into following positive step
- VetoSignalProcessor for continuous absence guard
- nor()/xor()/xnor() DSL factories and runtime folds
- xor() sugar: sequence(xor(a,b), c) == sequence(and(xor(a,b), c))
- not() resets sequence instead of terminating it (full-reset semantics)
- Trailing not()/nor() without completeWithin rejected at canonical-model level
Closes https://github.com/apache/incubator-kie/issues/7081
Assisted by IBM Bob
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie#7081] sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6516366961
Thank you @Rikkola for the update. Here are the AI review results for the latest changes. You might want to address the issues in separate PRs, but I'll leave that up to you.
---
- **Fixed:** the activation-time veto. `validateActivationMatching()` now rejects a `oneOf` with two or more children that already match at activation, including when it is wrapped in `and(...)` or the matching child is an `or(...)`. A `oneOf` with exactly one such child is still accepted and works.
- **Fixed:** both Copilot findings. A stale `other` no longer completes `sequence(s, or(noneOrAll(a, b), other), ack)`, and `and(oneOf(a, b))` now vetoes on `a, b`.
- **OK:** moving the `AlphaAdapter` filter-skip bug to a separate PR. Could you link the issue or PR here once it exists?
- **OK:** the `oneOf`-under-`or` behaviour is now documented and pinned by tests. See item 1, though. It only holds when the next trigger sits inside the same `and(...)`.
### 1. [Major] A guard-only `or(...)` step is not folded into the following trigger
`hasPositiveTrigger()` treats an `or` as positive only if every branch is positive, so `or(x, noneOrAll(a, b))` and `or(oneOf(a, b), c)` count as non-positive. But `rewriteRevertingGateSteps()` folds only bare `oneOf` / `noneOrAll`, and `flattenNonPositiveAndSteps()` flattens only `and`. Such an `or` therefore stays a standalone step, and the step completes as soon as any one branch matches:
- A `noneOrAll` branch matches at activation, so the whole step is skipped and the guard is silently ignored.
- A `oneOf` branch completes the step on its first match, so a later over-match is never seen. That contradicts the new `oneOf()` javadoc, which says the veto propagates past `or()`.
So the same guard behaves differently depending on whether the trigger is written inside `and(..., d)` or as the next step. (`ev(name)` is `pattern(declarationOf(Event.class)).expr("is-" + name, e -> e.getName().equals(name))`.)
```text
sequence(s, oneOf(a, b), d) s,a,b,d -> 0 firings
sequence(s, or(oneOf(a, b), oneOf(c, e)), d) s,a,b,d -> 1 firing (expected 0)
sequence(s, and(or(oneOf(a, b), c), d)) s,a,c,b,d -> 0 firings (as documented)
sequence(s, or(oneOf(a, b), c), d) s,a,c,b,d -> 1 firing (expected 0)
sequence(s, and(or(x, noneOrAll(a, b)), d)) s,a,d -> 0 firings
sequence(s, or(x, noneOrAll(a, b)), d) s,a,d -> 1 firing (expected 0)
```
This is the same class of problem as Copilot's `and(oneOf(a, b))` finding, but only `and` was fixed. One option is to generalize the bare-`oneOf` rule: fold every step that is neither positive nor `absent()` into the following positive step. That would probably make `flattenNonPositiveAndSteps()` unnecessary too. The other option is to reject these shapes at intermediate positions.
### 2. [Minor] Existing tests from `main` are deleted
The PR deletes `SequenceReviewProbeTest.java` (added in #6909) and `PatternDSLSequenceConsequenceTest.twoSequencesInOneRuleMultipleAnchorsBothFire`. Three of the probe tests were moved to `PatternDSLSequenceEdgeCaseTest`. These four were not moved anywhere: `sameTypeInTwoSteps`, `stepWithTwoConstraintsIsUnsatisfiable`, `stepWithTwoConstraintsBothSatisfiedFires`, and `joinAfterSequenceHasCorrectTupleWidth`. I restored the deleted tests temporarily and ran them against this PR. All of them pass, except `stepWithNoConstraintThrowsDescriptiveError`, whose behaviour this PR changes on purpose. Could you keep them, e.g. by moving them into `PatternDSLSequenceEdgeCaseTest`?
### 3. Small notes
- **Unconstrained steps:** a step without `expr()` now matches any fact of its type through `TrueConstraint`, instead of failing with the error added in #6909. I checked that facts of other types still don't satisfy it. Since this changes v1 behaviour independently of the new API, it may be worth mentioning in the PR description.
- **Same-fact veto (open from last round):** in `sequence(s, absent(s), d)` with arrivals s, s, d, should the vetoing `s` also start a new attempt? It still fires 0 times. Today the result depends on the filter-skip bug and the filter order, so the fix in the separate PR may change it. It would be good to decide the intended behaviour and pin it with a test.
- **Nested `absent()`:** `and(absent(x), d)` and `oneOf(absent(x), a)` still throw `UnsupportedOperationException` ("does not yet support condition type NOT as a composite step gate") when the KieBase is built, while the other shape errors are `IllegalArgumentException`s from `rule.build()`. The message is clear, so this is optional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4193213511
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGate.java:
##########
@@ -97,19 +118,24 @@ public void resetPrior(SequenceMemory memory, ValueResolver valueResolver) {
gate.reset(memory, valueResolver);
}
- memory.resetLogicGateMemory(gateIndex, valueResolver);
+ if (!statusCanRevert) {
+ memory.resetLogicGateMemory(gateIndex, valueResolver);
+ }
}
public void reset(SequenceMemory memory, ValueResolver valueResolver) {
resetPrior(memory, valueResolver);
output.reset(memory, valueResolver);
}
- public void activate(SequenceMemory memory) {
+ public void activate(SequenceMemory memory, ValueResolver valueResolver) {
for (int i = 0; i < filterIndexes.length; i++) {
memory.activateSignalAdapter(filterIndexes[i], this, signalAdapterIndexes[i], i + 1); // bit indexes start at 1
}
-
+ if (predicate.test(0L, allMatched)) {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4193218742
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGateOutputSignalProcessor.java:
##########
@@ -46,4 +46,10 @@ protected void reset(SequenceMemory memory, ValueResolver valueResolver) {
// by each downstream LogicGate resetting itself via resetPrior(). There is no
// state held here that needs clearing.
}
+
+ public void clearParentBit(SequenceMemory memory) {
+ long[] gateMemory = memory.getLogicGateMemory();
+ gateMemory[gate.getGateIndex()] = gateMemory[gate.getGateIndex()] & ~(1L << (index - 1));
+ memory.setLogicGateMatched(gate.getGateIndex(), false);
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie#7081] sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6472132885
Ok. I should have chopped this into even smaller bits. I will keep that in mind with the rest of the work.
For @tkobayas
1. I added a blocker that prevents this situation. It is impossible to get working right, but I have another change coming that can cover it.
2. True it is pre-existing. I made a fix, but will make a new PR.
3. I pinned the current behaviour with the test:
```
oneOfUnderOrUnderAndHappyPathExactlyOneAndPositiveFires
oneOfUnderOrUnderAndOrderACBDXorOvermatchVetoResets
oneOfUnderOrUnderAndOrderCABDXorOvermatchVetoResets
oneOfUnderOrUnderAndOrderABCDXorOvermatchBeforeOrSatisfiedVetoResets
```
Nit was actually never reached, so it was removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6219248572
Swapping back to draft. Had to do some changes to the solution and some renaming due to the issues that came up in reviews.
Let's see what co-pilot and test build has to say about the new version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie#7081] sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4409610839
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -194,6 +219,235 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
throw new UnsupportedOperationException( "Unknown " + ruleItem );
}
+ private static boolean hasPositiveTrigger(SequenceStep step) {
+ if (step instanceof ExistentialExprViewItem) {
+ ExistentialExprViewItem ex = (ExistentialExprViewItem) step;
+ if (ex.getType() == Condition.Type.NOT) {
+ return false;
+ }
+ return true;
+ }
+ if (step instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) step;
+ Condition.Type type = comb.getType();
+ if (type == Condition.Type.NOT || type == Condition.Type.NOR
+ || type == Condition.Type.XOR || type == Condition.Type.XNOR) {
+ return false;
+ }
+ if (type == Condition.Type.AND) {
+ // In an AND composite, at least one child must be a positive trigger
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (expr instanceof SequenceStep) {
+ if (hasPositiveTrigger((SequenceStep) expr)) {
+ return true;
+ }
+ } else {
+ return true;
+ }
+ }
+ return false;
+ }
+ if (type == Condition.Type.OR) {
+ // In an OR composite, all branches must have a positive trigger
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (expr instanceof SequenceStep) {
+ if (!hasPositiveTrigger((SequenceStep) expr)) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+ }
+ return true;
+ }
+
+ private static boolean isAbsenceStep(SequenceStep step) {
+ if (step instanceof ExistentialExprViewItem) {
+ return ((ExistentialExprViewItem) step).getType() == Condition.Type.NOT;
+ }
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type type = ((CombinedExprViewItem) step).getType();
+ return type == Condition.Type.NOT || type == Condition.Type.NOR;
+ }
+ throw new IllegalStateException("Unexpected SequenceStep type: " + step.getClass());
+ }
+
+ private static void validateActivationMatching(org.drools.model.view.ViewItem item) {
+ if (item instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) item;
+ Condition.Type type = comb.getType();
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ validateActivationMatching(expr);
+ }
+ if (type == Condition.Type.XOR) {
+ int matchedChildren = 0;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matchedChildren++;
+ }
+ }
+ if (matchedChildren >= 2) {
+ throw new IllegalArgumentException(
+ "sequence(): oneOf() has " + matchedChildren + " children that match on step activation (0 inputs), " +
+ "causing an immediate over-match veto upon activation. oneOf() requires exactly one child to match.");
+ }
+ }
+ }
+ }
+
+ private static boolean matchesOnActivation(org.drools.model.view.ViewItem item) {
+ if (item instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) item;
+ Condition.Type type = comb.getType();
+ if (type == Condition.Type.XNOR) {
+ int matched = 0;
+ int total = comb.getExpressions().length;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matched++;
+ }
+ }
+ return matched == 0 || matched == total;
+ }
+ if (type == Condition.Type.XOR) {
+ int matched = 0;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matched++;
+ }
+ }
+ return matched == 1;
+ }
+ if (type == Condition.Type.AND) {
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (!matchesOnActivation(expr)) {
+ return false;
+ }
+ }
+ return comb.getExpressions().length > 0;
+ }
+ if (type == Condition.Type.OR) {
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ return true;
+ }
+ }
+ return false;
+ }
+ if (type == Condition.Type.NOT || type == Condition.Type.NOR) {
+ return false;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Rewrites runs of revertible gate steps (such as {@code oneOf()} / XOR and {@code noneOrAll()} / XNOR)
+ * by folding them together with their immediately following positive trigger into an {@code and(...)}
+ * composite step.
+ * <p>
+ * Standalone XOR/XNOR gates are non-propagating on their own within the sequence pipeline and require
+ * a following positive pattern step to act as the activation trigger. This transformation:
+ * <ul>
+ * <li>Separates and emits any interleaved absence guards ({@code NOT}/{@code NOR}/{@code absent()})
+ * before the folded composite so that compiler accumulation loops build them as independent
+ * veto side-channels.</li>
+ * <li>Folds consecutive XOR/XNOR gate steps and the subsequent positive trigger step into a single
+ * {@link CombinedExprViewItem} of type {@link Condition.Type#AND}.</li>
+ * </ul>
+ *
+ * @param steps the original sequence steps
+ * @return the rewritten sequence steps with folded gate composites
+ */
+ private static SequenceStep[] rewriteRevertingGateSteps(SequenceStep[] steps) {
+ boolean hasXor = false;
+ for (int i = 0; i < steps.length; i++) {
+ SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
Review Comment:
Only bare XOR/XNOR steps are normalized, so `sequence(start, and(oneOf(a,b)), ack)` remains three runtime steps. Matching `a` completes the middle step and unregisters `b`, so `start, a, b, ack` fires instead of vetoing the attempt. Fold guard-only composites into their following positive trigger, or reject them at intermediate positions as well as at the end.
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/steps/LogicCircuitStep.java:
##########
@@ -38,7 +38,7 @@ public LogicCircuit getCircuit() {
public void activate(SequenceMemory sequenceMemory, ValueResolver valueResolver) {
for (LogicGate gate : circuit.getGates()) {
- gate.activate(sequenceMemory);
+ gate.activate(sequenceMemory, valueResolver);
Review Comment:
`gate.activate()` can synchronously advance the sequence, but this loop then activates remaining gates from the consumed step. For `sequence(start, or(noneOrAll(a,b), other), ack)`, activation advances to `ack` and then re-registers `other`, allowing an `other` event to complete the sequence without an acknowledgement. Stop activating the old circuit when the current step changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4193227876
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -194,6 +221,50 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
throw new UnsupportedOperationException( "Unknown " + ruleItem );
}
+ /**
+ * Rewrites bare xor()/xnor() steps in a sequence by folding each such step
+ * with the immediately following positive step into an and(xor(...), next) pair.
+ * <p>
+ * {@code sequence(xor(a, b), c)} is legal sugar for {@code sequence(and(xor(a, b), c))}.
+ * The trailing-XOR check in the caller will catch {@code sequence(xor(a, b))} (no following step).
+ */
+ private static org.drools.model.SequenceStep[] rewriteXorSteps(org.drools.model.SequenceStep[] steps) {
+ boolean hasXor = false;
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
+ hasXor = true;
+ break;
+ }
+ }
+ }
+ if (!hasXor) {
+ return steps;
+ }
+ List<org.drools.model.SequenceStep> result = new ArrayList<>();
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
+ // Wrap: and(xor/xnor(...), nextStep)
+ CombinedExprViewItem wrapped = new CombinedExprViewItem(
+ Condition.Type.AND,
+ new ViewItem[]{(ViewItem) step, (ViewItem) steps[i + 1]});
+ result.add(wrapped);
+ i++; // consume the following step
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4193221485
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/DSL.java:
##########
@@ -555,7 +555,7 @@ public static FixedValueItem expr( String exprId, boolean value ) {
// -- Existential operator --
- public static ExprViewItem not( ViewItemBuilder<?> expression, ViewItemBuilder<?>... expressions) {
+ public static ExistentialExprViewItem not( ViewItemBuilder<?> expression, ViewItemBuilder<?>... expressions) {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie#7081] sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4412817277
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/steps/LogicCircuitStep.java:
##########
@@ -38,7 +38,7 @@ public LogicCircuit getCircuit() {
public void activate(SequenceMemory sequenceMemory, ValueResolver valueResolver) {
for (LogicGate gate : circuit.getGates()) {
- gate.activate(sequenceMemory);
+ gate.activate(sequenceMemory, valueResolver);
Review Comment:
Fixed by breaking out of the gate activation loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6075733957
Thanks for the work on this, @Rikkola . Here are five issues identified by AI.
---
## 1. Infinite loop: veto reset with multiple anchors
**File:** `VetoSignalProcessor.java:89` / `DynamicFilter.java:61-65`
When two left tuples activate `sequence(not(X), D)` and X is inserted, `fireAllRules()` never returns.
`DynamicFilter.assertObject()` iterates the `signalAdapters` linked list. For each adapter it calls `receive()`, which flows into `VetoSignalProcessor.consume()` — that deactivates the current step, resets to step 0, and re-activates step 0 synchronously. Re-activation re-registers the adapter at the **tail** of the same list that is being iterated. With two anchors the two adapters keep chasing each other.
The pre-captured `next` pointer guards against removal during iteration but does not guard against tail-append.
Reproduction:
```java
List<String> results = new ArrayList<>();
Rule rule = rule("multiple-anchors").build(
pattern(declarationOf(String.class))
.expr("anchor", s -> s.startsWith("anchor")),
sequence(not(event("x")), event("d")),
execute(() -> results.add("fired"))
);
KieSession session = KieBaseBuilder.createKieBaseFromModel(
new ModelImpl().addRule(rule)).newKieSession();
session.insert("anchor1");
session.insert("anchor2");
session.fireAllRules();
session.insert(new Event("x"));
session.fireAllRules(); // Expected: returns. Actual: infinite loop.
```
Stack trace from a 2-second watchdog:
```
LogicGate.activate(LogicGate.java:136)
LogicCircuitStep.activate(LogicCircuitStep.java:41)
VetoSignalProcessor.consume(VetoSignalProcessor.java:89)
LogicGate.propagate(LogicGate.java:117)
LogicGate.consume(LogicGate.java:102)
SignalAdapter.receive(SignalAdapter.java:46)
DynamicFilter.assertObject(DynamicFilter.java:63)
```
**Suggested fix:** defer re-subscription until the current event's delivery completes, so newly registered adapters are not visited for the in-flight event.
---
## 2. Binary incompatibility: `DSL.not()` return type change
**File:** `DSL.java:558`
This is the same one as Copilot suggested. I think you haven't fixed it.
The return type of `public static ExprViewItem not(ViewItemBuilder<?>, ViewItemBuilder<?>...)` changed to `ExistentialExprViewItem`. This is source-compatible but **binary-incompatible** — the JVM method descriptor includes the return type, so pre-compiled callers get `NoSuchMethodError`.
This is not limited to sequence users; any existing executable model that calls `DSL.not()` is affected. e.g., a KJAR built by executable-model.
Reproduction — compile this client against the parent commit's DSL, then run it with the PR's DSL on the classpath (without recompiling the client):
```java
import org.drools.model.DSL;
import org.drools.model.view.ExprViewItem;
import org.drools.model.view.ViewItemBuilder;
public class Pr7160BinaryCompatibilityProbe {
public static void main(String[] args) {
ExprViewItem result = DSL.not(
DSL.expr("true", true), new ViewItemBuilder<?>[0]);
System.out.println("Previous API client returned " + result);
}
}
```
Result:
```
java.lang.NoSuchMethodError:
'org.drools.model.view.ExprViewItem org.drools.model.DSL.not(
org.drools.model.view.ViewItemBuilder,
org.drools.model.view.ViewItemBuilder[])'
```
**Suggested fix:** keep the existing method descriptor (`ExprViewItem` return type). Expose the `SequenceStep` capability through a separate method or interface.
---
## 3. Nested XNOR activation evaluates stale zero state
**File:** `LogicGate.java:139`
This is similar to what Copilot suggested, but a little more complex.
```java
if (predicate.test(0L, allMatched)) {
```
`activate()` always evaluates against `0L` (empty input). For a standalone XNOR this is correct — zero inputs means XNOR is satisfied. But in a nested circuit like `and(xnor(xnor(A, B), C), D)`:
1. The inner `xnor(A, B)` activates first and propagates a match bit to the outer XNOR.
2. The outer XNOR then activates but ignores the bit already written to its gate memory — it evaluates `0L` instead of the actual memory.
3. The outer XNOR sees zero inputs and reports "matched," so D alone fires the rule.
Reproduction:
```java
check(
"nested-xnor",
0,
and(
xnor(xnor(event("a"), event("b")), event("c")),
event("d")
),
"d"
);
// nested-xnor expected=0 actual=1
```
**Suggested fix:** read the actual gate memory in `activate()` instead of hard-coding `0L`, so bits propagated by child gates during earlier activation are respected.
---
## 4. Reversion does not re-evaluate the parent gate
**File:** `LogicGateOutputSignalProcessor.java:58-69`
`clearParentBit()` removes the child's contribution bit from the parent and unconditionally sets the parent to unmatched. It does not re-evaluate the parent's predicate after clearing the bit.
For `and(xor(xor(A, B), C), D)` with arrival order A, C, B, D:
1. A matches inner XOR (1 input) — matched. Outer XOR gets 1 input — matched.
2. C matches outer XOR (2 inputs) — unmatched (XOR requires exactly 1).
3. B matches inner XOR (2 inputs) — unmatched, reverts. Outer XOR now has only C — should be matched again.
4. But `clearParentBit` sets the outer XOR to unmatched without re-evaluating. D cannot fire.
Reproduction:
```java
check(
"nested-xor-reversion",
1,
and(
xor(xor(event("a"), event("b")), event("c")),
event("d")
),
"a", "c", "b", "d"
);
// nested-xor-reversion expected=1 actual=0
```
**Suggested fix:** after clearing the bit, re-evaluate the parent's predicate. If the result is a state transition (unmatched -> matched), propagate upward. This needs to be recursive through the circuit.
---
## 5. `resetPrior()` clears parent AND/OR memory, preventing recovery
**File:** `LogicGate.java:120-128`
When a gate propagates a match, `resetPrior()` calls `memory.resetLogicGateMemory(gateIndex, ...)` for non-reverting gates. This clears the AND/OR parent's record of **other** inputs that already matched.
For `and(and(xnor(A, B), C), D)` with arrival order C, A, B, D:
1. XNOR starts matched (zero inputs). C arrives — inner AND has both inputs, propagates, and its memory is reset (C's bit is cleared).
2. A arrives — XNOR becomes unmatched (1 input).
3. B arrives — XNOR is matched again (2 inputs). But the inner AND lost C's bit, so it sees only XNOR and waits for C again.
4. D cannot fire.
Reproduction (case A — AND recovery):
```java
check(
"and-xnor-recovery",
1,
and(
and(xnor(event("a"), event("b")), event("c")),
event("d")
),
"c", "a", "b", "d"
);
// and-xnor-recovery expected=1 actual=0
```
Same pattern breaks OR (case B — OR retention):
```java
check(
"or-reversion",
1,
and(
or(xor(event("a"), event("b")), event("c")),
event("d")
),
"a", "c", "b", "d"
);
// or-reversion expected=1 actual=0
```
**Suggested fix:** for circuits that contain reverting children, the parent gate must retain its input memory across propagation rather than resetting it. `statusCanRevert` on the child alone is not sufficient — the ancestor chain needs to know it may be re-evaluated.
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4323713702
##########
drools-base/src/main/java/org/drools/base/reteoo/DynamicFilter.java:
##########
@@ -56,32 +58,37 @@ public LinkedList<SignalAdapter> getSignalAdapters() {
public void assertObject(final FactHandle factHandle,
final ValueResolver valueResolver) {
+ final int startEpoch = ++this.deliveryEpoch;
+
if (constraint.getRequiredDeclarations().length == 0) {
if (constraint.isAllowed(factHandle, valueResolver)) {
for (SignalAdapter current = signalAdapters.getFirst(); current != null; ) {
+ if (current.getRegistrationEpoch() >= startEpoch) {
+ break;
+ }
SignalAdapter next = current.getNext();
current.receive(valueResolver, factHandle);
current = next;
}
}
} else {
for (SignalAdapter current = signalAdapters.getFirst(); current != null; ) {
+ if (current.getRegistrationEpoch() >= startEpoch) {
+ break;
+ }
SignalAdapter next = current.getNext();
- if (test(factHandle, valueResolver, current.getAnchorTuple())) {
+ if (test(factHandle, current.getAnchorTuple(), valueResolver)) {
current.receive(valueResolver, factHandle);
}
current = next;
}
}
}
- private boolean test(FactHandle factHandle, ValueResolver valueResolver, BaseTuple tuple) {
- if (constraint instanceof BetaConstraint) {
- BetaConstraint<ContextEntry> betaConstraint = (BetaConstraint<ContextEntry>) constraint;
- ContextEntry ctx = betaConstraint.createContext();
- ctx.updateFromTuple(valueResolver, tuple);
- return betaConstraint.isAllowedCachedLeft(ctx, factHandle);
- }
- throw new IllegalStateException("test() called with a non-beta constraint: " + constraint.getClass());
+ private boolean test(final FactHandle factHandle, final BaseTuple anchorTuple, final ValueResolver valueResolver) {
+ BetaConstraint<ContextEntry> betaConstraint = (BetaConstraint<ContextEntry>) constraint;
+ ContextEntry ctx = betaConstraint.createContext();
+ ctx.updateFromTuple(valueResolver, anchorTuple);
+ return betaConstraint.isAllowedCachedLeft(ctx, factHandle);
}
Review Comment:
This method now unconditionally casts `constraint` to `BetaConstraint`, whereas the previous implementation guarded the cast and produced a clear exception if a non-beta constraint ever reached this code path. If `constraint` can be non-beta with `getRequiredDeclarations().length != 0` (or if that assumption changes), this will fail with a `ClassCastException` which is harder to diagnose and may occur in production. Safer options: restore the `instanceof BetaConstraint` guard (throwing an explanatory `IllegalStateException`), or narrow the field type so a non-beta constraint is impossible at compile time.
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/VetoSignalProcessor.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.drools.base.reteoo.sequencing.signalprocessors;
+
+import org.drools.base.base.ValueResolver;
+import org.drools.base.reteoo.sequencing.Sequence.SequenceMemory;
+
+/**
+ * Output processor for the absence leaf gate inside a folded composite step.
+ *
+ * <p>When the absence pattern fires — i.e. a matching fact is inserted while the step is
+ * active — this processor resets the sequence: it deactivates the current step, resets
+ * the step pointer to 0, and re-activates step 0 so the sequence starts listening again
+ * from the beginning.
+ *
+ * <h3>How it is wired</h3>
+ * <p>Inside a {@code sequence()}, any {@code not()} or {@code nor()} condition is compiled
+ * into an <em>absence leaf gate</em> whose output is set to this processor. The absence
+ * leaf gate is folded together with the following positive step into one
+ * {@link LogicCircuit}, so both run inside the same step. The absence side-channel is
+ * independent: it is not wired into the positive gate's parent AND composite.
+ * Example topology for {@code sequence(A, not(X), B)}:
+ * <pre>
+ * step 0 -> [gate: A matches] -> TerminatingSignalProcessor -> advance to step 1
+ * step 1 -> [gate: X arrives] -> VetoSignalProcessor -> reset to step 0
+ * \ [gate: B matches] -> TerminatingSignalProcessor -> advance to step 2
+ * </pre>
+ *
+ * <h3>Full-reset semantics (why always step 0)</h3>
+ * <p>The reset always targets step 0, regardless of which step in the sequence contains
+ * the veto gate. In a multi-not sequence such as
+ * {@code sequence(A, not(X), B, not(Y), C)}, a veto while waiting at step 2 discards
+ * all earlier progress and restarts from step 0. This is intentional, for three reasons:
+ * <ol>
+ * <li><b>A not-guard invalidates the whole attempt.</b> {@code not(X)} means "X must
+ * not appear during this attempt." When X does appear, the entire attempt is
+ * invalid — there is no partially valid prefix to retain.</li>
+ * <li><b>Partial reset is observationally ambiguous.</b> Resetting to the step
+ * preceding the veto gate would imply that facts matched during the previous attempt
+ * still count toward the new attempt. Whether a previously matched fact is still
+ * the <em>right</em> fact is undefined; retaining bindings across a veto would
+ * require a dedicated binding-retention mechanism that does not exist.</li>
+ * <li><b>Consistency with CEP restart semantics.</b> Drools temporal constraints
+ * ({@code after}, {@code before}) restart pattern matching from scratch when a match
+ * is invalidated. Partial restart from a mid-sequence checkpoint would introduce a
+ * new restart model not justified by any current use case.</li>
+ * </ol>
+ *
+ * <h3>Singleton design</h3>
+ * <p>This class is a stateless singleton ({@link #get()}). The inability to encode a
+ * target step index is a deliberate design forcing function: if partial reset were ever
+ * needed, a distinct processor type (e.g. {@code PartialResetSignalProcessor(int targetStep)})
+ * would need to be introduced as an explicit API change requiring its own design review.
+ * The {@link #consume(int, SequenceMemory, ValueResolver)} overload (used by gates that
+ * report back to a parent composite) is therefore not supported and throws
+ * {@link UnsupportedOperationException}.
+ */
+public class VetoSignalProcessor extends SignalProcessor {
+
+ private static final VetoSignalProcessor INSTANCE = new VetoSignalProcessor();
+
+ private VetoSignalProcessor() {}
+
+ public static VetoSignalProcessor get() {
+ return INSTANCE;
+ }
+
+ @Override
+ public void consume(SequenceMemory memory, ValueResolver valueResolver) {
+ int step = memory.getStep();
+ memory.getSequence().getSteps()[step].deactivate(memory, valueResolver);
+ memory.setStep(0);
+ memory.getSequence().getSteps()[0].activate(memory, valueResolver);
+ }
+
+ @Override
+ public void consume(int signalBitIndex, SequenceMemory memory, ValueResolver valueResolver) {
+ throw new UnsupportedOperationException();
+ }
Review Comment:
Throwing `UnsupportedOperationException` without a message makes debugging harder when this overload is accidentally invoked. Consider adding a descriptive message (e.g., that `VetoSignalProcessor` only supports `consume(SequenceMemory, ValueResolver)` because it is not a parent-bit consumer).
##########
drools-base/src/main/java/org/drools/base/reteoo/DynamicFilter.java:
##########
@@ -56,32 +58,37 @@ public LinkedList<SignalAdapter> getSignalAdapters() {
public void assertObject(final FactHandle factHandle,
final ValueResolver valueResolver) {
+ final int startEpoch = ++this.deliveryEpoch;
+
if (constraint.getRequiredDeclarations().length == 0) {
if (constraint.isAllowed(factHandle, valueResolver)) {
for (SignalAdapter current = signalAdapters.getFirst(); current != null; ) {
+ if (current.getRegistrationEpoch() >= startEpoch) {
+ break;
+ }
SignalAdapter next = current.getNext();
current.receive(valueResolver, factHandle);
current = next;
}
}
} else {
for (SignalAdapter current = signalAdapters.getFirst(); current != null; ) {
+ if (current.getRegistrationEpoch() >= startEpoch) {
+ break;
+ }
SignalAdapter next = current.getNext();
- if (test(factHandle, valueResolver, current.getAnchorTuple())) {
+ if (test(factHandle, current.getAnchorTuple(), valueResolver)) {
current.receive(valueResolver, factHandle);
}
current = next;
}
}
Review Comment:
The early `break` assumes `signalAdapters` is ordered by increasing `registrationEpoch` (i.e., adapters registered during this delivery are guaranteed to appear after all pre-existing adapters). If the list ordering ever changes (e.g., insertion at head, concurrent modifications that re-link nodes), this `break` can skip delivering to older adapters that happen to appear after a newly-registered node. To make this robust, consider using `continue` to skip only the newly-registered adapter(s), or explicitly documenting/enforcing the list’s append-only ordering with a clear invariant (and/or assertion) in `addSignalAdapter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6359154326
Failed tests
`BusinessRuleTaskIT.testBusinessRuleTaskWithIOExpression`
`CallActivityTaskIT.testCallActivityTaskWithExpressionsForIO`
`CallActivityTaskIT.testCallActivityTaskWithExpressionsForIONested`
`TravelTest.testTravelVisaRequired`
seemed to be caused by https://github.com/apache/incubator-kie/pull/7199#issuecomment-6359095595
Ignore them for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6354794700
CI failed because of https://github.com/apache/incubator-kie/issues/7226 . In order to take an effect of https://github.com/apache/incubator-kie/pull/7227 fix , I'm going to close+reopen this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas closed pull request #7160: feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics
URL: https://github.com/apache/incubator-kie/pull/7160
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4167963447
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGate.java:
##########
@@ -97,19 +118,24 @@ public void resetPrior(SequenceMemory memory, ValueResolver valueResolver) {
gate.reset(memory, valueResolver);
}
- memory.resetLogicGateMemory(gateIndex, valueResolver);
+ if (!statusCanRevert) {
+ memory.resetLogicGateMemory(gateIndex, valueResolver);
+ }
}
public void reset(SequenceMemory memory, ValueResolver valueResolver) {
resetPrior(memory, valueResolver);
output.reset(memory, valueResolver);
}
- public void activate(SequenceMemory memory) {
+ public void activate(SequenceMemory memory, ValueResolver valueResolver) {
for (int i = 0; i < filterIndexes.length; i++) {
memory.activateSignalAdapter(filterIndexes[i], this, signalAdapterIndexes[i], i + 1); // bit indexes start at 1
}
-
+ if (predicate.test(0L, allMatched)) {
Review Comment:
Activation evaluates the gate as if no child had matched, but child gates are activated before their parents and zero-state gates can already signal the parent. For example, in `xnor(xnor(a, b), c)` the inner XNOR sets one outer input bit; the outer activation still evaluates `0` and incorrectly propagates a match. Evaluate the gate's actual memory so nested XNOR state is respected.
##########
drools-base/src/main/java/org/drools/base/reteoo/sequencing/signalprocessors/LogicGateOutputSignalProcessor.java:
##########
@@ -46,4 +46,10 @@ protected void reset(SequenceMemory memory, ValueResolver valueResolver) {
// by each downstream LogicGate resetting itself via resetPrior(). There is no
// state held here that needs clearing.
}
+
+ public void clearParentBit(SequenceMemory memory) {
+ long[] gateMemory = memory.getLogicGateMemory();
+ gateMemory[gate.getGateIndex()] = gateMemory[gate.getGateIndex()] & ~(1L << (index - 1));
+ memory.setLogicGateMatched(gate.getGateIndex(), false);
Review Comment:
This clears only the immediate parent's state. If that parent already propagated to a grandparent, the grandparent retains a stale match bit. For example, with `and(and(xor(a, b), c), d)`, matching `a` and `c`, then matching `b` reverts the XOR but leaves the outer AND's inner-AND bit set, so `d` incorrectly completes the step. Reversion needs to re-evaluate the parent and propagate the transition recursively through the circuit.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/DSL.java:
##########
@@ -555,7 +555,7 @@ public static FixedValueItem expr( String exprId, boolean value ) {
// -- Existential operator --
- public static ExprViewItem not( ViewItemBuilder<?> expression, ViewItemBuilder<?>... expressions) {
+ public static ExistentialExprViewItem not( ViewItemBuilder<?> expression, ViewItemBuilder<?>... expressions) {
Review Comment:
Changing this public static method's return type changes its JVM descriptor, so clients compiled against the previous `ExprViewItem not(...)` signature will fail with `NoSuchMethodError` even though source recompilation succeeds. Preserve the existing descriptor and expose sequence-step typing through a binary-compatible API design.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -194,6 +221,50 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
throw new UnsupportedOperationException( "Unknown " + ruleItem );
}
+ /**
+ * Rewrites bare xor()/xnor() steps in a sequence by folding each such step
+ * with the immediately following positive step into an and(xor(...), next) pair.
+ * <p>
+ * {@code sequence(xor(a, b), c)} is legal sugar for {@code sequence(and(xor(a, b), c))}.
+ * The trailing-XOR check in the caller will catch {@code sequence(xor(a, b))} (no following step).
+ */
+ private static org.drools.model.SequenceStep[] rewriteXorSteps(org.drools.model.SequenceStep[] steps) {
+ boolean hasXor = false;
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
+ hasXor = true;
+ break;
+ }
+ }
+ }
+ if (!hasXor) {
+ return steps;
+ }
+ List<org.drools.model.SequenceStep> result = new ArrayList<>();
+ for (int i = 0; i < steps.length; i++) {
+ org.drools.model.SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
+ // Wrap: and(xor/xnor(...), nextStep)
+ CombinedExprViewItem wrapped = new CombinedExprViewItem(
+ Condition.Type.AND,
+ new ViewItem[]{(ViewItem) step, (ViewItem) steps[i + 1]});
+ result.add(wrapped);
+ i++; // consume the following step
Review Comment:
The rewrite consumes any following step, not necessarily the required positive step. Thus `sequence(xor(a,b), xor(c,d), e)` becomes `sequence(and(xor(a,b), xor(c,d)), e)`: the second XOR is no longer folded with `e`, and the first step can advance before `e`. Validate the next step as positive or normalize chained absence/reverting gates without consuming their intended trigger.
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -185,7 +185,34 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
if (ruleItem instanceof SequenceViewItem) {
SequenceViewItem sv = (SequenceViewItem) ruleItem;
- List<Condition> steps = Arrays.stream(sv.getSteps())
+ org.drools.model.SequenceStep[] rawSteps = rewriteXorSteps(sv.getSteps());
+ if (rawSteps.length > 0) {
+ org.drools.model.SequenceStep last = rawSteps[rawSteps.length - 1];
+ if (last instanceof ExistentialExprViewItem) {
+ ExistentialExprViewItem ex = (ExistentialExprViewItem) last;
+ if (ex.getType() == Condition.Type.NOT) {
+ throw new IllegalArgumentException(
+ "sequence(): trailing not() or nor() requires a following positive step " +
+ "or a completeWithin(...) deadline. " +
+ "Trailing absence with completeWithin is planned but not yet implemented. " +
+ "See ADR 0002.");
+ }
+ }
+ if (last instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) last;
+ if (comb.getType() == Condition.Type.NOT
+ || comb.getType() == Condition.Type.NOR
+ || comb.getType() == Condition.Type.XOR
+ || comb.getType() == Condition.Type.XNOR) {
Review Comment:
The trailing-gate guard only checks the outer node, so wrapping a reverting gate bypasses it. `sequence(and(xor(a, b)))` is accepted, then terminates as soon as the first child matches; the second child can never revert it, defeating the exactly-one-before-a-positive-trigger contract. Validate the effective condition tree and require a genuine positive trigger rather than relying on the top-level type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4193224790
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -185,7 +185,34 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
if (ruleItem instanceof SequenceViewItem) {
SequenceViewItem sv = (SequenceViewItem) ruleItem;
- List<Condition> steps = Arrays.stream(sv.getSteps())
+ org.drools.model.SequenceStep[] rawSteps = rewriteXorSteps(sv.getSteps());
+ if (rawSteps.length > 0) {
+ org.drools.model.SequenceStep last = rawSteps[rawSteps.length - 1];
+ if (last instanceof ExistentialExprViewItem) {
+ ExistentialExprViewItem ex = (ExistentialExprViewItem) last;
+ if (ex.getType() == Condition.Type.NOT) {
+ throw new IllegalArgumentException(
+ "sequence(): trailing not() or nor() requires a following positive step " +
+ "or a completeWithin(...) deadline. " +
+ "Trailing absence with completeWithin is planned but not yet implemented. " +
+ "See ADR 0002.");
+ }
+ }
+ if (last instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) last;
+ if (comb.getType() == Condition.Type.NOT
+ || comb.getType() == Condition.Type.NOR
+ || comb.getType() == Condition.Type.XOR
+ || comb.getType() == Condition.Type.XNOR) {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie#7081] sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#discussion_r4412809227
##########
drools-model/drools-canonical-model/src/main/java/org/drools/model/impl/ViewPatternBuilder.java:
##########
@@ -194,6 +219,235 @@ public static Condition ruleItem2Condition(RuleItem ruleItem) {
throw new UnsupportedOperationException( "Unknown " + ruleItem );
}
+ private static boolean hasPositiveTrigger(SequenceStep step) {
+ if (step instanceof ExistentialExprViewItem) {
+ ExistentialExprViewItem ex = (ExistentialExprViewItem) step;
+ if (ex.getType() == Condition.Type.NOT) {
+ return false;
+ }
+ return true;
+ }
+ if (step instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) step;
+ Condition.Type type = comb.getType();
+ if (type == Condition.Type.NOT || type == Condition.Type.NOR
+ || type == Condition.Type.XOR || type == Condition.Type.XNOR) {
+ return false;
+ }
+ if (type == Condition.Type.AND) {
+ // In an AND composite, at least one child must be a positive trigger
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (expr instanceof SequenceStep) {
+ if (hasPositiveTrigger((SequenceStep) expr)) {
+ return true;
+ }
+ } else {
+ return true;
+ }
+ }
+ return false;
+ }
+ if (type == Condition.Type.OR) {
+ // In an OR composite, all branches must have a positive trigger
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (expr instanceof SequenceStep) {
+ if (!hasPositiveTrigger((SequenceStep) expr)) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+ }
+ return true;
+ }
+
+ private static boolean isAbsenceStep(SequenceStep step) {
+ if (step instanceof ExistentialExprViewItem) {
+ return ((ExistentialExprViewItem) step).getType() == Condition.Type.NOT;
+ }
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type type = ((CombinedExprViewItem) step).getType();
+ return type == Condition.Type.NOT || type == Condition.Type.NOR;
+ }
+ throw new IllegalStateException("Unexpected SequenceStep type: " + step.getClass());
+ }
+
+ private static void validateActivationMatching(org.drools.model.view.ViewItem item) {
+ if (item instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) item;
+ Condition.Type type = comb.getType();
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ validateActivationMatching(expr);
+ }
+ if (type == Condition.Type.XOR) {
+ int matchedChildren = 0;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matchedChildren++;
+ }
+ }
+ if (matchedChildren >= 2) {
+ throw new IllegalArgumentException(
+ "sequence(): oneOf() has " + matchedChildren + " children that match on step activation (0 inputs), " +
+ "causing an immediate over-match veto upon activation. oneOf() requires exactly one child to match.");
+ }
+ }
+ }
+ }
+
+ private static boolean matchesOnActivation(org.drools.model.view.ViewItem item) {
+ if (item instanceof CombinedExprViewItem) {
+ CombinedExprViewItem comb = (CombinedExprViewItem) item;
+ Condition.Type type = comb.getType();
+ if (type == Condition.Type.XNOR) {
+ int matched = 0;
+ int total = comb.getExpressions().length;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matched++;
+ }
+ }
+ return matched == 0 || matched == total;
+ }
+ if (type == Condition.Type.XOR) {
+ int matched = 0;
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ matched++;
+ }
+ }
+ return matched == 1;
+ }
+ if (type == Condition.Type.AND) {
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (!matchesOnActivation(expr)) {
+ return false;
+ }
+ }
+ return comb.getExpressions().length > 0;
+ }
+ if (type == Condition.Type.OR) {
+ for (org.drools.model.view.ViewItem expr : comb.getExpressions()) {
+ if (matchesOnActivation(expr)) {
+ return true;
+ }
+ }
+ return false;
+ }
+ if (type == Condition.Type.NOT || type == Condition.Type.NOR) {
+ return false;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Rewrites runs of revertible gate steps (such as {@code oneOf()} / XOR and {@code noneOrAll()} / XNOR)
+ * by folding them together with their immediately following positive trigger into an {@code and(...)}
+ * composite step.
+ * <p>
+ * Standalone XOR/XNOR gates are non-propagating on their own within the sequence pipeline and require
+ * a following positive pattern step to act as the activation trigger. This transformation:
+ * <ul>
+ * <li>Separates and emits any interleaved absence guards ({@code NOT}/{@code NOR}/{@code absent()})
+ * before the folded composite so that compiler accumulation loops build them as independent
+ * veto side-channels.</li>
+ * <li>Folds consecutive XOR/XNOR gate steps and the subsequent positive trigger step into a single
+ * {@link CombinedExprViewItem} of type {@link Condition.Type#AND}.</li>
+ * </ul>
+ *
+ * @param steps the original sequence steps
+ * @return the rewritten sequence steps with folded gate composites
+ */
+ private static SequenceStep[] rewriteRevertingGateSteps(SequenceStep[] steps) {
+ boolean hasXor = false;
+ for (int i = 0; i < steps.length; i++) {
+ SequenceStep step = steps[i];
+ if (step instanceof CombinedExprViewItem) {
+ Condition.Type t = ((CombinedExprViewItem) step).getType();
+ if ((t == Condition.Type.XOR || t == Condition.Type.XNOR) && i + 1 < steps.length) {
Review Comment:
And is now flattened to prevent this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6359936104
Thanks for the update. Re-reviewed by AI. Thanks!
---
- **Fixed:** the multi-anchor veto loop, the binary compatibility of `DSL.not()` (now untouched, with `absent()` added instead), the nested XNOR initialization, the nested XOR reversion (consistent with the new veto semantics), and the AND + XNOR recovery.
- **Full reset works as described:** after a veto, `oneOf` / `noneOrAll` / later-step state is cleared, and a fresh attempt behaves like a new sequence.
One new blocker and a few questions remain.
### 1. [Blocker] A veto raised during `activate()` is not handled
If a `oneOf` has two or more children that are already satisfied at activation time (e.g. two `noneOrAll`, which match with zero inputs), the XOR over-matches while the step is still being activated, and `VetoSignalProcessor` runs from inside `LogicCircuitStep.activate()`.
`ev(name)` below is `pattern(declarationOf(Event.class)).expr("is-" + name, e -> e.getName().equals(name))`.
```java
// step 0: veto -> re-activate step 0 -> veto -> ... => StackOverflowError
sequence(oneOf(noneOrAll(ev("a"), ev("b")), noneOrAll(ev("c"), ev("e"))), ev("d"))
// later step: inserting s, d, s, d => ArrayIndexOutOfBoundsException at Sequence.java:176
sequence(ev("s"), oneOf(noneOrAll(ev("a"), ev("b")), noneOrAll(ev("c"), ev("e"))), ev("d"))
```
```text
StackOverflowError
at LogicGate.activate(LogicGate.java:172)
at LogicCircuitStep.activate(LogicCircuitStep.java:41)
at VetoSignalProcessor.consume(VetoSignalProcessor.java:89)
at LogicGate.consume(LogicGate.java:118)
at LogicGateOutputSignalProcessor.consume(LogicGateOutputSignalProcessor.java:35)
at LogicGate.propagate(LogicGate.java:148)
at LogicGate.activate(LogicGate.java:172)
...
```
In the later-step case, after the veto resets to step 0, the `activate()` loop of the vetoed step keeps going and registers adapters for a step that is no longer current. That corrupts the sequence state.
Such a `oneOf` can never succeed, so rejecting it at build time is probably the simplest fix. Alternatively, defer a veto raised during activation until the step's activation has finished.
### 2. [Pre-existing, probably a separate issue] `SequenceNode.AlphaAdapter.assertObject` can skip filters
The loop in `SequenceNode.AlphaAdapter.assertObject` caches `next` before delivering. If delivering to one filter causes an anchor to leave its step, the anchor's now-empty filters are removed from the active list. When one of them is the cached `next`, `LinkedList.remove()` nulls its pointers and the loop ends early. Filters further down the list never see the fact.
```java
sequence(ev("s"),
absent(ev("x")),
pattern(declarationOf(Event.class)).expr("d-own", anchor, (e, a) -> e.getName().equals("d") && a.equals(e.getOwner())),
ev("x"))
// anchor1, anchor2; insert s (both -> step 1); insert d owned by anchor2 (anchor2 -> step 2);
// insert x: anchor1 is vetoed, and anchor2 never receives x => expected 1 firing, got 0
```
The same thing happens without any veto, with `or(ev("x"), d-own)` in place of `absent(ev("x"))`. So this looks like an existing v1 bug, since `SequenceNode` is not touched by this PR. The veto adds a new way to hit it, though. Maybe fix it here, or file a separate issue?
### 3. Questions on semantics
- **`oneOf` under `or`:** with `and(or(oneOf(a, b), c), d)` and arrivals a, c, b, d, the OR is already satisfied by `c`. Still, the `oneOf` over-match on `b` vetoes the whole sequence, so there are 0 firings. Is that intended? If so, it may be worth documenting, or rejecting `oneOf` under `or`.
- **Does the vetoing fact start a new attempt?** With `sequence(ev("s"), absent(ev("s")), ev("d"))` and arrivals s, s, d, there are 0 firings: the second `s` vetoes but doesn't restart the sequence. Right now that outcome comes from the early exit in item 2 and the order of the filters, not from an explicit rule. It would be good to define it.
### Nit
- A top-level bare `oneOf()` / `noneOrAll()` throws `UnsupportedOperationException` in `KiePackagesBuilder`. The other validation errors in this PR throw `IllegalArgumentException`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] feat(sequencing): sequence() API v2 — NOR/XOR gates, absence-guard fold, xnor() step, veto reset semantics [incubator-kie]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6340742129
@tkobayas I went through each item and took actions. Here is a summary.
1. Fixed. DynamicFilter.assertObject() now increments a deliveryEpoch counter before iterating and stamps every newly-registered adapter with that epoch in addSignalAdapter().
2. Fixed. DSL.not() returns ExprViewItem (not ExistentialExprViewItem).
3. Fixed. LogicGate.activate() now evaluates predicate.test(memory.getLogicGateMemory()[gateIndex], allMatched) — the actual gate memory — instead of the hard-coded 0L.
4. Fixed. LogicGate.clearInputBit() (called by LogicGateOutputSignalProcessor.clearParentBit()) re-evaluates the predicate after clearing the bit (lines 132–143): if the result transitions matched → unmatched it calls clearParentBit on the parent recursively; if it transitions unmatched → matched it calls propagate upward. The reversion therefore cascades through the full ancestor chain
5. Fixed. LogicGate.resetPrior() only clears memory when !statusCanRevert. Gates compiled for reverting children (XOR, XNOR) have statusCanRevert = true, so their parent retains the contribution bits from other inputs across a propagation. Non-reverting gates (AND, OR, NOT) still reset as before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org