fix: restore support for null values in sortable value ranges#2238
Draft
zepfred wants to merge 1 commit intoTimefoldAI:mainfrom
Draft
fix: restore support for null values in sortable value ranges#2238zepfred wants to merge 1 commit intoTimefoldAI:mainfrom
zepfred wants to merge 1 commit intoTimefoldAI:mainfrom
Conversation
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds coverage for scenarios where planning variables (basic and list) allow unassigned values while also using value sorting, ensuring the “unassigned/null” option is preserved through sorting and during construction heuristic solving.
Changes:
- Fix
NullAllowingValueRange.sort(...)to keep the null-allowing wrapper after sorting. - Add new test domain solution/entity pairs for sortable entities with
allowsUnassigned/allowsUnassignedValues. - Extend construction heuristic tests to verify solvers can prefer leaving variables unassigned when constraints penalize assignments.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/NullAllowingValueRange.java | Ensures sorting retains the null-allowing wrapper, keeping the unassigned option available. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/valuerange/NullAllowingValueRangeTest.java | Updates assertions to reflect null being present after sorting. |
| core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java | Adds constraint-stream-based tests verifying CH can keep basic/list variables unassigned when optimal. |
| core/src/test/java/ai/timefold/solver/core/testdomain/unassignedvar/sort/TestdataAllowsUnassignedSortableSolution.java | New test solution for basic sortable entity with unassigned variable support. |
| core/src/test/java/ai/timefold/solver/core/testdomain/unassignedvar/sort/TestdataAllowsUnassignedSortableEntity.java | New planning entity with @PlanningVariable(allowsUnassigned = true) and comparator sorting. |
| core/src/test/java/ai/timefold/solver/core/testdomain/list/unassignedvar/sort/TestdataAllowsUnassignedListSortableSolution.java | New test solution for list-variable sortable entity with unassigned values support. |
| core/src/test/java/ai/timefold/solver/core/testdomain/list/unassignedvar/sort/TestdataAllowsUnassignedListSortableEntity.java | New planning entity with @PlanningListVariable(allowsUnassignedValues = true) and comparator sorting. |
zepfred
changed the title
triceo
approved these changes
Apr 15, 2026
Collaborator
triceo
left a comment
triceo
left a comment
There was a problem hiding this comment.
LGTM after comments are resolved.
| .withConstraintProviderClass(PenalizeAssignedConstraintProvider.class) | ||
| .withPhases(new ConstructionHeuristicPhaseConfig() | ||
| .withTerminationConfig(new TerminationConfig().withStepCountLimit(3))); | ||
| solverConfig.getScoreDirectorFactoryConfig().setEasyScoreCalculatorClass(null); |
Collaborator
There was a problem hiding this comment.
Please remove this line.
If PlannerTestUtils.buildSolverConfig is forcing you to do this, don't use that method either.
| .withConstraintProviderClass(ListPenalizeAssignedConstraintProvider.class) | ||
| .withPhases(new ConstructionHeuristicPhaseConfig() | ||
| .withTerminationConfig(new TerminationConfig().withStepCountLimit(3))); | ||
| solverConfig.getScoreDirectorFactoryConfig().setEasyScoreCalculatorClass(null); |
| solverConfig.getScoreDirectorFactoryConfig().setEasyScoreCalculatorClass(null); | ||
| var problem = TestdataAllowsUnassignedListSortableSolution.generateSolution(1, 1, false); | ||
| var solution = PlannerTestUtils.solve(solverConfig, problem); | ||
| assertThat(solution).isNotNull(); |
Collaborator
There was a problem hiding this comment.
Arguably not necessary; not sure why we do this so often. Solution can never be null.
| } | ||
|
|
||
| Constraint penalizeAssigned(ConstraintFactory constraintFactory) { | ||
| return constraintFactory.forEach(TestdataAllowsUnassignedSortableEntity.class) |
Collaborator
There was a problem hiding this comment.
This will skip every null value. Shouldn't this be forEachIncludingUnassigned?
Otherwise the subsequent filter is useless.
triceo
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.