[COLLECTIONS-897] Add LexicographicPermutationIterator - #721
[COLLECTIONS-897] Add LexicographicPermutationIterator#721hextriclosan wants to merge 3 commits into
Conversation
Add an Iterator<List<E>> that generates the permutations of a collection in lexicographical order, complementing PermutationIterator, which uses the Steinhaus-Johnson-Trotter ordering. Elements are ordered by their natural ordering, or by a Comparator supplied to the two-argument constructor, which also allows permuting elements that do not implement Comparable. Each call to next() advances by the standard next-permutation step: locate the pivot, swap it with its successor, then reverse the descending tail. Equal elements are not distinguished, so an input with duplicates yields fewer than n! permutations. An empty collection yields exactly one empty list, as 0! = 1. remove() is unsupported. Comparator dispatch follows the java.util.TreeMap pattern of testing the comparator field for null on each comparison; benchmarking showed no measurable difference against normalizing null to Comparator.naturalOrder() in the constructor. Tests extend AbstractIteratorTest to cover the Iterator contract, and add cases for lexicographical exhaustivity, duplicate handling, custom and reverse comparators, non-Comparable elements, stream traversal, exhaustion, and equals/hashCode.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Iterator<List<E>> implementation that generates permutations in lexicographic order (optionally using a provided Comparator), complementing the existing Steinhaus–Johnson–Trotter-based PermutationIterator.
Changes:
- Added
LexicographicPermutationIterator<E>implementing next-permutation lexicographic advancement. - Added a comprehensive JUnit test suite for iterator contract behavior and key permutation scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/collections4/iterators/LexicographicPermutationIterator.java | Adds new iterator that generates permutations using lexicographic next-permutation logic (with optional comparator). |
| src/test/java/org/apache/commons/collections4/iterators/LexicographicPermutationIteratorTest.java | Adds unit tests for ordering, duplicates, custom comparators, non-Comparable elements, iterator contract, and stream traversal. |
Suppressed comments (1)
src/test/java/org/apache/commons/collections4/iterators/LexicographicPermutationIteratorTest.java:274
- If the iterator sorts the input to establish the first lexicographic permutation, then providing non-Comparable elements without a comparator will fail during construction (when sorting), not on the first
next()call. Update the test to assert theClassCastExceptionat construction time to match the iterator’s initialization behavior.
@Test
void testNonComparableElementsThrow() {
final Iterator<List<NonComparableObject<Character>>> permutationIterator = new LexicographicPermutationIterator<>(
Arrays.asList(
new NonComparableObject<>('A'),
new NonComparableObject<>('B')));
assertTrue(permutationIterator.hasNext());
assertThrows(ClassCastException.class, permutationIterator::next);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public LexicographicPermutationIterator(final Collection<? extends E> collection, final Comparator<? super E> comparator) { | ||
| Objects.requireNonNull(collection, "collection"); | ||
| nextPermutation = new ArrayList<>(collection); | ||
| this.comparator = comparator; | ||
| } |
There was a problem hiding this comment.
The described behavior is real and the javadoc was actively misleading about it. I've pushed doc and test changes rather than the sort, and here's the reasoning.
The truncation is intentional. This class is the iterator form of the classic next-permutation step: it starts wherever the input puts it and advances to the smallest arrangement greater than the current one, exactly as std::next_permutation does in C++. Sorted input is a precondition for enumerating the full set, in the same way sorted input is a precondition for Collections.binarySearch. That precondition simply wasn't documented.
I'd rather not sort in the constructor, because sorting is not a neutral addition. It removes a capability that cannot be recovered:
- Resuming. A caller that persisted the last arrangement it processed can construct an iterator from it and carry on. With a constructor sort there is no way to express "start here".
- Splitting the work. The permutation space can be divided across threads or machines by handing each worker a different starting arrangement. Same problem.
A caller who wants the complete set can always sort before constructing, and that is one line at the call site. A caller who wants to start partway through has no recourse if the constructor sorts. The asymmetry is what decides it for me: preserving the given order is strictly the more expressive of the two designs.
There's also a ready alternative for callers who want all n! without thinking about order, namely PermutationIterator, which reaches every arrangement from any starting point because Steinhaus-Johnson-Trotter enumerates the whole group. The two classes are genuinely different tools, and I've added a note making the differences explicit so the @see link stops implying they're interchangeable.
| void testCustomComparator() { | ||
| final Iterator<List<Character>> permutationIterator = new LexicographicPermutationIterator<>(Arrays.asList('C', 'B', 'A'), | ||
| Comparator.reverseOrder()); | ||
|
|
There was a problem hiding this comment.
Good news on this one: the test already covers what you're after, and I can show it. I patched compareElements to ignore the comparator and ran the suite. testCustomComparator fails, and so does testCustomComparatorWithNonComparableObjects.
The reason ['C','B','A'] works as input is that it's the maximum under natural ordering. An implementation that ignored the comparator would find no pivot, terminate after a single permutation, and fail on the second assertTrue. The test therefore separates "comparator honoured, 6 permutations in reverse-lexicographic order" from "comparator ignored, 1 permutation".
You're right that neither test exercised input unsorted under its own comparator. I've added testUnsortedCollectionStartsAtGivenArrangementWithComparator for exactly that, passing ['B','C','A'] with reverseOrder() and asserting the four permutations that follow it. Starting at the given arrangement is the intended contract here rather than a bug, for the reasons in the other thread, and that test now pins it.
|
Hello @hextriclosan Thank you for the PR. Please review each Copiot comment and address them in comments here, in the code, or both. If you update the code, do make sure new unit tests cover all execution paths. TY! |
|
I've pushed doc and test changes. The implementation is unchanged, my reasoning is in the inline replies. |
|
C++? That's completely irrelevant. The code should only care about (1) the specifics of Commons Collections, and (2) how we extend Java Collections. Whatever happens in a C++ library doesn't come into play. |
|
Fair point, the C++ reference doesn't belong here. Let me restate it in terms of this library. Within Java Collections, Within Commons Collections, The substantive reason is about what each design permits. A caller who wants the complete set can sort before constructing, one line at the call site. A caller who wants to resume from a previously reached arrangement, or to split the permutation space across workers by giving each a different starting point, has no recourse if the constructor sorts. Sorting is not recoverable from outside the class, so the version that preserves the given order is strictly the more capable of the two. What I've pushed documents the precondition, since the javadoc previously contradicted it, and adds tests pinning it for both natural ordering and a supplied comparator. If you'd still rather the complete set be the default, I'd suggest a static factory such as |
Add an Iterator<List> that generates the permutations of a collection in lexicographical order, complementing PermutationIterator, which uses the Steinhaus-Johnson-Trotter ordering.
Elements are ordered by their natural ordering, or by a Comparator supplied to the two-argument constructor, which also allows permuting elements that do not implement Comparable. Each call to next() advances by the standard next-permutation step: locate the pivot, swap it with its successor, then reverse the descending tail. Equal elements are not distinguished, so an input with duplicates yields fewer than n! permutations. An empty collection yields exactly one empty list, as 0! = 1. remove() is unsupported.
Comparator dispatch follows the java.util.TreeMap pattern of testing the comparator field for null on each comparison; benchmarking showed no measurable difference against normalizing null to Comparator.naturalOrder() in the constructor.
Tests extend AbstractIteratorTest to cover the Iterator contract, and add cases for lexicographical exhaustivity, duplicate handling, custom and reverse comparators, non-Comparable elements, stream traversal, exhaustion, and equals/hashCode.
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.