[COLLECTIONS-897] Add LexicographicPermutationIterator

[hextriclosan]

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:

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude for polishing documentation.
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[Copilot]

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 the ClassCastException at 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.

[hextriclosan]

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.

[hextriclosan]

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.

[garydgregory]

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!

[hextriclosan]

@garydgregory,

I've pushed doc and test changes. The implementation is unchanged, my reasoning is in the inline replies.

[garydgregory]

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.

[hextriclosan]

Fair point, the C++ reference doesn't belong here. Let me restate it in terms of this library.

Within Java Collections, Collections.binarySearch is the pattern: "The list must be sorted into ascending order according to the natural ordering of its elements... If it is not sorted, the results are undefined." The JDK documents the precondition and trusts the caller rather than sorting defensively.

Within Commons Collections, CollatingIterator in this same package does the same thing. It provides an ordered iteration over a collection of ordered iterators, states that in the first line of its javadoc, and never sorts its inputs. Unordered input gives unordered output. That's the precedent I should have cited from the start.

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 overAll(collection) that sorts a copy, keeping the start-anywhere behavior available on the constructor.

[garydgregory]

Hello @hextriclosan

Thank you for the update. Please see my 2 comments in the code.

WRT tests, how about adding:

• Explicit null-collection test: the constructor uses Objects.requireNonNull but there is no explicit assertThrows(NullPointerException.class, …) test.
• Equality when comparators differ, and when the iterators are at different positions.
• Hash-code stability/change after next().
• Behavior with all-equal elements: only one permutation should be produced.
• Defensive-copy expectation: a test that mutating a returned list does not affect subsequent next() calls.
• forEachRemaining() is inherited from AbstractIteratorTest is exercised, but a specific test for the lexicographic order of forEachRemaining would be nice.

TY!

[garydgregory]

Suggested change

	* Indicates if there are more permutation available.
	* Indicates if there are more permutations available.

[hextriclosan]

Fixed, thanks

[garydgregory]

The method returns the internal List reference that was used for the previous step. PermutationIterator does the same, so the behavior is consistent with the existing code base, but it means callers receive a mutable list that is no longer referenced internally after the call. Would a defensive copy or unmodifiable view would be safer?

Alternatively, the Javadoc could state that the returned List is mutable and should be treated as read-only.

[hextriclosan]

Switched to a defensive copy. Details in a separate comment.

[hextriclosan]

Hello @garydgregory,

Thank you for the review and the advice.

What changed:

• Javadoc polished
• Missing tests added
• next() now returns a defensive copy, covered by new tests

Two points where I would value your opinion:

1. next() returning a copy.
The copy is taken before the iterator advances, so the caller's list is detached regardless of what the successor computation does:

• pros: callers may modify the returned list freely, and the guarantee no longer depends on internal ordering.
• cons: one extra list allocation per permutation, and since PermutationIterator hands out its internal list, the two classes now differ internally.

2. Are equals/hashCode worth keeping?
They looked reasonable to me, but I have doubts:

• No other iterator in o.a.c.c.iterators overrides them, and neither do the JDK iterators. Identity semantics seem to be the convention.
• The hash code changes as the iterator advances, so an instance used as a HashMap key becomes unfindable after the first next().
• Two exhausted iterators built from different collections compare equal, since equality is defined on the comparator and the next permutation (see testEqualsAndHashCodeForExhaustedIterators). Defensible, as both will emit nothing further, but surprising.

Thanks again for taking the time.

[garydgregory]

Hello @hextriclosan, picking this low-hanging fruit: equals/hashCode aren't worth keeping for the first version of this class. Keeping the initial API surface small reduces maintenance, and the feature can be added later once there's a clear use case without worrying about backward compatibility. As you noted, other iterators don't support it, so adding it here would be inconsistent and invite questions about why we don't implement it everywhere, which we shouldn't take on.

[hextriclosan]

Hello @hextriclosan, picking this low-hanging fruit: equals/hashCode aren't worth keeping for the first version of this class. Keeping the initial API surface small reduces maintenance, and the feature can be added later once there's a clear use case without worrying about backward compatibility. As you noted, other iterators don't support it, so adding it here would be inconsistent and invite questions about why we don't implement it everywhere, which we shouldn't take on.

Hello @garydgregory,
I've removed equals/hashCode