Skip to content

Commit b752c8a

Browse files
joegallojoegallo
authored
Use getOrDefault in IngestDocument rather than containsKey+get (elastic#120571) (elastic#120815)
1 parent 0ab3585 commit b752c8a

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
@@ -55,6 +55,9 @@ public final class IngestDocument {
5555
// This is the maximum number of nested pipelines that can be within a pipeline. If there are more, we bail out with an error
5656
public static final int MAX_PIPELINES = Integer.parseInt(System.getProperty("es.ingest.max_pipelines", "100"));
5757

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

@@ -375,11 +378,15 @@ private static ResolveResult resolve(String pathElement, String fullPath, Object
375378
if (context == null) {
376379
return ResolveResult.error("cannot resolve [" + pathElement + "] from null as part of path [" + fullPath + "]");
377380
}
378-
if (context instanceof Map<?, ?> map) {
379-
if (map.containsKey(pathElement)) {
380-
return ResolveResult.success(map.get(pathElement));
381+
if (context instanceof Map<?, ?>) {
382+
@SuppressWarnings("unchecked")
383+
Map<String, Object> map = (Map<String, Object>) context;
384+
Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
385+
if (object == NOT_FOUND) {
386+
return ResolveResult.error("field [" + pathElement + "] not present as part of path [" + fullPath + "]");
387+
} else {
388+
return ResolveResult.success(object);
381389
}
382-
return ResolveResult.error("field [" + pathElement + "] not present as part of path [" + fullPath + "]");
383390
}
384391
if (context instanceof List<?> list) {
385392
int index;
@@ -546,12 +553,13 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
546553
if (context instanceof Map) {
547554
@SuppressWarnings("unchecked")
548555
Map<String, Object> map = (Map<String, Object>) context;
549-
if (map.containsKey(pathElement)) {
550-
context = map.get(pathElement);
551-
} else {
552-
HashMap<Object, Object> newMap = new HashMap<>();
556+
Object object = map.getOrDefault(pathElement, NOT_FOUND); // getOrDefault is faster than containsKey + get
557+
if (object == NOT_FOUND) {
558+
Map<Object, Object> newMap = new HashMap<>();
553559
map.put(pathElement, newMap);
554560
context = newMap;
561+
} else {
562+
context = object;
555563
}
556564
} else if (context instanceof List<?> list) {
557565
int index;
@@ -590,16 +598,16 @@ private void setFieldValue(String path, Object value, boolean append, boolean al
590598
@SuppressWarnings("unchecked")
591599
Map<String, Object> map = (Map<String, Object>) context;
592600
if (append) {
593-
if (map.containsKey(leafKey)) {
594-
Object object = map.get(leafKey);
601+
Object object = map.getOrDefault(leafKey, NOT_FOUND); // getOrDefault is faster than containsKey + get
602+
if (object == NOT_FOUND) {
603+
List<Object> list = new ArrayList<>();
604+
appendValues(list, value);
605+
map.put(leafKey, list);
606+
} else {
595607
Object list = appendValues(object, value, allowDuplicates);
596608
if (list != object) {
597609
map.put(leafKey, list);
598610
}
599-
} else {
600-
List<Object> list = new ArrayList<>();
601-
appendValues(list, value);
602-
map.put(leafKey, list);
603611
}
604612
return;
605613
}

‎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)