-
Notifications
You must be signed in to change notification settings - Fork 165
chore: refactor indexing to prepare for Neighborhoods #1942
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
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 indexing infrastructure to prepare for the Neighborhoods feature. The main changes include introducing a new ElementAwareArrayList collection that supports random access (at the cost of 10-15% slower performance compared to the linked list variant), and renaming IndexKeys to CompositeKey throughout the codebase for better clarity.
- Introduced
ElementAwareArrayListas a new indexing backend with random access support - Created
ListEntryinterface to unify entry types across different list implementations - Renamed
IndexKeystoCompositeKeyfor improved code clarity - Renamed
ElementAwareListtoElementAwareLinkedListfor consistency - Introduced
IndexerBackendinterface to support multiple backend implementations
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ElementAwareArrayList.java | New array-backed list with gap-tolerant removal and lazy compaction |
| ElementAwareLinkedList.java | Renamed from ElementAwareList, with Entry class moved inside |
| ListEntry.java | New common interface for list entries |
| IndexerBackend.java | New sealed interface for indexer backends |
| RandomAccessIndexerBackend.java | New indexer backend using ElementAwareArrayList |
| LinkedListIndexerBackend.java | Renamed from NoneIndexer, uses ElementAwareLinkedList |
| CompositeKey.java | Renamed from IndexKeys with updated documentation |
| BiCompositeKey.java | Renamed from TwoIndexKeys |
| MegaCompositeKey.java | Renamed from ManyIndexKeys |
| CompositeKeyRetriever.java | Renamed from ManyKeyRetriever |
| EqualsIndexer.java | Updated to use CompositeKey terminology and added asList support |
| ComparisonIndexer.java | Updated to use CompositeKey terminology and added asList support |
| Indexer.java | Updated interface with CompositeKey terminology and new asList method |
| IndexerFactory.java | Updated to use CompositeKey and LinkedListIndexerBackend |
| All test files | Updated to use new naming conventions |
Comments suppressed due to low confidence (1)
core/src/main/java/ai/timefold/solver/core/impl/util/ElementAwareLinkedList.java:333
- This method overrides ListEntry.getElement; it is advisable to add an Override annotation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/ai/timefold/solver/core/impl/util/ElementAwareArrayList.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/util/ElementAwareArrayList.java
Outdated
Show resolved
Hide resolved
|



This PR has two commits, which will be merged separately and I suggest they be reviewed separately as well. (I should have probably just made them two different PRs, but alas.)
First commit introduces a new collection that will serve as indexing backend for Neighborhoods. It is on average 10 - 15 % slower than the CS indexing backend, but gives us the power of direct access. This is a worthwhile trade-off, because Neighborhoods will need to access elements of the index randomly, and that would require copying the linked list to an array list, killing performance entirely.
Second commit renames "indexKeys" to "compositeKey" - this is a very straightforward name, and I have no idea why we were using the confusing name "indexKeys" until now. It's a composite key, let's call it that. It also does other small refactorings, as I was already there.
Microbenchmarks did not detect any regression, as expected; there are no actual runtime changes here.