Skip to content

chore: add indictment support to Bavet's Node Network - #2528

Open
Christopher-Chianelli wants to merge 27 commits into
TimefoldAI:mainfrom
Christopher-Chianelli:feat/729
Open

Christopher-Chianelli wants to merge 27 commits into
TimefoldAI:mainfrom
Christopher-Chianelli:feat/729

Conversation

@Christopher-Chianelli

Copy link
Copy Markdown
Contributor

Each tuple now have an IndictmentSource field. This field is set to IndictmentSource.DISABLED when indictments are disabled, which is used to exit early and avoid work only required for indictments. ConstraintMatchPolicy have two new values:

  • ENABLED_WITHOUT_JUSTIFICATIONS_AND_INDICTMENTS this has the previous behaviour of ENABLED_WITHOUT_JUSTIFICATIONS; has constraint matches but no justifications nor indictments
  • ENABLED_WITHOUT_INDICTMENTS this includes justifications but not indictments

ENABLED was changed to include both justifications and indictments instead of just justifications. ENABLED_WITHOUT_JUSTIFICATIONS was changed to include indictments.

The actual work each node does is pretty simple. Do note that the attach IndictmentSource does not change after an insert:

  • forEach: if indictments are enabled, set the source of the tuple to a RootIndictmentSource (perhaps LeafIndictmentSource would be a better name) on insert. Retract and update do no extra work; retract removes the tuple (and hence the indictment source and potential constraint match), and update does not change the instance tracked.
  • filter: not a node, and does not affect what object is indicted.
  • map/expand/flatten: these creates new tuples from a source tuple, and simply copy the source's indictment source on insert. Update and retract do not need to do anything following the same reasoning as forEach`.
  • join: on insert, set the indictment source of the joined tuple to a JoinedIndictmentSource that have a reference to both the left and right tuples' indictment sources.
  • groupBy: on insert, set the indictment source of the group's tuple to an aggregation containing the indictment sources of the tuples inside the group. On update, remove the tuple's indictment source from the old group aggregation and add it to the new group aggregation. On retract, remove the tuple's indictment source from the aggregation.
  • distinct: this is groupBy in a trenchcoat and behaves similarly.
  • concat: behaves similarly to map/expand/flatten, since the left side and right side act independently and don't affect each other.
  • ifExists/ifNotExists: The most complex of the bunch. When indictments are enabled, it takes a drastically different path than normal. In particular, right tuple will propagate updates even if the corresponding left tuple is not update. This is to prevent constraint matches with stale indictments referring to removed/retracted entities/problem facts. Additional tuple iteration occurs for indexed ifExists nodes so the tuple's indictments are up to date when the left tuple changes. Moreover, since ifExists does not create a new tuple but still adds to the indictment, the same indictment source is used, but the indictment source's support map for the key corresponding to the ifExists node's id is updated.
  • precompute: Since it uses an independent node network, its node's ids do not correspond to the outer node network's node ids. As such, the indictment support map of the produced tuples must be aggregated along with the original indictment source.

Each constraint is now aware of the ids of the nodes that affect the constraint. It uses it when creating constraint matches to iterate through the support map so it does not indict objects from other constraints with ifExists that so happen to share the same tuple. Each IndictmentSource is iterated in a tree like matter to collect a set of unique indicted objects which is converted to a list inside the actual ConstraintMatch.

The vast majority of the changes were making all existing ConstraintStream tests aware of indictments. assertMatch was changed so it sets the indictment list to the justifications, which work for some but not all tests. For the other tests, withIndictedObjects is used to set the expected indicted objects for each match.

Note: I am aware of the stale "NOTE: By not propagating here, ..." comment in IfExistsNode and will remove it.
Note: I am aware that the code under if (testFiltering(...)) in IfExistsNode should do a branch depending on if indictments are enabled and call the right method (was written before the change to keep the indictments up to date and the two methods were not too different from each other).
Note: I am aware the carnality and storeSize methods added to Tuple can be removed; they are an artifact from when tuple cloning were used for ifExists's indictment enabled path.

@triceo triceo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First read-through. An elegant solution, was expecting this to be more code; not happy with some bits, though. Let's try to adjust if we can.

Before we move further though, I believe we need to have some clarity on how stable this is. We have the turtle tests - let's run them on this. (They may need some ad-hoc adjusting to be able to run with indictments.) If at the same time we can gather move speed data from them, we can compare to non-indictment runs and have a decent idea as to how much slower this will be.

Comment thread core/src/main/java/ai/timefold/solver/core/impl/bavet/common/TupleRecorder.java Outdated
@sonarqubecloud

Copy link
Copy Markdown

@Christopher-Chianelli

Copy link
Copy Markdown
Contributor Author

For benchmarking purposes, I added PreviewFeature.SOLVE_WITH_INDICTMENTS, which can be used to solve with indictments enabled.

This allows support infomation to be automatically
passed when a new tuple is created (ex: in map/expand).
…d, add notes about stale ifExists indictments
…oesn't update, make PrecomputeUni tests aware of indictments
… if exists, aggregate support for precompute, make BiPrecomputeTest Indictment aware

@triceo triceo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Final points for discussion.
  • Documentation is missing.
  • Sonar is worth checking.

*
* @return a string that can be used to identify the class of {@link #getIndictedObject()}
*/
IndictableTypeRef getIndictableTypeRef();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd just turn this to a String - the extra level of indirection doesn't seem to be necessary. People can easily convert class name to a string themselves, or choose a different ID entirely.

Avoids introducing a new type, and with it a naming conversation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was debating this with myself when implementing it. One advantage of this is that it is easier/more intuitive for the user to get the correct key when not implementing Indictable; do IndictableTypeRef.of(MyClass.class) instead of MyClass.class.getCanonicalName(); and it matches ConstraintRef, which is also a record of a single String component. No strong feelings for it though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO just the name IndictableTypeRef is something that turns a simple concept into rocket science. I would avoid it.

If we want to stay with the concept of this being any random ID, then naming is an issue. However, if we decide that this is just a class name, then we can simply make this String getClassName() and we have something everyone will immediately understand.

*
* @return the indicted object
*/
Object indictee();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explicitly mention that the identification of the object sits one level above. (It's not immediately obvious here.)

Comment on lines +15 to +16
* All included {@link ConstraintAnalysis} objects include full {@link ConstraintAnalysis#matches() match analysis},
* and {@link ScoreAnalysis#indictmentMap()} will be populated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we've decided to throw indictments even at people who never asked for them before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is FETCH_ALL; it would be confusing to have a FETCH_ALL_INCLUDING_INDICTMENTS, and would make more sense to have FETCH_ALL_EXCLUDING_INDICTMENTS which we could make the new default used for SolutionManager.analyze()`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at from one side, entirely correct reasoning. From the point of backwards compatibility, an unexpected addition that could break people if they iterate the JSON fields as opposed to accessing them as keys in a map.

The 100 % correct choice would have been to deprecate FETCH_ALL and introduce something else to take its place. Worth it though? Not entirely sure.

}
if (testFiltering(leftTuple, rightTuple)) {
incrementCounterRight(counter);
IndictmentSource.addSupport(getId(), counter.getTuple(), rightTuple);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I see it used, I question the name addSupport/removeSupport - it seems as if we're somehow adding or removing support for indictments, which is not true.

I'd consider renaming this concept. Auxilliary, corroborating, ancillary, aiding, ...

solverScope.setScoreDirector(scoreDirector);
solverScope.setProblemChangeDirector(new DefaultProblemChangeDirector<>(scoreDirector));
var moveThreadCount = resolveMoveThreadCount(true);
var previewFeaturesEnabled = solverConfig.getEnablePreviewFeatureSet();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like unnecessary change.

@triceo triceo added this to the v2.8.0-rc-1 milestone Sep 23, 2026

This branch was successfully deployed

1 active deployment
internal adac8c16 Deployed Sep 23, 2026 by Christopher-Chianelli via approval_required #4863
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