-
Notifications
You must be signed in to change notification settings - Fork 163
chore: improve Neighborhoods design for scalability #1903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the move stream framework by replacing the bi-dataset implementation with a joining iterator approach for improved performance. The changes eliminate BiDataset and associated classes, replacing them with iterator-based joining logic that performs filtering on-demand during iteration.
- Removed BiDataset and replaced with UniDataset-based joining using JoiningIterator
- Renamed BiEnumeratingFilter to BiEnumeratingPredicate for consistency
- Updated move definitions to use pick().pick() pattern with joiners instead of join() operations
- Fixed test assertions to reflect new iteration order (left-to-right instead of cached ordering)
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| DefaultBiSamplingStream.java | Renamed from DefaultBiFromUnisSamplingStream; updated to use BiEnumeratingJoinerComber instead of BiPredicate |
| DefaultBiFromBiSamplingStream.java | Deleted as BiDataset implementation removed |
| BiMoveStream.java | New implementation replacing FromUniBiMoveStream and FromBiUniMoveStream |
| BiOriginalMoveIterator.java | New iterator for sequential bi-move enumeration using JoiningIterator |
| BiRandomMoveIterator.java | New iterator for randomized bi-move selection with caching |
| JoiningIterator.java | New core iterator implementing joiner and filter logic |
| BiEnumeratingPredicate.java | Renamed from BiEnumeratingFilter for consistency |
| AbstractDataJoiner.java | Deleted; functionality moved to AbstractJoiner in bavet package |
| BiDataset.java, BiDatasetInstance.java, TerminalBiEnumeratingStream.java | Deleted as bi-dataset approach removed |
| SwapMoveDefinition.java | Refactored to use pick().pick() with lessThan joiner for uniqueness |
| ChangeMoveDefinition.java | Refactored to use pick().pick() with differentValueFilter and valueInRangeFilter |
| ListChangeMoveDefinition.java | Updated parameter order in isValidChange predicate |
| Test files | Updated assertions to match new left-to-right iteration order |
Comments suppressed due to low confidence (1)
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/maybeapi/stream/enumerating/function/BiEnumeratingPredicate.java:30
- The lambda implementation doesn't cast to BiEnumeratingPredicate<Solution_, A, B> as the return type specifies. The previous implementation used an explicit cast. Without the cast, this may cause type inference issues in some contexts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/move/BiOriginalMoveIterator.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/move/JoiningIterator.java
Show resolved
Hide resolved
...ain/java/ai/timefold/solver/core/impl/neighborhood/maybeapi/move/ListSwapMoveDefinition.java
Show resolved
Hide resolved
...fold/solver/core/impl/neighborhood/stream/enumerating/joiner/DefaultBiEnumeratingJoiner.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/move/BiMoveStream.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/maybeapi/stream/enumerating/function/BiEnumeratingPredicate.java:31
- [nitpick] The
andmethod now creates a lambda instead of castingTriPredicate.super.and(other). While this works, it loses the potential optimization that the default implementation in TriPredicate might have. Consider whether the explicit implementation is necessary or if the original approach with casting was preferable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/ai/timefold/solver/core/impl/neighborhood/stream/enumerating/uni/UniDatasetInstance.java
Show resolved
Hide resolved
...rc/main/java/ai/timefold/solver/core/impl/neighborhood/maybeapi/move/SwapMoveDefinition.java
Show resolved
Hide resolved
...ain/java/ai/timefold/solver/core/impl/neighborhood/maybeapi/move/ListSwapMoveDefinition.java
Show resolved
Hide resolved
|



Previously, Neighborhoods depended on joins on the left-hand side - these joins are, however, possibly massive. With large datasets, joining 100k entities with themselves would result in billions of entries per move. This is clearly not acceptable and would never scale.
The new approach only uses joins within
pick(), and these joins will be enumerated on-demand. Therefore the memory use of this mechanism will be minimized. (Although possibly still significant, especially for random selection.)The extra benefit of this approach is that it actually respects the order of entities and facts that was given by the user.
Further improvements to scalability are likely still possible, but will be introduced when the situation calls for it. (Which I expect it will.)