Skip to content

Commit 83dd34f

Browse files
joegallojoegallo
authored
Use getOrDefault in IngestDocument rather than containsKey+get (#120571)
1 parent 60f78e4 commit 83dd34f

File tree

6 files changed

+96
-14
lines changed

6 files changed

+96
-14
lines changed

‎server/src/main/java/org/elasticsearch/ingest/IngestDocument.java‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ public final class IngestDocument {
5656
// This is the maximum number of nested pipelines that can be within a pipeline. If there are more, we bail out with an error
5757
public static final int MAX_PIPELINES = Integer.parseInt(System.getProperty("es.ingest.max_pipelines", "100"));
5858

59+
// a 'not found' sentinel value for use in getOrDefault calls in order to avoid containsKey-and-then-get
60+
private static final Object NOT_FOUND = new Object();
61+
5962
private final IngestCtxMap ctxMap;
6063
private final Map<String, Object> ingestMetadata;
6164

@@ -376,11 +379,15 @@ private static ResolveResult resolve(String pathElement, String fullPath, Object
376379
if (context == null) {
377380
return ResolveResult.error("cannot resolve [" + pathElement + "] from null as part of path [" + fullPath + "]");
378381
}
379-
if (context instanceof Map<?, ?> map) {
380-
if (map.containsKey(pathElement)) {
381-
return ResolveResult.success(map.get(pathElement));
382+
if (context instanceof Map<?, ?>) {
383+
@SuppressWarnings("unchecked")
384+
Map<String, Object> map = (Map<String, Object>) context;
385+
Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
386+
if (object == NOT_FOUND) {
387+
return ResolveResult.error("field [" + pathElement + "] not present as part of path [" + fullPath + "]");
388+
} else {
389+
return ResolveResult.success(object);
382390
}
383-
return ResolveResult.error("field [" + pathElement + "] not present as part of path [" + fullPath + "]");
384391
}
385392
if (context instanceof List<?> list) {
386393
int index;
@@ -547,12 +554,13 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
547554
if (context instanceof Map) {
548555
@SuppressWarnings("unchecked")
549556
Map<String, Object> map = (Map<String, Object>) context;
550-
if (map.containsKey(pathElement)) {
551-
context = map.get(pathElement);
552-
} else {
553-
HashMap<Object, Object> newMap = new HashMap<>();
557+
Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
558+
if (object == NOT_FOUND) {
559+
Map<Object, Object> newMap = new HashMap<>();
554560
map.put(pathElement, newMap);
555561
context = newMap;
562+
} else {
563+
context = object;
556564
}
557565
} else if (context instanceof List<?> list) {
558566
int index;
@@ -591,16 +599,16 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
591599
@SuppressWarnings("unchecked")
592600
Map<String, Object> map = (Map<String, Object>) context;
593601
if (append) {
594-
if (map.containsKey(leafKey)) {
595-
Object object = map.get(leafKey);
602+
Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
603+
if (object == NOT_FOUND) {
604+
List<Object> list = new ArrayList<>();
605+
appendValues(list, value);
606+
map.put(leafKey, list);
607+
} else {
596608
Object list = appendValues(object, value, allowDuplicates);
597609
if (list != object) {
598610
map.put(leafKey, list);
599611
}
600-
} else {
601-
List<Object> list = new ArrayList<>();
602-
appendValues(list, value);
603-
map.put(leafKey, list);
604612
}
605613
return;
606614
}

‎server/src/main/java/org/elasticsearch/script/CtxMap.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,18 @@ public Object get(Object key) {
196196
return directSourceAccess() ? source.get(key) : (SOURCE.equals(key) ? source : null);
197197
}
198198

199+
@Override
200+
public Object getOrDefault(Object key, Object defaultValue) {
201+
// uses map directly to avoid Map's implementation that is just get and then containsKey and so could require two isAvailable calls
202+
if (key instanceof String str) {
203+
if (metadata.isAvailable(str)) {
204+
return metadata.getOrDefault(str, defaultValue);
205+
}
206+
return directSourceAccess() ? source.getOrDefault(key, defaultValue) : (SOURCE.equals(key) ? source : defaultValue);
207+
}
208+
return defaultValue;
209+
}
210+
199211
/**
200212
* Set of entries of the wrapped map that calls the appropriate validator before changing an entries value or removing an entry.
201213
*

‎server/src/main/java/org/elasticsearch/script/Metadata.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,13 @@ public Object get(String key) {
240240
return map.get(key);
241241
}
242242

243+
/**
244+
* Get the value associated with {@param key}, otherwise return {@param defaultValue}
245+
*/
246+
public Object getOrDefault(String key, Object defaultValue) {
247+
return map.getOrDefault(key, defaultValue);
248+
}
249+
243250
/**
244251
* Remove the mapping associated with {@param key}
245252
* @throws IllegalArgumentException if {@link #isAvailable(String)} is false or the key cannot be removed.

‎server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,18 @@ public void testHandlesAllVersionTypes() {
329329
assertNull(md.getVersionType());
330330
}
331331

332+
public void testGetOrDefault() {
333+
map = new IngestCtxMap(Map.of("foo", "bar"), new IngestDocMetadata(Map.of("_version", 5L), null));
334+
335+
// it does the expected thing for fields that are present
336+
assertThat(map.getOrDefault("_version", -1L), equalTo(5L));
337+
assertThat(map.getOrDefault("foo", "wat"), equalTo("bar"));
338+
339+
// it does the expected thing for fields that are not present
340+
assertThat(map.getOrDefault("_version_type", "something"), equalTo("something"));
341+
assertThat(map.getOrDefault("baz", "quux"), equalTo("quux"));
342+
}
343+
332344
private static class TestEntry implements Map.Entry<String, Object> {
333345
String key;
334346
Object value;

‎server/src/test/java/org/elasticsearch/script/CtxMapTests.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@
1717
import java.util.List;
1818
import java.util.Map;
1919

20+
import static org.elasticsearch.script.Metadata.LongField;
21+
import static org.elasticsearch.script.Metadata.VERSION;
2022
import static org.hamcrest.Matchers.containsString;
23+
import static org.hamcrest.Matchers.equalTo;
2124

2225
public class CtxMapTests extends ESTestCase {
2326
CtxMap<?> map;
@@ -29,6 +32,37 @@ public void setUp() throws Exception {
2932
map = new CtxMap<>(new HashMap<>(), new Metadata(Map.of(), Map.of()));
3033
}
3134

35+
@SuppressWarnings("unchecked")
36+
public void testGetOrDefault() {
37+
{
38+
map = new CtxMap<>(Map.of("foo", "bar"), new Metadata(Map.of("_version", 5L), Map.of(VERSION, LongField.withWritable())));
39+
40+
// it does the expected thing for fields that are present
41+
assertThat(map.getOrDefault("_version", -1L), equalTo(5L));
42+
assertThat(((Map<String, Object>) map.getOrDefault("_source", Map.of())).getOrDefault("foo", "wat"), equalTo("bar"));
43+
44+
// it does the expected thing for fields that are not present
45+
assertThat(map.getOrDefault("_version_type", "something"), equalTo("something"));
46+
assertThat(map.getOrDefault("baz", "quux"), equalTo("quux"));
47+
}
48+
{
49+
map = new CtxMap<>(Map.of("foo", "bar"), new Metadata(Map.of("_version", 5L), Map.of(VERSION, LongField.withWritable()))) {
50+
@Override
51+
protected boolean directSourceAccess() {
52+
return true;
53+
}
54+
};
55+
56+
// it does the expected thing for fields that are present
57+
assertThat(map.getOrDefault("_version", -1L), equalTo(5L));
58+
assertThat(map.getOrDefault("foo", "wat"), equalTo("bar"));
59+
60+
// it does the expected thing for fields that are not present
61+
assertThat(map.getOrDefault("_version_type", "something"), equalTo("something"));
62+
assertThat(map.getOrDefault("baz", "quux"), equalTo("quux"));
63+
}
64+
}
65+
3266
public void testAddingJunkToCtx() {
3367
IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.put("junk", "stuff"));
3468
assertEquals("Cannot put key [junk] with value [stuff] into ctx", err.getMessage());

‎server/src/test/java/org/elasticsearch/script/MetadataTests.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import java.util.Map;
1717
import java.util.Set;
1818

19+
import static org.hamcrest.Matchers.equalTo;
20+
1921
public class MetadataTests extends ESTestCase {
2022
Metadata md;
2123
private static final Metadata.FieldProperty<String> STRING_PROP = new Metadata.FieldProperty<>(String.class, true, true, null);
@@ -279,4 +281,11 @@ public void testImmutablePropertiesMap() {
279281
new Metadata(Map.of(), Map.of());
280282
new Metadata(Map.of(), Map.copyOf(new HashMap<>()));
281283
}
284+
285+
public void testGetOrDefault() {
286+
md = new Metadata(new HashMap<>(Map.of("foo", "bar")), Map.of("foo", STRING_PROP, "baz", STRING_PROP));
287+
assertThat(md.getOrDefault("foo", "wat"), equalTo("bar"));
288+
assertThat(md.getOrDefault("bar", "wat"), equalTo("wat"));
289+
assertThat(md.getOrDefault("yo", "wat"), equalTo("wat"));
290+
}
282291
}

0 commit comments

Comments
 (0)