Add MultiValuedMap.inverted() by paulk-asert · Pull Request #665 · apache/commons-collections

paulk-asert · 2025-12-12T11:29:15Z

This PR adds an inverse method into the following multimap implementations:

ArrayListValuedHashMap
ArrayListValuedLinkedHashMap
HashSetValuedHashMap
HashSetValuedLinkedHashMap

There is also an inverseInto method added to MultiMapUtils to handle the case of a heterogenous collector.

As well as a test case to the relevant source files. Decorated multimaps throw UnsupportedOperationException by default.

garydgregory · 2025-12-12T11:36:41Z

Hi @paulk-asert

Thank you for the PR.

You'll want to run 'mvn' by itself and fix errors before you push. That will run the default goal which includes all our checks.

paulk-asert · 2025-12-12T12:10:36Z

Hi @paulk-asert
Thank you for the PR.
You'll want to run 'mvn' by itself and fix errors before you push. That will run the default goal which includes all our checks.

@garydgregory Should be fixed. Sorry for the earlier noise.

garydgregory

Hello @paulk-asert
Thank you for the PR :)
I have a couple of comments.

garydgregory · 2025-12-13T20:56:52Z

src/main/java/org/apache/commons/collections4/MultiValuedMap.java

+     *
+     * @return the inverse MultiValuedMap
+     */
+    default MultiValuedMap<V, K> inverse() {


Java 21 has java.util.List.reversed(), so how about inverted() to match the same naming pattern?

garydgregory · 2025-12-13T21:01:22Z

src/main/java/org/apache/commons/collections4/multimap/ArrayListValuedHashMap.java

+        final ArrayListValuedHashMap<V, K> result = new ArrayListValuedHashMap<>();
+        MultiMapUtils.inverseInto(this, result);
+        return result;


No need to be so verbose here and for all implementations IMO:

Suggested change

final ArrayListValuedHashMap<V, K> result = new ArrayListValuedHashMap<>();

MultiMapUtils.inverseInto(this, result);

return result;

return MultiMapUtils.inverseInto(this, new ArrayListValuedHashMap<>());

sebbASF · 2025-12-15T09:09:17Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+     * @param <K> the dest key type
+     * @param <V> the dest value type
+     * @param <M> the dest multimap
+     * @return the destination


This does not make sense to me. What is the destination?

I think the Javadoc needs to make clear that the method creates a new map with the inverted keys

The utility method doesn't create a new multimap but stores inverted mappings in the supplied collector (destination) which is also returned. The inverted method implementations supply a new multimap as the collector, but it doesn't need to be the same type or be new (to allow aggregation) when using the utility method. I could try to use "collector" everywhere and avoid "destination" if you think that would be clearer?

I think collector would be clearer here.
For the return comment, it could say 'the updated collector'

Also it would help to document the valid types for source and collector.

Can you clarify what you are after for source and collector? The types/generics information isn't sufficient?

sebbASF · 2025-12-15T09:13:01Z

src/main/java/org/apache/commons/collections4/MultiValuedMap.java

    Collection<V> get(K key);

+    /**
+     * Returns the inverse MultiValuedMap with inverted mappings.


'the' => 'a new'

'the' implies that there is only one such map

In mathematical jargon, the thing being returned is the "inverse multimap". I see what you are saying that those not knowing the math jargon might confuse that wording with meaning just one. Then again, totally forgoing the math jargon might be less clear for those that are aware of the term. I'll try to think of a middle ground.

The comment refers to 'the MultiValueMap' which is a class, not a mathematical concept.

Yes, it is the name of the abstraction used to represent a multimap, but I understand where you are coming from and I'll see what improvement to the wording I can think of.

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

garydgregory · 2025-12-15T14:38:34Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+     * @return the updated collector
+     */
+    public static <K, V, M extends MultiValuedMap<K, V>>
+    M invertInto(MultiValuedMap<? extends V, ? extends K> source, M collector) {


Hi @paulk-asert
A few items here:

I'm wondering if a simpler name like invert would be better since we don't usually see Into in method names. We know from the param names what the source and target are. Using "Into" doesn't add value for me.

The parameter name collector might be confusing because a Collector in Java is something else but kind of related. Since we have source, having dest, destination or target would be better IMO.

Maybe invertMerge would better describe what the method does?

Also rather than source and dest, would input and output be clearer?

I don't like the abbreviation 'dest'; it's best to spell names in full as not all users will have English as a first language.

Guava has similar methods to what is proposed here. It uses invertFrom as the utility method and inverse as the instance method. I think invert and invertMerge have some benefits. I'll ponder a change and convert collector to target or similar. Hmmm.. CollectionUtils#collect uses inputCollection and outputCollection. Maybe that convention is the way to go.

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

sebbASF · 2025-12-15T15:04:54Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+    /**
+     * A utility method to invert the mappings from a source MultiValuedMap
+     * to a collector MultiValuedMap. If you are trying to create an
+     * inverse multimap of the same kind as the original, consider using


'same kind' is ambiguous.

AIUI, this method returns a MultiValuedMap; is that not the 'same kind' of map as the original?

It's not clear when one should consider using #inverted()

Okay, the intention is that when calling inverted you have the following input -> output class relationships:

ArrayListValuedHashMap -> ArrayListValuedHashMap
ArrayListValuedLinkedHashMap -> ArrayListValuedLinkedHashMap
LinkedHashSetValuedLinkedHashMap -> LinkedHashSetValuedLinkedHashMap
LinkedHashSetValuedHashMap -> LinkedHashSetValuedHashMap

I'll try to think of better wording.

sebbASF · 2025-12-15T15:08:02Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+     * @param collector add value-to-key mappings here
+     * @param <K> the collector key type
+     * @param <V> the collector value type
+     * @param <M> the collector multimap


What are the valid types for M?
Are all multimap types suitable?

I changed the wording. Let me know if it makes sense or still sounds confusing to you?

sebbASF · 2025-12-16T12:57:45Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+     * @param outputMultimap add value-to-key mappings here
+     * @param <K> the outputMultimap key type
+     * @param <V> the outputMultimap value type
+     * @param <M> the outputMultimap multimap with key and value types reversed compare to inputMultimap


compare to => compared with

sebbASF · 2025-12-16T13:01:19Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java


+    /**
+     * A utility method to invert the mappings from an inputMultimap
+     * and add them to an outputMultimap. Some {@code MultiValuedMap}


inputMultimap => input MultiValuedMap
outputMultimap => output MultiValuedMap

inputMultimap and outputMultimap are not Java types, and AFAICT they must both be MultiValuedMap, rather than just Multimap

sebbASF · 2025-12-16T13:07:10Z

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

+     * and add them to an outputMultimap. Some {@code MultiValuedMap}
+     * implementations support the {@link MultiValuedMap#inverted()} method.
+     * These typically return a new multimap instance with the same class as the original.
+     * Consider using this method instead if you wish to supply a particular


'this method' is ambiguous here: does it refer to the method inverted() which has just been described, or the method in this class?

I think it would be clearer to describe the usage for the current class method before explaining that there are alternatives.

garydgregory

LGTM.

paulk-asert force-pushed the multimapInverse branch from 3cff965 to 355f42f Compare December 12, 2025 12:06

garydgregory requested changes Dec 13, 2025

View reviewed changes

paulk-asert force-pushed the multimapInverse branch from 355f42f to df560b8 Compare December 13, 2025 22:11

garydgregory changed the title ~~provide an inverse method for multimaps~~ Add MultiValuedMap.inverted() Dec 14, 2025

sebbASF requested changes Dec 15, 2025

View reviewed changes

sebbASF reviewed Dec 15, 2025

View reviewed changes

src/main/java/org/apache/commons/collections4/MultiMapUtils.java Show resolved Hide resolved

garydgregory reviewed Dec 15, 2025

View reviewed changes

sebbASF requested changes Dec 15, 2025

View reviewed changes

paulk-asert force-pushed the multimapInverse branch from dfd9f7f to 0835f9e Compare December 16, 2025 05:10

sebbASF reviewed Dec 16, 2025

View reviewed changes

sebbASF requested changes Dec 16, 2025

View reviewed changes

provide an inverse method for multimaps

6f0275e

paulk-asert force-pushed the multimapInverse branch from 0835f9e to 6f0275e Compare December 18, 2025 04:08

garydgregory approved these changes Dec 19, 2025

View reviewed changes

garydgregory merged commit d7a8201 into apache:master Dec 19, 2025
9 checks passed

garydgregory added a commit that referenced this pull request Dec 19, 2025

Add MultiValuedMap.inverted() #665

e605b58

Conversation

paulk-asert commented Dec 12, 2025

Uh oh!

garydgregory commented Dec 12, 2025

Uh oh!

paulk-asert commented Dec 12, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulk-asert Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

paulk-asert Dec 16, 2025 •

edited

Loading