Skip to content

Commit 1073832

Browse files
findepifindepi
committed
Reimplement CharSequenceMap to obey Map contract
The previous `CharSequenceMap` implementation did not follow general `Map` contract. For example, `Map.keySet()` should be a live view of the keys in the map, but it was a copy. This commit reimplements the class so that it obeys the map contract, while maintaining the fundamental objective of the class, which is to compare `CharSequence` keys by the value of char sequences they describe.
1 parent 7912035 commit 1073832

File tree

7 files changed

+194
-227
lines changed

7 files changed

+194
-227
lines changed

‎.palantir/revapi.yml

+159
Original file line numberDiff line numberDiff line change
@@ -1063,6 +1063,159 @@ acceptedBreaks:
10631063
new: "method void org.apache.iceberg.encryption.PlaintextEncryptionManager::<init>()"
10641064
justification: "Deprecations for 1.6.0 release"
10651065
"1.6.0":
1066+
org.apache.iceberg:iceberg-api:
1067+
- code: "java.class.noLongerImplementsInterface"
1068+
old: "class org.apache.iceberg.util.CharSequenceMap<V extends java.lang.Object>"
1069+
new: "class org.apache.iceberg.util.CharSequenceMap"
1070+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1071+
- code: "java.class.nowAbstract"
1072+
old: "class org.apache.iceberg.util.CharSequenceMap<V extends java.lang.Object>"
1073+
new: "class org.apache.iceberg.util.CharSequenceMap"
1074+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1075+
- code: "java.generics.formalTypeParameterRemoved"
1076+
old: "class org.apache.iceberg.util.CharSequenceMap<V extends java.lang.Object>"
1077+
new: "class org.apache.iceberg.util.CharSequenceMap"
1078+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1079+
- code: "java.method.removed"
1080+
old: "method <K, V> java.util.Map.Entry<K, V> java.util.Map<K, V>::entry(K,\
1081+
\ V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1082+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1083+
- code: "java.method.removed"
1084+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::copyOf(java.util.Map<?\
1085+
\ extends K, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1086+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1087+
- code: "java.method.removed"
1088+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of() @ org.apache.iceberg.util.CharSequenceMap<V>"
1089+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1090+
- code: "java.method.removed"
1091+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1092+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1093+
- code: "java.method.removed"
1094+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V)\
1095+
\ @ org.apache.iceberg.util.CharSequenceMap<V>"
1096+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1097+
- code: "java.method.removed"
1098+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1099+
\ K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1100+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1101+
- code: "java.method.removed"
1102+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1103+
\ K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1104+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1105+
- code: "java.method.removed"
1106+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1107+
\ K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1108+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1109+
- code: "java.method.removed"
1110+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1111+
\ K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1112+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1113+
- code: "java.method.removed"
1114+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1115+
\ K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1116+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1117+
- code: "java.method.removed"
1118+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1119+
\ K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1120+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1121+
- code: "java.method.removed"
1122+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1123+
\ K, V, K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1124+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1125+
- code: "java.method.removed"
1126+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
1127+
\ K, V, K, V, K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1128+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1129+
- code: "java.method.removed"
1130+
old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::ofEntries(java.util.Map.Entry<?\
1131+
\ extends K, ? extends V>[]) @ org.apache.iceberg.util.CharSequenceMap<V>"
1132+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1133+
- code: "java.method.removed"
1134+
old: "method V java.util.Map<K, V>::compute(K, java.util.function.BiFunction<?\
1135+
\ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1136+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1137+
- code: "java.method.removed"
1138+
old: "method V java.util.Map<K, V>::computeIfAbsent(K, java.util.function.Function<?\
1139+
\ super K, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1140+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1141+
- code: "java.method.removed"
1142+
old: "method V java.util.Map<K, V>::computeIfPresent(K, java.util.function.BiFunction<?\
1143+
\ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1144+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1145+
- code: "java.method.removed"
1146+
old: "method V java.util.Map<K, V>::getOrDefault(java.lang.Object, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1147+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1148+
- code: "java.method.removed"
1149+
old: "method V java.util.Map<K, V>::merge(K, V, java.util.function.BiFunction<?\
1150+
\ super V, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1151+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1152+
- code: "java.method.removed"
1153+
old: "method V java.util.Map<K, V>::putIfAbsent(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1154+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1155+
- code: "java.method.removed"
1156+
old: "method V java.util.Map<K, V>::replace(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1157+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1158+
- code: "java.method.removed"
1159+
old: "method V org.apache.iceberg.util.CharSequenceMap<V>::computeIfAbsent(java.lang.CharSequence,\
1160+
\ java.util.function.Supplier<V>)"
1161+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1162+
- code: "java.method.removed"
1163+
old: "method V org.apache.iceberg.util.CharSequenceMap<V>::get(java.lang.Object)"
1164+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1165+
- code: "java.method.removed"
1166+
old: "method V org.apache.iceberg.util.CharSequenceMap<V>::put(java.lang.CharSequence,\
1167+
\ V)"
1168+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1169+
- code: "java.method.removed"
1170+
old: "method V org.apache.iceberg.util.CharSequenceMap<V>::remove(java.lang.Object)"
1171+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1172+
- code: "java.method.removed"
1173+
old: "method boolean java.util.Map<K, V>::remove(java.lang.Object, java.lang.Object)\
1174+
\ @ org.apache.iceberg.util.CharSequenceMap<V>"
1175+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1176+
- code: "java.method.removed"
1177+
old: "method boolean java.util.Map<K, V>::replace(K, V, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
1178+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1179+
- code: "java.method.removed"
1180+
old: "method boolean org.apache.iceberg.util.CharSequenceMap<V>::containsKey(java.lang.Object)"
1181+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1182+
- code: "java.method.removed"
1183+
old: "method boolean org.apache.iceberg.util.CharSequenceMap<V>::containsValue(java.lang.Object)"
1184+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1185+
- code: "java.method.removed"
1186+
old: "method boolean org.apache.iceberg.util.CharSequenceMap<V>::isEmpty()"
1187+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1188+
- code: "java.method.removed"
1189+
old: "method int org.apache.iceberg.util.CharSequenceMap<V>::size()"
1190+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1191+
- code: "java.method.removed"
1192+
old: "method java.util.Collection<V> org.apache.iceberg.util.CharSequenceMap<V>::values()"
1193+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1194+
- code: "java.method.removed"
1195+
old: "method java.util.Set<java.lang.CharSequence> org.apache.iceberg.util.CharSequenceMap<V>::keySet()"
1196+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1197+
- code: "java.method.removed"
1198+
old: "method java.util.Set<java.util.Map.Entry<java.lang.CharSequence, V>> org.apache.iceberg.util.CharSequenceMap<V>::entrySet()"
1199+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1200+
- code: "java.method.removed"
1201+
old: "method void java.util.Map<K, V>::forEach(java.util.function.BiConsumer<?\
1202+
\ super K, ? super V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1203+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1204+
- code: "java.method.removed"
1205+
old: "method void java.util.Map<K, V>::replaceAll(java.util.function.BiFunction<?\
1206+
\ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
1207+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1208+
- code: "java.method.removed"
1209+
old: "method void org.apache.iceberg.util.CharSequenceMap<V>::clear()"
1210+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1211+
- code: "java.method.removed"
1212+
old: "method void org.apache.iceberg.util.CharSequenceMap<V>::putAll(java.util.Map<?\
1213+
\ extends java.lang.CharSequence, ? extends V>)"
1214+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
1215+
- code: "java.method.returnTypeChanged"
1216+
old: "method <T> org.apache.iceberg.util.CharSequenceMap<T> org.apache.iceberg.util.CharSequenceMap<V>::create()"
1217+
new: "method <V> java.util.Map<java.lang.CharSequence, V> org.apache.iceberg.util.CharSequenceMap::create()"
1218+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
10661219
org.apache.iceberg:iceberg-common:
10671220
- code: "java.method.removed"
10681221
old: "method <T> org.apache.iceberg.common.DynFields.StaticField<T> org.apache.iceberg.common.DynFields.Builder::buildStaticChecked()\
@@ -1139,6 +1292,12 @@ acceptedBreaks:
11391292
\ java.lang.String>, java.lang.String, java.lang.String, java.lang.String,\
11401293
\ java.lang.String, java.lang.String)"
11411294
justification: "Removing deprecated code"
1295+
- code: "java.method.returnTypeChanged"
1296+
old: "method <T extends org.apache.iceberg.StructLike> org.apache.iceberg.util.CharSequenceMap<org.apache.iceberg.deletes.PositionDeleteIndex>\
1297+
\ org.apache.iceberg.deletes.Deletes::toPositionIndexes(org.apache.iceberg.io.CloseableIterable<T>)"
1298+
new: "method <T extends org.apache.iceberg.StructLike> java.util.Map<java.lang.CharSequence,\
1299+
\ org.apache.iceberg.deletes.PositionDeleteIndex> org.apache.iceberg.deletes.Deletes::toPositionIndexes(org.apache.iceberg.io.CloseableIterable<T>)"
1300+
justification: "CharSequenceMap did not fulfill Map contract and was reimplemented"
11421301
- code: "java.method.returnTypeChanged"
11431302
old: "method org.apache.iceberg.BaseMetastoreTableOperations.CommitStatus org.apache.iceberg.BaseMetastoreTableOperations::checkCommitStatus(java.lang.String,\
11441303
\ org.apache.iceberg.TableMetadata)"

‎api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java

+5-201
Original file line numberDiff line numberDiff line change
@@ -18,208 +18,12 @@
1818
*/
1919
package org.apache.iceberg.util;
2020

21-
import java.io.Serializable;
22-
import java.util.Collection;
2321
import java.util.Map;
24-
import java.util.Objects;
25-
import java.util.Set;
26-
import java.util.function.Supplier;
27-
import java.util.stream.Collectors;
28-
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
29-
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
22+
import java.util.TreeMap;
3023

31-
/**
32-
* A map that uses char sequences as keys.
33-
*
34-
* <p>This implementation wraps provided keys into {@link CharSequenceWrapper} for consistent
35-
* hashing and equals behavior. This ensures that objects of different types that represent the same
36-
* sequence of characters are treated as equal keys in the map.
37-
*
38-
* <p>Note: This map is not designed for concurrent modification by multiple threads. However, it
39-
* supports safe concurrent reads, assuming there are no concurrent writes.
40-
*
41-
* <p>Note: This map does not support null keys.
42-
*
43-
* @param <V> the type of values
44-
*/
45-
public class CharSequenceMap<V> implements Map<CharSequence, V>, Serializable {
46-
47-
private static final long serialVersionUID = 1L;
48-
private static final ThreadLocal<CharSequenceWrapper> WRAPPERS =
49-
ThreadLocal.withInitial(() -> CharSequenceWrapper.wrap(null));
50-
51-
private final Map<CharSequenceWrapper, V> wrapperMap;
52-
53-
private CharSequenceMap() {
54-
this.wrapperMap = Maps.newHashMap();
55-
}
56-
57-
public static <T> CharSequenceMap<T> create() {
58-
return new CharSequenceMap<>();
59-
}
60-
61-
@Override
62-
public int size() {
63-
return wrapperMap.size();
64-
}
65-
66-
@Override
67-
public boolean isEmpty() {
68-
return wrapperMap.isEmpty();
69-
}
70-
71-
@Override
72-
public boolean containsKey(Object key) {
73-
if (key instanceof CharSequence) {
74-
CharSequenceWrapper wrapper = WRAPPERS.get();
75-
boolean result = wrapperMap.containsKey(wrapper.set((CharSequence) key));
76-
wrapper.set(null); // don't hold a reference to the key
77-
return result;
78-
}
79-
80-
return false;
81-
}
82-
83-
@Override
84-
public boolean containsValue(Object value) {
85-
return wrapperMap.containsValue(value);
86-
}
87-
88-
@Override
89-
public V get(Object key) {
90-
if (key instanceof CharSequence) {
91-
CharSequenceWrapper wrapper = WRAPPERS.get();
92-
V result = wrapperMap.get(wrapper.set((CharSequence) key));
93-
wrapper.set(null); // don't hold a reference to the value
94-
return result;
95-
}
96-
97-
return null;
98-
}
99-
100-
@Override
101-
public V put(CharSequence key, V value) {
102-
return wrapperMap.put(CharSequenceWrapper.wrap(key), value);
103-
}
104-
105-
@Override
106-
public V remove(Object key) {
107-
if (key instanceof CharSequence) {
108-
CharSequenceWrapper wrapper = WRAPPERS.get();
109-
V result = wrapperMap.remove(wrapper.set((CharSequence) key));
110-
wrapper.set(null); // don't hold a reference to the value
111-
return result;
112-
}
113-
114-
return null;
115-
}
116-
117-
@Override
118-
public void putAll(Map<? extends CharSequence, ? extends V> otherMap) {
119-
otherMap.forEach(this::put);
120-
}
121-
122-
@Override
123-
public void clear() {
124-
wrapperMap.clear();
125-
}
126-
127-
@Override
128-
public Set<CharSequence> keySet() {
129-
CharSequenceSet keySet = CharSequenceSet.empty();
130-
131-
for (CharSequenceWrapper wrapper : wrapperMap.keySet()) {
132-
keySet.add(wrapper.get());
133-
}
134-
135-
return keySet;
136-
}
137-
138-
@Override
139-
public Collection<V> values() {
140-
return wrapperMap.values();
141-
}
142-
143-
@Override
144-
public Set<Entry<CharSequence, V>> entrySet() {
145-
Set<Entry<CharSequence, V>> entrySet = Sets.newHashSet();
146-
147-
for (Entry<CharSequenceWrapper, V> entry : wrapperMap.entrySet()) {
148-
entrySet.add(new CharSequenceEntry<>(entry));
149-
}
150-
151-
return entrySet;
152-
}
153-
154-
public V computeIfAbsent(CharSequence key, Supplier<V> valueSupplier) {
155-
return computeIfAbsent(key, ignored -> valueSupplier.get());
156-
}
157-
158-
@Override
159-
public boolean equals(Object other) {
160-
if (this == other) {
161-
return true;
162-
} else if (other == null || getClass() != other.getClass()) {
163-
return false;
164-
}
165-
166-
CharSequenceMap<?> that = (CharSequenceMap<?>) other;
167-
return Objects.equals(wrapperMap, that.wrapperMap);
168-
}
169-
170-
@Override
171-
public int hashCode() {
172-
return Objects.hashCode(wrapperMap);
173-
}
174-
175-
@Override
176-
public String toString() {
177-
return entrySet().stream().map(this::toString).collect(Collectors.joining(", ", "{", "}"));
178-
}
179-
180-
private String toString(Entry<CharSequence, V> entry) {
181-
CharSequence key = entry.getKey();
182-
V value = entry.getValue();
183-
return key + "=" + (value == this ? "(this Map)" : value);
184-
}
185-
186-
private static class CharSequenceEntry<V> implements Entry<CharSequence, V> {
187-
private final Entry<CharSequenceWrapper, V> inner;
188-
189-
private CharSequenceEntry(Entry<CharSequenceWrapper, V> inner) {
190-
this.inner = inner;
191-
}
192-
193-
@Override
194-
public CharSequence getKey() {
195-
return inner.getKey().get();
196-
}
197-
198-
@Override
199-
public V getValue() {
200-
return inner.getValue();
201-
}
202-
203-
@Override
204-
public int hashCode() {
205-
return inner.hashCode();
206-
}
207-
208-
@Override
209-
public boolean equals(Object other) {
210-
if (this == other) {
211-
return true;
212-
} else if (other == null || getClass() != other.getClass()) {
213-
return false;
214-
}
215-
216-
CharSequenceEntry<?> that = (CharSequenceEntry<?>) other;
217-
return inner.equals(that.inner);
218-
}
219-
220-
@Override
221-
public V setValue(V value) {
222-
throw new UnsupportedOperationException("Cannot set value");
223-
}
24+
/** A map that uses char sequences as keys and compares them by value. */
25+
public abstract class CharSequenceMap {
26+
public static <V> Map<CharSequence, V> create() {
27+
return new TreeMap<>(CharSequence::compare);
22428
}
22529
}

0 commit comments

Comments
 (0)