Skip to content

Conversation

@ge0ffrey
Copy link
Contributor

@ge0ffrey ge0ffrey commented Dec 26, 2025

Before

    Constraint speakerUnavailableTimeslot(ConstraintFactory factory) {
        return factory.forEach(Talk.class)
                .join(Speaker.class,
                        filtering((talk, speaker) -> talk.hasSpeaker(speaker)
                                && speaker.getUnavailableTimeslots().contains(talk.getTimeslot())))
                .penalize(...)
                .asConstraint(...);
    }

After (bigO order of magnitude more scalable)

    Constraint speakerUnavailableTimeslot(ConstraintFactory factory) {
        return factory.forEach(Talk.class)
                .join(Speaker.class,
                        contain(Talk::getSpeakers, speaker -> speaker),
                        containedIn(Talk::getTimeslot, Speaker::getUnavailableTimeslots))
                .penalize(...)
                .asConstraint(...);
    }

Refactorings

  • Unify IndexerFactory.buildIndexer() into one switch across the joiner types
    -- 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.
  • Renamed EqualsIndexer to EqualIndexer because its Joiners.equal() and not Joiners.equals()

Open questions (added as todo's)

  • For Neighberhood, why track a left index?
  • For neighberhoods, why not flip the right bridge?
    I 'd image this code should feel exactly the same as scoring, but not left side
    so always flip basically?

Tasks

  • API
  • javadocs (API, ...)
  • Refactorings to plug it in
  • Unit tests
  • Indexer implementations
  • Use it in Quickstarts (conference scheduling, ...)
  • Docs

@ge0ffrey
Copy link
Contributor Author

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?
Copy link
Collaborator

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?
Copy link
Collaborator

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.

@triceo
Copy link
Collaborator

triceo commented Dec 26, 2025

(bigO order of magnitude more scalable)

In theory, yes. In practice, I've seen hashing overhead kill promising experiments in the past. Have you actually seen this improvement?

@ge0ffrey
Copy link
Contributor Author

(bigO order of magnitude more scalable)

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).
But for the cases where it matters - tags for example - I believe it can be a game changer.

@ge0ffrey
Copy link
Contributor Author

The tests succeed, but the conference scheduling PR doesn't

TimefoldAI/timefold-quickstarts#972

java.lang.IllegalStateException: Impossible state: the tuple (ai.timefold.solver.core.impl.util.CompositeListEntry@614ca71c) with composite key (BiCompositeKey[propertyA=2025-12-27T11:30, propertyB=[]]) doesn't exist in the indexer size = 4.
        at ai.timefold.solver.core.impl.bavet.common.index.ComparisonIndexer.getDownstreamIndexer(ComparisonIndexer.java:73)
        at ai.timefold.solver.core.impl.bavet.common.index.ComparisonIndexer.remove(ComparisonIndexer.java:61)
        at ai.timefold.solver.core.impl.bavet.common.AbstractIndexedJoinNode.updateRight(AbstractIndexedJoinNode.java:148)
        at ai.timefold.solver.core.impl.bavet.common.AbstractIndexedJoinNode.updateRight(AbstractIndexedJoinNode.java:22)
        at ai.timefold.solver.core.impl.bavet.common.tuple.RightTupleLifecycleImpl.update(RightTupleLifecycleImpl.java:20)
        at ai.timefold.solver.core.impl.bavet.common.tuple.AggregatedTupleLifecycle.update(AggregatedTupleLifecycle.java:25)
        at ai.timefold.solver.core.impl.bavet.common.StaticPropagationQueue.processAndClear(StaticPropagationQueue.java:115)
        at ai.timefold.solver.core.impl.bavet.common.StaticPropagationQueue.propagateUpdates(StaticPropagationQueue.java:94)
        at ai.timefold.solver.core.impl.bavet.NodeNetwork.settleLayer(NodeNetwork.java:65)
        at ai.timefold.solver.core.impl.bavet.NodeNetwork.settle(NodeNetwork.java:52)
        at ai.timefold.solver.core.impl.bavet.AbstractSession.settle(AbstractSession.java:63)

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants