-
Notifications
You must be signed in to change notification settings - Fork 169
feat: Joiners.contain() and Joiners.containedIn() #1999
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
|
Implements part of #8 |
| // (<A, B> becomes <B, A>.) | ||
| // TODO Does the requiresRandomAccess check make sense? | ||
| // Shouldn't a right bridge always flip, even if there is no left bridge? | ||
| // TODO For neighborhoods, why create a left bridge index and keep it up to date at all? |
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.
We do not create left index in Neighborhoods. The comment below says that.
| // Note that if creating indexer for a right bridge node, the joiner type has to be flipped. | ||
| // (<A, B> becomes <B, A>.) | ||
| // TODO Does the requiresRandomAccess check make sense? | ||
| // Shouldn't a right bridge always flip, even if there is no left bridge? |
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.
No. The right bridge doesn't flip for Neighborhoods, because it's queried from the left side. <A, B> never becomes <B, A>, and therefore the index doesn't flip either.
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/index/ContainIndexer.java
Outdated
Show resolved
Hide resolved
In theory, yes. In practice, I've seen hashing overhead kill promising experiments in the past. Have you actually seen this improvement? |
No. We'll need to benchmark it. This PR will allow us to do exactly that. I even suspect that sometimes you'll want to prefer using filtering(a.contains(b)) over joiners.contain(a, b). |
|
The tests succeed, but the conference scheduling PR works with this code: But fails with this code (containedIn after greaterThan): The exception: Notice how it's the ComparisonIndexer that cannot find the Contain(edIn)Indexer during removal of a talk. |
|
|
The conference scheduling dataset with 15 talks does not show a move speed evaluation improvement when two constraints are replaced to use contain() and containedIn(). It's a scalability improvement, not a performance improvement, so I presume we need to benchmark with far bigger datasets to confirm it's usefullness. |
|
School timetabling scheduling POC benchmark of contains vs filtering Before 2026-01-02 17:31:42,151 INFO Problem scale: entity count (100), variable count (200), approximate value count (31), approximate problem scale (4.065119 × 10^217). After 2026-01-02 17:32:46,053 INFO Problem scale: entity count (100), variable count (200), approximate value count (31), approximate problem scale (4.065119 × 10^217). |



Before
After (bigO order of magnitude more scalable)
Refactorings
-- Do not create single/composite KeyRetriever in EqualsIndexer and ComparisonIndexer to unify to one constructor.
-- Extract KeyRetriever creation to IndexerFactory (that class is responsibility for those key tricks)
-- Implement equal joiner flip() as no-op, so all right bridges indexer can just get a flip.
Open questions (added as todo's)
I 'd image this code should feel exactly the same as scoring, but not left side
so always flip basically?
Tasks