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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
