From 5c8daaa4626f885d319d3de41f57f7327691debf Mon Sep 17 00:00:00 2001 From: Lukas Petrovicky Date: Sun, 26 Jul 2026 08:29:22 +0200 Subject: [PATCH] fix: prevent stale tuples after a join --- .../bavet/common/AbstractIfExistsNode.java | 111 ++--- .../common/AbstractIndexedIfExistsNode.java | 8 +- .../bavet/common/AbstractIndexedJoinNode.java | 19 +- .../impl/bavet/common/AbstractJoinNode.java | 94 +++-- .../common/AbstractUnindexedIfExistsNode.java | 8 +- .../common/AbstractUnindexedJoinNode.java | 15 +- .../stream/bavet/BavetRegressionTest.java | 384 ++++++++++++++++++ 7 files changed, 525 insertions(+), 114 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIfExistsNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIfExistsNode.java index 2cfea42e88..cb57dd83d0 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIfExistsNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIfExistsNode.java @@ -126,33 +126,24 @@ protected void decrementCounterRight(ExistsCounter counter) { } // Else do not even propagate an update } - // Prepends tracker into the left tuple's hidden intrusive tracker list. - // The left tuple's store at inputStoreIndexLeftTrackerList holds the list head (null = empty). - private void linkLeft(FilteringTracker tracker) { - var leftTuple = tracker.counter.leftTuple; - FilteringTracker head = leftTuple.getStore(inputStoreIndexLeftTrackerList); - tracker.leftNext = head; - if (head != null) { - head.leftPrev = tracker; + // Clears the left tracker list rooted at leftTuple's inputStoreIndexLeftTrackerList slot, + // cross-removing each tracker from its right tuple's hidden list. No-op when !isFiltering. + // Walk safety: removeFromRight only touches right-side links, so leftNext is stable across the call. + protected void clearLeftTrackerList(LeftTuple_ leftTuple) { + if (!isFiltering) { + return; } - leftTuple.setStore(inputStoreIndexLeftTrackerList, tracker); - } - - // Prepends tracker into the right tuple's hidden intrusive tracker list. - // The right tuple's store at inputStoreIndexRightTrackerList holds the list head (null = empty). - private void linkRight(FilteringTracker tracker) { - var rightTuple = tracker.rightTuple; - FilteringTracker head = rightTuple.getStore(inputStoreIndexRightTrackerList); - tracker.rightNext = head; - if (head != null) { - head.rightPrev = tracker; + FilteringTracker tracker = leftTuple.removeStore(inputStoreIndexLeftTrackerList); + while (tracker != null) { + var next = tracker.leftNext; + removeRight(tracker); + tracker = next; } - rightTuple.setStore(inputStoreIndexRightTrackerList, tracker); } // Splices tracker out of its right tuple's hidden list (used when clearing from the left side). // Nulls the tracker's right links; if tracker is the head, updates the right tuple's slot. - private void removeFromRight(FilteringTracker tracker) { + private void removeRight(FilteringTracker tracker) { var prev = tracker.rightPrev; var next = tracker.rightNext; if (prev != null) { @@ -168,9 +159,22 @@ private void removeFromRight(FilteringTracker tracker) { tracker.rightNext = null; } + // Clears the right tracker list rooted at rightTuple's inputStoreIndexRightTrackerList slot, + // decrementing each counter and cross-removing each tracker from its left tuple's hidden list. + // Walk safety: removeFromLeft only touches left-side links, so rightNext is stable across the call. + protected void clearRightTrackerList(UniTuple rightTuple) { + FilteringTracker tracker = rightTuple.removeStore(inputStoreIndexRightTrackerList); + while (tracker != null) { + var next = tracker.rightNext; + decrementCounterRight(tracker.counter); + removeLeft(tracker); + tracker = next; + } + } + // Splices tracker out of its left tuple's hidden list (used when clearing from the right side). // Nulls the tracker's left links; if tracker is the head, updates the left tuple's slot. - private void removeFromLeft(FilteringTracker tracker) { + private void removeLeft(FilteringTracker tracker) { var prev = tracker.leftPrev; var next = tracker.leftNext; if (prev != null) { @@ -186,44 +190,47 @@ private void removeFromLeft(FilteringTracker tracker) { tracker.leftNext = null; } - // Clears the left tracker list rooted at leftTuple's inputStoreIndexLeftTrackerList slot, - // cross-removing each tracker from its right tuple's hidden list. No-op when !isFiltering. - // Walk safety: removeFromRight only touches right-side links, so leftNext is stable across the call. - protected void clearLeftTrackerList(LeftTuple_ leftTuple) { - if (!isFiltering) { + protected void updateCounterLeft(ExistsCounter counter, UniTuple rightTuple) { + if (!rightTuple.getState().isActive()) { + // The mirror image of updateCounterRight(...): here the right tuple is the retracting one, + // which happens when the right input's node sits in a higher layer than the left input's, + // so the left's inserts and updates are delivered before the right's retracts are. + // Skipping is safe, as the pending retract will not have a tracker to clear for this pair. return; } - FilteringTracker tracker = leftTuple.removeStore(inputStoreIndexLeftTrackerList); - while (tracker != null) { - var next = tracker.leftNext; - removeFromRight(tracker); - tracker = next; + if (testFiltering(counter.leftTuple, rightTuple)) { + counter.countRight++; + var tracker = new FilteringTracker<>(counter, rightTuple); + linkLeft(tracker); + linkRight(tracker); } } - // Clears the right tracker list rooted at rightTuple's inputStoreIndexRightTrackerList slot, - // decrementing each counter and cross-removing each tracker from its left tuple's hidden list. - // Walk safety: removeFromLeft only touches left-side links, so rightNext is stable across the call. - protected void clearRightTrackerList(UniTuple rightTuple) { - FilteringTracker tracker = rightTuple.removeStore(inputStoreIndexRightTrackerList); - while (tracker != null) { - var next = tracker.rightNext; - decrementCounterRight(tracker.counter); - removeFromLeft(tracker); - tracker = next; + // Prepends tracker into the left tuple's hidden intrusive tracker list. + // The left tuple's store at inputStoreIndexLeftTrackerList holds the list head (null = empty). + private void linkLeft(FilteringTracker tracker) { + var leftTuple = tracker.counter.leftTuple; + FilteringTracker head = leftTuple.getStore(inputStoreIndexLeftTrackerList); + tracker.leftNext = head; + if (head != null) { + head.leftPrev = tracker; } + leftTuple.setStore(inputStoreIndexLeftTrackerList, tracker); } - protected void updateCounterFromLeft(ExistsCounter counter, UniTuple rightTuple) { - if (testFiltering(counter.leftTuple, rightTuple)) { - counter.countRight++; - var tracker = new FilteringTracker<>(counter, rightTuple); - linkLeft(tracker); - linkRight(tracker); + // Prepends tracker into the right tuple's hidden intrusive tracker list. + // The right tuple's store at inputStoreIndexRightTrackerList holds the list head (null = empty). + private void linkRight(FilteringTracker tracker) { + var rightTuple = tracker.rightTuple; + FilteringTracker head = rightTuple.getStore(inputStoreIndexRightTrackerList); + tracker.rightNext = head; + if (head != null) { + head.rightPrev = tracker; } + rightTuple.setStore(inputStoreIndexRightTrackerList, tracker); } - protected void updateCounterFromRight(ExistsCounter counter, UniTuple rightTuple) { + protected void updateCounterRight(ExistsCounter counter, UniTuple rightTuple) { var leftTuple = counter.leftTuple; if (!leftTuple.getState().isActive()) { // Assume the following scenario: @@ -237,9 +244,9 @@ protected void updateCounterFromRight(ExistsCounter counter, UniTupl // We avoid this situation as it is clear that the outTuple must be retracted anyway, // and therefore any further updates to it are pointless. // - // It is possible that the same problem would exist coming from the other side as well, - // and therefore the right tuple would have to be checked for active state as well. - // However, no such issue could have been reproduced; when in doubt, leave it out. + // The left tuple can be inactive here because its node sits in a higher layer than the right's: + // the right's inserts and updates are delivered before the left's retracts are. + // The mirror case is possible too, see updateCounterLeft(...). return; } if (testFiltering(leftTuple, rightTuple)) { diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedIfExistsNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedIfExistsNode.java index 3387b3cac9..7e27a733fb 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedIfExistsNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedIfExistsNode.java @@ -103,7 +103,7 @@ private void updateCounterRight(LeftTuple_ leftTuple, Object compositeKey, Exist // Trackers link themselves into the left tuple's inputStoreIndexLeftTrackerList slot. // No list object is needed; the slot starts null and the first tracker becomes the head. forEachRightFromLeft(leftTuple, compositeKey, - rightTuple -> updateCounterFromLeft(counter, rightTuple)); + rightTuple -> updateCounterLeft(counter, rightTuple)); } } @@ -138,7 +138,7 @@ public final void updateLeft(LeftTuple_ leftTuple) { clearLeftTrackerList(leftTuple); counter.countRight = 0; forEachRightFromLeft(leftTuple, oldCompositeKey, - rightTuple -> updateCounterFromLeft(counter, rightTuple)); + rightTuple -> updateCounterLeft(counter, rightTuple)); updateCounterLeft(counter); } } else { @@ -208,7 +208,7 @@ private void updateCounterLeft(UniTuple rightTuple, Object compositeKey) // Trackers link themselves into the right tuple's inputStoreIndexRightTrackerList slot. // No list object is needed; the slot starts null and the first tracker becomes the head. forEachLeftCounter(rightTuple, compositeKey, - counter -> updateCounterFromRight(counter, rightTuple)); + counter -> updateCounterRight(counter, rightTuple)); } } @@ -226,7 +226,7 @@ public final void updateRight(UniTuple rightTuple) { if (isFiltering) { clearRightTrackerList(rightTuple); forEachLeftCounter(rightTuple, oldCompositeKey, - counter -> updateCounterFromRight(counter, rightTuple)); + counter -> updateCounterRight(counter, rightTuple)); } } else { // sameBucket: equal prefix unchanged ⇒ keep & reuse the cached bucket (no top lookup, no drop/recreate). diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedJoinNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedJoinNode.java index b3d5b01e2b..d836709abf 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedJoinNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractIndexedJoinNode.java @@ -119,7 +119,7 @@ public final void updateLeft(LeftTuple_ leftTuple) { indexerLeft.remove(oldCompositeKey, entry); } } - outTupleListLeft.clear(this::retractOutTupleByLeft); + outTupleListLeft.clear(this::retractOutTupleLeft); // outTupleListLeft is now empty // No need for leftTuple.setStore(inputStoreIndexLeftOutTupleList, outTupleListLeft); indexAndPropagateLeft(leftTuple, newCompositeKey, reuseBucket); @@ -153,12 +153,13 @@ private void indexAndPropagateLeft(LeftTuple_ leftTuple, Object compositeKey, bo // and requires adding null checks to the filter for something that should intuitively be impossible. // We avoid this situation as it is clear that it is pointless to insert this tuple. // - // It is possible that the same problem would exist coming from the other side as well, - // and therefore the right tuple would have to be checked for active state as well. - // However, no such issue could have been reproduced; when in doubt, leave it out. + // The left tuple can be inactive here because its node sits in a higher layer than the right's: + // the right's inserts are delivered before the left's retracts are. return; } - forEachRightMatch(leftTuple, compositeKey, rightTuple -> insertOutTupleFiltered(leftTuple, rightTuple)); + // The right tuples come out of the index and can be retracting for the mirror-image reason, + // hence insertOutTupleFilteredFromRight(...) rather than the unguarded insert. + forEachRightMatch(leftTuple, compositeKey, rightTuple -> insertOutTupleFilteredRight(leftTuple, rightTuple)); } @Override @@ -177,7 +178,7 @@ public final void retractLeft(LeftTuple_ leftTuple) { } else { indexerLeft.remove(compositeKey, entry); } - outTupleListLeft.clear(this::retractOutTupleByLeft); + outTupleListLeft.clear(this::retractOutTupleLeft); } @Override @@ -222,7 +223,7 @@ public final void updateRight(UniTuple rightTuple) { indexerRight.remove(oldCompositeKey, entry); } } - outTupleListRight.clear(this::retractOutTupleByRight); + outTupleListRight.clear(this::retractOutTupleRight); // outTupleListRight is now empty // No need for rightTuple.setStore(inputStoreIndexRightOutTupleList, outTupleListRight); indexAndPropagateRight(rightTuple, newCompositeKey, reuseBucket); @@ -245,7 +246,7 @@ private void indexAndPropagateRight(UniTuple rightTuple, Object composit } else { rightTuple.setStore(inputStoreIndexRightEntry, indexerRight.put(compositeKey, rightTuple)); } - forEachLeftMatch(rightTuple, compositeKey, leftTuple -> insertOutTupleFilteredFromLeft(leftTuple, rightTuple)); + forEachLeftMatch(rightTuple, compositeKey, leftTuple -> insertOutTupleFilteredLeft(leftTuple, rightTuple)); } @Override @@ -265,7 +266,7 @@ public final void retractRight(UniTuple rightTuple) { indexerRight.remove(compositeKey, entry); } - outTupleListRight.clear(this::retractOutTupleByRight); + outTupleListRight.clear(this::retractOutTupleRight); } /** diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractJoinNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractJoinNode.java index 00dfed5752..6edbe0b583 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractJoinNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractJoinNode.java @@ -69,18 +69,7 @@ public StreamKind getStreamKind() { protected abstract boolean testFiltering(LeftTuple_ leftTuple, UniTuple rightTuple); - protected final void insertOutTuple(LeftTuple_ leftTuple, UniTuple rightTuple) { - var outTuple = createOutTuple(leftTuple, rightTuple); - TupleList outTupleListLeft = leftTuple.getStore(inputStoreIndexLeftOutTupleList); - outTupleListLeft.add(outTuple); - outTuple.setStore(outputStoreIndexLeftOutTupleList, outTupleListLeft); - TupleList outTupleListRight = rightTuple.getStore(inputStoreIndexRightOutTupleList); - outTupleListRight.add(outTuple); - outTuple.setStore(outputStoreIndexRightOutTupleList, outTupleListRight); - propagationQueue.insert(outTuple); - } - - protected final void insertOutTupleFilteredFromLeft(LeftTuple_ leftTuple, UniTuple rightTuple) { + protected final void insertOutTupleFilteredLeft(LeftTuple_ leftTuple, UniTuple rightTuple) { if (!leftTuple.getState().isActive()) { // Assume the following scenario: // - The join is of two entities of the same type, both filtering out unassigned. @@ -92,20 +81,44 @@ protected final void insertOutTupleFilteredFromLeft(LeftTuple_ leftTuple, UniTup // and requires adding null checks to the filter for something that should intuitively be impossible. // We avoid this situation as it is clear that it is pointless to insert this tuple. // - // It is possible that the same problem would exist coming from the other side as well, - // and therefore the right tuple would have to be checked for active state as well. - // However, no such issue could have been reproduced; when in doubt, leave it out. + // The left tuple can be inactive here because its node sits in a higher layer than the right's: + // the right's inserts are delivered before the left's retracts are. + // The mirror case is possible too, see insertOutTupleFilteredRight(...). + return; + } + insertOutTupleIfActiveFiltered(leftTuple, rightTuple); + } + + /** + * The mirror image of {@link #insertOutTupleFilteredLeft}: + * the right tuple is the one read out of storage, and can therefore be the retracting one. + * Reachable when the right input's node sits in a higher layer than the left input's, + * which delivers the left's inserts before the right's retracts. + */ + protected final void insertOutTupleFilteredRight(LeftTuple_ leftTuple, UniTuple rightTuple) { + if (!rightTuple.getState().isActive()) { return; } - insertOutTupleFiltered(leftTuple, rightTuple); + insertOutTupleIfActiveFiltered(leftTuple, rightTuple); } - protected final void insertOutTupleFiltered(LeftTuple_ leftTuple, UniTuple rightTuple) { + private void insertOutTupleIfActiveFiltered(LeftTuple_ leftTuple, UniTuple rightTuple) { if (!isFiltering || testFiltering(leftTuple, rightTuple)) { insertOutTuple(leftTuple, rightTuple); } } + private void insertOutTuple(LeftTuple_ leftTuple, UniTuple rightTuple) { + var outTuple = createOutTuple(leftTuple, rightTuple); + TupleList outTupleListLeft = leftTuple.getStore(inputStoreIndexLeftOutTupleList); + outTupleListLeft.add(outTuple); + outTuple.setStore(outputStoreIndexLeftOutTupleList, outTupleListLeft); + TupleList outTupleListRight = rightTuple.getStore(inputStoreIndexRightOutTupleList); + outTupleListRight.add(outTuple); + outTuple.setStore(outputStoreIndexRightOutTupleList, outTupleListRight); + propagationQueue.insert(outTuple); + } + protected final void innerUpdateLeft(LeftTuple_ leftTuple, Consumer>> rightTupleConsumer) { // Prefer an update over retract-insert if possible TupleList outTupleListLeft = leftTuple.getStore(inputStoreIndexLeftOutTupleList); @@ -127,9 +140,9 @@ protected final void innerUpdateLeft(LeftTuple_ leftTuple, Consumer outTupleListRight = outTuple.getStore(outputStoreIndexRightOutTupleList); outTupleListRight.mark(outTuple, version); } - rightTupleConsumer.accept(rightTuple -> processOutTupleUpdateFromRight(leftTuple, rightTuple, version)); + rightTupleConsumer.accept(rightTuple -> processOutTupleUpdateRight(leftTuple, rightTuple, version)); } } @@ -160,7 +173,12 @@ private void doUpdateOutTuple(OutTuple_ outTuple) { propagationQueue.update(outTuple); } - private void processOutTupleUpdateFromRight(LeftTuple_ leftTuple, UniTuple rightTuple, long version) { + private void processOutTupleUpdateRight(LeftTuple_ leftTuple, UniTuple rightTuple, long version) { + if (!rightTuple.getState().isActive()) { + // The mirror image of processOutTupleUpdateLeft(...): here the right tuple is the retracting one. + // Leaving its mark set is harmless, as getMark() only ever returns a mark of the matching version. + return; + } TupleList outTupleListRight = rightTuple.getStore(inputStoreIndexRightOutTupleList); processOutTupleUpdate(leftTuple, rightTuple, outTupleListRight.getMark(version)); } @@ -199,6 +217,15 @@ private void removeRightEntry(OutTuple_ outTuple) { removeEntry(outTuple, outputStoreIndexRightOutTupleList); } + private void propagateRetract(OutTuple_ outTuple) { + var state = outTuple.getState(); + if (!state.isActive()) { // Impossible because they shouldn't linger in the indexes. + throw new IllegalStateException("Impossible state: The tuple (%s) in node (%s) is in an unexpected state (%s)." + .formatted(outTuple, this, state)); + } + propagationQueue.retract(outTuple, state == TupleState.CREATING ? TupleState.ABORTING : TupleState.DYING); + } + protected final void innerUpdateRight(UniTuple rightTuple, Consumer> leftTupleConsumer) { // Prefer an update over retract-insert if possible TupleList outTupleListRight = rightTuple.getStore(inputStoreIndexRightOutTupleList); @@ -214,11 +241,11 @@ protected final void innerUpdateRight(UniTuple rightTuple, Consumer outTupleListLeft = outTuple.getStore(outputStoreIndexLeftOutTupleList); outTupleListLeft.mark(outTuple, version); } - leftTupleConsumer.accept(leftTuple -> processOutTupleUpdateFromLeft(leftTuple, rightTuple, version)); + leftTupleConsumer.accept(leftTuple -> processOutTupleUpdateLeft(leftTuple, rightTuple, version)); } } - private void processOutTupleUpdateFromLeft(LeftTuple_ leftTuple, UniTuple rightTuple, long version) { + private void processOutTupleUpdateLeft(LeftTuple_ leftTuple, UniTuple rightTuple, long version) { if (!leftTuple.getState().isActive()) { // Assume the following scenario: // - The join is of two entities of the same type, both filtering out unassigned. @@ -231,9 +258,9 @@ private void processOutTupleUpdateFromLeft(LeftTuple_ leftTuple, UniTuple outTupleListLeft = leftTuple.getStore(inputStoreIndexLeftOutTupleList); @@ -259,22 +286,13 @@ private void updateOutTupleRight(OutTuple_ outTuple, UniTuple rightTuple doUpdateOutTuple(outTuple); } - private void propagateRetract(OutTuple_ outTuple) { - var state = outTuple.getState(); - if (!state.isActive()) { // Impossible because they shouldn't linger in the indexes. - throw new IllegalStateException("Impossible state: The tuple (%s) in node (%s) is in an unexpected state (%s)." - .formatted(outTuple, this, state)); - } - propagationQueue.retract(outTuple, state == TupleState.CREATING ? TupleState.ABORTING : TupleState.DYING); - } - - protected void retractOutTupleByLeft(OutTuple_ outTuple) { + protected void retractOutTupleLeft(OutTuple_ outTuple) { outTuple.setStore(outputStoreIndexLeftOutTupleList, null); // The caller will clear the entire list in one go. removeRightEntry(outTuple); propagateRetract(outTuple); } - protected void retractOutTupleByRight(OutTuple_ outTuple) { + protected void retractOutTupleRight(OutTuple_ outTuple) { removeLeftEntry(outTuple); outTuple.setStore(outputStoreIndexRightOutTupleList, null); // The caller will clear the entire list in one go. propagateRetract(outTuple); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedIfExistsNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedIfExistsNode.java index 8ee8f8df2d..b0a2224f26 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedIfExistsNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedIfExistsNode.java @@ -50,7 +50,7 @@ public final void insertLeft(LeftTuple_ leftTuple) { // Trackers link themselves into the left tuple's inputStoreIndexLeftTrackerList slot. // No list object is needed; the slot starts null and the first tracker becomes the head. for (var rightTuple : rightTupleList) { - updateCounterFromLeft(counter, rightTuple); + updateCounterLeft(counter, rightTuple); } } initCounterLeft(counter); @@ -74,7 +74,7 @@ public final void updateLeft(LeftTuple_ leftTuple) { clearLeftTrackerList(leftTuple); counter.countRight = 0; for (var rightTuple : rightTupleList) { - updateCounterFromLeft(counter, rightTuple); + updateCounterLeft(counter, rightTuple); } updateCounterLeft(counter); } @@ -108,7 +108,7 @@ public final void insertRight(UniTuple rightTuple) { // Trackers link themselves into the right tuple's inputStoreIndexRightTrackerList slot. // No list object is needed; the slot starts null and the first tracker becomes the head. for (var counter : counterList) { - updateCounterFromRight(counter, rightTuple); + updateCounterRight(counter, rightTuple); } } } @@ -124,7 +124,7 @@ public final void updateRight(UniTuple rightTuple) { if (isFiltering) { clearRightTrackerList(rightTuple); for (var counter : counterList) { - updateCounterFromRight(counter, rightTuple); + updateCounterRight(counter, rightTuple); } } } diff --git a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedJoinNode.java b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedJoinNode.java index add197a08e..11620b076b 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedJoinNode.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractUnindexedJoinNode.java @@ -51,13 +51,14 @@ public final void insertLeft(LeftTuple_ leftTuple) { // and requires adding null checks to the filter for something that should intuitively be impossible. // We avoid this situation as it is clear that it is pointless to insert this tuple. // - // It is possible that the same problem would exist coming from the other side as well, - // and therefore the right tuple would have to be checked for active state as well. - // However, no such issue could have been reproduced; when in doubt, leave it out. + // The left tuple can be inactive here because its node sits in a higher layer than the right's: + // the right's inserts are delivered before the left's retracts are. return; } + // The right tuples come out of the list and can be retracting for the mirror-image reason, + // hence insertOutTupleFilteredFromRight(...) rather than the unguarded insert. for (var rightTuple = rightTupleList.first(); rightTuple != null; rightTuple = rightTupleList.next(rightTuple)) { - insertOutTupleFiltered(leftTuple, rightTuple); + insertOutTupleFilteredRight(leftTuple, rightTuple); } } @@ -79,7 +80,7 @@ public final void retractLeft(LeftTuple_ leftTuple) { return; } leftTupleList.remove(leftTuple); - outTupleListLeft.clear(this::retractOutTupleByLeft); + outTupleListLeft.clear(this::retractOutTupleLeft); } @Override @@ -92,7 +93,7 @@ public final void insertRight(UniTuple rightTuple) { rightTupleList.add(rightTuple); rightTuple.setStore(inputStoreIndexRightOutTupleList, rightOutTupleListBuilder.get()); for (var leftTuple = leftTupleList.first(); leftTuple != null; leftTuple = leftTupleList.next(leftTuple)) { - insertOutTupleFilteredFromLeft(leftTuple, rightTuple); + insertOutTupleFilteredLeft(leftTuple, rightTuple); } } @@ -114,7 +115,7 @@ public final void retractRight(UniTuple rightTuple) { return; } rightTupleList.remove(rightTuple); - outTupleListRight.clear(this::retractOutTupleByRight); + outTupleListRight.clear(this::retractOutTupleRight); } } diff --git a/core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java b/core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java index 8e7de7380f..9ef46b3952 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java @@ -1,8 +1,11 @@ package ai.timefold.solver.core.impl.score.stream.bavet; +import static ai.timefold.solver.core.api.score.stream.Joiners.equal; import static ai.timefold.solver.core.api.score.stream.Joiners.filtering; import static org.assertj.core.api.Assertions.assertThat; +import java.util.List; +import java.util.Objects; import java.util.function.Function; import ai.timefold.solver.core.api.score.SimpleScore; @@ -14,6 +17,9 @@ import ai.timefold.solver.core.impl.score.stream.common.AbstractConstraintStreamTest; import ai.timefold.solver.core.testdomain.TestdataEntity; import ai.timefold.solver.core.testdomain.TestdataSolution; +import ai.timefold.solver.core.testdomain.list.unassignedvar.TestdataAllowsUnassignedValuesListEntity; +import ai.timefold.solver.core.testdomain.list.unassignedvar.TestdataAllowsUnassignedValuesListSolution; +import ai.timefold.solver.core.testdomain.list.unassignedvar.TestdataAllowsUnassignedValuesListValue; import ai.timefold.solver.core.testdomain.shadow.multiplelistener.TestdataListMultipleShadowVariableSolution; import ai.timefold.solver.core.testdomain.shadow.multiplelistener.TestdataListMultipleShadowVariableValue; @@ -74,6 +80,384 @@ void joinWithNullKeyFromRight() { assertMatch(entity2, entity2)); } + @TestTemplate + public void filteringJoinNullConflictRight() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .join(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .map(v -> v), + equal(TestdataAllowsUnassignedValuesListValue::getEntity, + TestdataAllowsUnassignedValuesListValue::getEntity), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 1); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + + // Unassign+assign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value2); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value2)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value2); + + assertScore(scoreDirector, + assertMatch(value2, value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value1)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + } + + } + + @TestTemplate + public void filteringJoinNullConflictRightUnindexed() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .join(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .map(v -> v), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 1); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + + // Unassign+assign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value2); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value2)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value2); + + assertScore(scoreDirector, + assertMatch(value2, value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value1)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + } + + } + + @TestTemplate + public void filteringJoinNullConflictRightViaIfExists() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .join(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .ifExists(TestdataAllowsUnassignedValuesListEntity.class), + equal(TestdataAllowsUnassignedValuesListValue::getEntity, + TestdataAllowsUnassignedValuesListValue::getEntity), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 1); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + + // Unassign+assign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value2); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value2)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value2); + + assertScore(scoreDirector, + assertMatch(value2, value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value1)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1, value1)); + } + + } + + @TestTemplate + public void filteringJoinNullConflictRightUnassignOne() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .join(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .map(v -> v), + equal(TestdataAllowsUnassignedValuesListValue::getEntity, + TestdataAllowsUnassignedValuesListValue::getEntity), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value2); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().addAll(List.of(value1, value2)); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 2); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value2); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1, value1), + assertMatch(value1, value2), + assertMatch(value2, value1), + assertMatch(value2, value2)); + + // Unassign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 2); + entity.getValueList().remove(value1); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value2, value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1, value1), + assertMatch(value1, value2), + assertMatch(value2, value1), + assertMatch(value2, value2)); + } + + } + + @TestTemplate + public void filteringIfExistsNullConflictRight() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .ifExists(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .map(v -> v), + equal(TestdataAllowsUnassignedValuesListValue::getEntity, + TestdataAllowsUnassignedValuesListValue::getEntity), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 1); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1)); + + // Unassign+assign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value2); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value2)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value2); + + assertScore(scoreDirector, + assertMatch(value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value1)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1)); + } + + } + + @TestTemplate + public void filteringIfExistsNullConflictRightUnindexed() { + var solution = TestdataAllowsUnassignedValuesListSolution.generateUninitializedSolution(2, 1); + var entity = solution.getEntityList().getFirst(); + var value1 = solution.getValueList().get(0); + var value2 = solution.getValueList().get(1); + + try (InnerScoreDirector scoreDirector = + buildScoreDirector(TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(), + factory -> new Constraint[] { + factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .ifExists(factory.forEach(TestdataAllowsUnassignedValuesListValue.class) + .map(v -> v), + filtering((a, b) -> { + Objects.requireNonNull(a.getEntity()); + Objects.requireNonNull(b.getEntity()); + return true; + })) + .penalize(SimpleScore.ONE) + .asConstraint(TEST_CONSTRAINT_ID) + })) { + + scoreDirector.setWorkingSolution(solution); + scoreDirector.beforeListVariableElementAssigned(entity, "valueList", value1); + scoreDirector.beforeListVariableChanged(entity, "valueList", 0, 0); + entity.getValueList().add(value1); + scoreDirector.afterListVariableChanged(entity, "valueList", 0, 1); + scoreDirector.afterListVariableElementAssigned(entity, "valueList", value1); + + assertScore(scoreDirector, + assertMatch(value1)); + + // Unassign+assign and check result. + var variableDescriptor = scoreDirector.getSolutionDescriptor() + .getListVariableDescriptor(); + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value2); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value2)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value1); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value2); + + assertScore(scoreDirector, + assertMatch(value2)); + + // Reassign and check result. + scoreDirector.beforeListVariableElementAssigned(variableDescriptor, value1); + scoreDirector.beforeListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.beforeListVariableChanged(variableDescriptor, entity, 0, 1); + entity.setValueList(List.of(value1)); + scoreDirector.afterListVariableChanged(variableDescriptor, entity, 0, 1); + scoreDirector.afterListVariableElementUnassigned(variableDescriptor, value2); + scoreDirector.afterListVariableElementAssigned(variableDescriptor, value1); + + assertScore(scoreDirector, + assertMatch(value1)); + } + + } + /** * @see Timefold Solver GitHub Issue 186 */