Skip to content

Commit

Permalink
Introduce ImmutableMap.Builder.buildKeepingLast.
Browse files Browse the repository at this point in the history 
This method is the same as the existing `.buildOrThrow` method, except when the same key appears more than once. In that case `buildOrThrow` throws an exception, while `buildKeepingLast` uses the last value that was associated with the key.

Relnotes:
  * A new method `ImmutableMap.Builder.buildKeepingLast()` keeps the last value for any given key rather than throwing an exception when a key appears more than once.
  * As a side-effect of the `buildKeepingLast()` change, the idiom `ImmutableList.copyOf(Maps.transformValues(map, function))` may produce different results if `function` has side-effects. (This is not recommended.)
PiperOrigin-RevId: 416035210
  • Loading branch information
@eamonnmcmanus
eamonnmcmanus authored and Google Java Core Libraries committed Dec 13, 2021
1 parent c98868c commit 68500b2
Show file tree
Hide file tree
Showing 18 changed files with 887 additions and 141 deletions.
3 changes: 2 additions & 1 deletion android/guava-tests/test/com/google/common/collect/ImmutableBiMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,8 @@ public void testDuplicateValues() {
ImmutableBiMap.copyOf(map);
fail();
} catch (IllegalArgumentException expected) {
assertThat(expected.getMessage()).contains("1");
assertThat(expected.getMessage()).containsMatch("1|2");
// We don't specify which of the two dups should be reported.
}
}

Expand Down
119 changes: 118 additions & 1 deletion android/guava-tests/test/com/google/common/collect/ImmutableMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;
Expand Down Expand Up @@ -388,6 +390,42 @@ public void testBuilder_orderEntriesByValue_usedTwiceFails() {
}
}

@GwtIncompatible // we haven't implemented this
public void testBuilder_orderEntriesByValue_keepingLast() {
ImmutableMap.Builder<String, Integer> builder =
new Builder<String, Integer>()
.orderEntriesByValue(Ordering.natural())
.put("three", 3)
.put("one", 1)
.put("five", 5)
.put("four", 3)
.put("four", 5)
.put("four", 4) // this should win because it's last
.put("two", 2);
assertMapEquals(
builder.buildKeepingLast(), "one", 1, "two", 2, "three", 3, "four", 4, "five", 5);
try {
builder.buildOrThrow();
fail("Expected exception from duplicate keys");
} catch (IllegalArgumentException expected) {
}
}

@GwtIncompatible // we haven't implemented this
public void testBuilder_orderEntriesByValue_keepingLast_builderSizeFieldPreserved() {
ImmutableMap.Builder<String, Integer> builder =
new Builder<String, Integer>()
.orderEntriesByValue(Ordering.natural())
.put("one", 1)
.put("one", 1);
assertMapEquals(builder.buildKeepingLast(), "one", 1);
try {
builder.buildOrThrow();
fail("Expected exception from duplicate keys");
} catch (IllegalArgumentException expected) {
}
}

public void testBuilder_withImmutableEntry() {
ImmutableMap<String, Integer> map =
new Builder<String, Integer>().put(Maps.immutableEntry("one", 1)).buildOrThrow();
Expand Down Expand Up @@ -559,7 +597,7 @@ public void testPuttingTheSameKeyTwiceThrowsOnBuild() {
Builder<String, Integer> builder =
new Builder<String, Integer>()
.put("one", 1)
.put("one", 1); // throwing on this line would be even better
.put("one", 1); // throwing on this line might be better but it's too late to change

try {
builder.buildOrThrow();
Expand All @@ -568,6 +606,85 @@ public void testPuttingTheSameKeyTwiceThrowsOnBuild() {
}
}

public void testBuildKeepingLast_allowsOverwrite() {
Builder<Integer, String> builder =
new Builder<Integer, String>()
.put(1, "un")
.put(2, "deux")
.put(70, "soixante-dix")
.put(70, "septante")
.put(70, "seventy")
.put(1, "one")
.put(2, "two");
ImmutableMap<Integer, String> map = builder.buildKeepingLast();
assertMapEquals(map, 1, "one", 2, "two", 70, "seventy");
}

// The java7 branch has different code depending on whether the entry indexes fit in a byte,
// short, or int. The small table in testBuildKeepingLast_allowsOverwrite will test the byte
// case. This method tests the short case.
public void testBuildKeepingLast_shortTable() {
Builder<Integer, String> builder = ImmutableMap.builder();
Map<Integer, String> expected = new LinkedHashMap<>();
for (int i = 0; i < 1000; i++) {
// Truncate to even key, so we have put(0, "0") then put(0, "1"). Half the entries are
// duplicates.
Integer key = i & ~1;
String value = String.valueOf(i);
builder.put(key, value);
expected.put(key, value);
}
ImmutableMap<Integer, String> map = builder.buildKeepingLast();
assertThat(map).hasSize(500);
assertThat(map).containsExactlyEntriesIn(expected).inOrder();
}

// This method tests the int case.
public void testBuildKeepingLast_bigTable() {
Builder<Integer, String> builder = ImmutableMap.builder();
Map<Integer, String> expected = new LinkedHashMap<>();
for (int i = 0; i < 200_000; i++) {
// Truncate to even key, so we have put(0, "0") then put(0, "1"). Half the entries are
// duplicates.
Integer key = i & ~1;
String value = String.valueOf(i);
builder.put(key, value);
expected.put(key, value);
}
ImmutableMap<Integer, String> map = builder.buildKeepingLast();
assertThat(map).hasSize(100_000);
assertThat(map).containsExactlyEntriesIn(expected).inOrder();
}

@GwtIncompatible // Pattern, Matcher
public void testBuilder_keepingLast_thenOrThrow() {
ImmutableMap.Builder<String, Integer> builder =
new Builder<String, Integer>()
.put("three", 3)
.put("one", 1)
.put("five", 5)
.put("four", 3)
.put("four", 5)
.put("four", 4) // this should win because it's last
.put("two", 2);
assertMapEquals(
builder.buildKeepingLast(), "three", 3, "one", 1, "five", 5, "four", 4, "two", 2);
try {
builder.buildOrThrow();
fail("Expected exception from duplicate keys");
} catch (IllegalArgumentException expected) {
// We don't really care which values the exception message contains, but they should be
// different from each other. If buildKeepingLast() collapsed duplicates, that might end up
// not being true.
Pattern pattern =
Pattern.compile("Multiple entries with same key: four=(.*) and four=(.*)");
assertThat(expected).hasMessageThat().matches(pattern);
Matcher matcher = pattern.matcher(expected.getMessage());
assertThat(matcher.matches()).isTrue();
assertThat(matcher.group(1)).isNotEqualTo(matcher.group(2));
}
}

public void testOf() {
assertMapEquals(ImmutableMap.of("one", 1), "one", 1);
assertMapEquals(ImmutableMap.of("one", 1, "two", 2), "one", 1, "two", 2);
Expand Down
22 changes: 21 additions & 1 deletion android/guava/src/com/google/common/collect/ImmutableBiMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,10 +448,30 @@ public ImmutableBiMap<K, V> buildOrThrow() {
if (size == 0) {
return of();
}
sortEntries();
if (valueComparator != null) {
if (entriesUsed) {
alternatingKeysAndValues = Arrays.copyOf(alternatingKeysAndValues, 2 * size);
}
sortEntries(alternatingKeysAndValues, size, valueComparator);
}
entriesUsed = true;
return new RegularImmutableBiMap<K, V>(alternatingKeysAndValues, size);
}

/**
* Throws {@link UnsupportedOperationException}. This method is inherited from {@link
* ImmutableMap.Builder}, but it does not make sense for bimaps.
*
* @throws UnsupportedOperationException always
* @deprecated This method does not make sense for bimaps and should not be called.
* @since NEXT
*/
@DoNotCall
@Deprecated
@Override
public ImmutableBiMap<K, V> buildKeepingLast() {
throw new UnsupportedOperationException("Not supported for bimaps");
}
}

/**
Expand Down
154 changes: 121 additions & 33 deletions android/guava/src/com/google/common/collect/ImmutableMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@
import java.io.Serializable;
import java.util.AbstractMap;
import java.util.Arrays;
import java.util.BitSet;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.SortedMap;
import javax.annotation.CheckForNull;
import org.checkerframework.checker.nullness.qual.Nullable;
Expand Down Expand Up @@ -337,7 +340,7 @@ public static <K, V> Builder<K, V> builderWithExpectedSize(int expectedSize) {
}

static void checkNoConflict(
boolean safe, String conflictDescription, Entry<?, ?> entry1, Entry<?, ?> entry2) {
boolean safe, String conflictDescription, Object entry1, Object entry2) {
if (!safe) {
throw conflictException(conflictDescription, entry1, entry2);
}
Expand Down Expand Up @@ -384,6 +387,11 @@ public static class Builder<K, V> {
@Nullable Object[] alternatingKeysAndValues;
int size;
boolean entriesUsed;
/**
* If non-null, a duplicate key we found in a previous buildKeepingLast() or buildOrThrow()
* call. A later buildOrThrow() can simply report this duplicate immediately.
*/
@Nullable DuplicateKey duplicateKey;

/**
* Creates a new builder. The returned builder is equivalent to the builder generated by {@link
Expand Down Expand Up @@ -498,10 +506,46 @@ Builder<K, V> combine(Builder<K, V> other) {
return this;
}

/*
* TODO(kevinb): Should build() and the ImmutableBiMap & ImmutableSortedMap
* versions throw an IllegalStateException instead?
*/
private ImmutableMap<K, V> build(boolean throwIfDuplicateKeys) {
if (throwIfDuplicateKeys && duplicateKey != null) {
throw duplicateKey.exception();
}
/*
* If entries is full, then this implementation may end up using the entries array
* directly and writing over the entry objects with non-terminal entries, but this is
* safe; if this Builder is used further, it will grow the entries array (so it can't
* affect the original array), and future build() calls will always copy any entry
* objects that cannot be safely reused.
*/
// localAlternatingKeysAndValues is an alias for the alternatingKeysAndValues field, except if
// we end up removing duplicates in a copy of the array.
@Nullable Object[] localAlternatingKeysAndValues;
int localSize = size;
if (valueComparator == null) {
localAlternatingKeysAndValues = alternatingKeysAndValues;
} else {
if (entriesUsed) {
alternatingKeysAndValues = Arrays.copyOf(alternatingKeysAndValues, 2 * size);
}
localAlternatingKeysAndValues = alternatingKeysAndValues;
if (!throwIfDuplicateKeys) {
// We want to retain only the last-put value for any given key, before sorting.
// This could be improved, but orderEntriesByValue is rather rarely used anyway.
localAlternatingKeysAndValues = lastEntryForEachKey(localAlternatingKeysAndValues, size);
if (localAlternatingKeysAndValues.length < alternatingKeysAndValues.length) {
localSize = localAlternatingKeysAndValues.length >>> 1;
}
}
sortEntries(localAlternatingKeysAndValues, localSize, valueComparator);
}
entriesUsed = true;
ImmutableMap<K, V> map =
RegularImmutableMap.create(localSize, localAlternatingKeysAndValues, this);
if (throwIfDuplicateKeys && duplicateKey != null) {
throw duplicateKey.exception();
}
return map;
}

/**
* Returns a newly-created immutable map. The iteration order of the returned map is the order
Expand All @@ -527,40 +571,84 @@ public ImmutableMap<K, V> build() {
* @throws IllegalArgumentException if duplicate keys were added
* @since 31.0
*/
@SuppressWarnings("unchecked")
public ImmutableMap<K, V> buildOrThrow() {
/*
* If entries is full, then this implementation may end up using the entries array
* directly and writing over the entry objects with non-terminal entries, but this is
* safe; if this Builder is used further, it will grow the entries array (so it can't
* affect the original array), and future build() calls will always copy any entry
* objects that cannot be safely reused.
*/
sortEntries();
entriesUsed = true;
return RegularImmutableMap.create(size, alternatingKeysAndValues);
return build(true);
}

void sortEntries() {
if (valueComparator != null) {
if (entriesUsed) {
alternatingKeysAndValues = Arrays.copyOf(alternatingKeysAndValues, 2 * size);
}
Entry<K, V>[] entries = new Entry[size];
for (int i = 0; i < size; i++) {
// requireNonNull is safe because the first `2*size` elements have been filled in.
entries[i] =
new AbstractMap.SimpleImmutableEntry<K, V>(
(K) requireNonNull(alternatingKeysAndValues[2 * i]),
(V) requireNonNull(alternatingKeysAndValues[2 * i + 1]));
/**
* Returns a newly-created immutable map, using the last value for any key that was added more
* than once. The iteration order of the returned map is the order in which entries were
* inserted into the builder, unless {@link #orderEntriesByValue} was called, in which case
* entries are sorted by value. If a key was added more than once, it appears in iteration order
* based on the first time it was added, again unless {@link #orderEntriesByValue} was called.
*
* @since NEXT
*/
public ImmutableMap<K, V> buildKeepingLast() {
return build(false);
}

static <V> void sortEntries(
@Nullable Object[] alternatingKeysAndValues, int size, Comparator<V> valueComparator) {
@SuppressWarnings({"rawtypes", "unchecked"})
Entry<Object, V>[] entries = new Entry[size];
for (int i = 0; i < size; i++) {
// requireNonNull is safe because the first `2*size` elements have been filled in.
Object key = requireNonNull(alternatingKeysAndValues[2 * i]);
@SuppressWarnings("unchecked")
V value = (V) requireNonNull(alternatingKeysAndValues[2 * i + 1]);
entries[i] = new AbstractMap.SimpleImmutableEntry<Object, V>(key, value);
}
Arrays.sort(
entries, 0, size, Ordering.from(valueComparator).onResultOf(Maps.<V>valueFunction()));
for (int i = 0; i < size; i++) {
alternatingKeysAndValues[2 * i] = entries[i].getKey();
alternatingKeysAndValues[2 * i + 1] = entries[i].getValue();
}
}

private @Nullable Object[] lastEntryForEachKey(
@Nullable Object[] localAlternatingKeysAndValues, int size) {
Set<Object> seenKeys = new HashSet<>();
BitSet dups = new BitSet(); // slots that are overridden by a later duplicate key
for (int i = size - 1; i >= 0; i--) {
Object key = requireNonNull(localAlternatingKeysAndValues[2 * i]);
if (!seenKeys.add(key)) {
dups.set(i);
}
Arrays.sort(
entries, 0, size, Ordering.from(valueComparator).onResultOf(Maps.<V>valueFunction()));
for (int i = 0; i < size; i++) {
alternatingKeysAndValues[2 * i] = entries[i].getKey();
alternatingKeysAndValues[2 * i + 1] = entries[i].getValue();
}
if (dups.isEmpty()) {
return localAlternatingKeysAndValues;
}
Object[] newAlternatingKeysAndValues = new Object[(size - dups.cardinality()) * 2];
for (int inI = 0, outI = 0; inI < size * 2; ) {
if (dups.get(inI >>> 1)) {
inI += 2;
} else {
newAlternatingKeysAndValues[outI++] =
requireNonNull(localAlternatingKeysAndValues[inI++]);
newAlternatingKeysAndValues[outI++] =
requireNonNull(localAlternatingKeysAndValues[inI++]);
}
}
return newAlternatingKeysAndValues;
}

static final class DuplicateKey {
private final Object key;
private final Object value1;
private final Object value2;

DuplicateKey(Object key, Object value1, Object value2) {
this.key = key;
this.value1 = value1;
this.value2 = value2;
}

IllegalArgumentException exception() {
return new IllegalArgumentException(
"Multiple entries with same key: " + key + "=" + value1 + " and " + key + "=" + value2);
}
}
}

Expand Down
Loading

0 comments on commit 68500b2

Please sign in to comment.