Optimize IngestDocMetadata isAvailable (#120753)

Author: joegallo

docs/changelog/120753.yaml

@@ -0,0 +1,5 @@
+pr: 120753
+summary: Optimize `IngestDocMetadata` `isAvailable`
+area: Ingest Node
+type: enhancement
+issues: []

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java

@@ -14,7 +14,6 @@
 import org.elasticsearch.ingest.RandomDocumentPicks;
 import org.elasticsearch.ingest.TestIngestDocument;
 import org.elasticsearch.ingest.TestTemplateService;
-import org.elasticsearch.script.Metadata;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.ArrayList;
@@ -140,42 +139,40 @@ public void testRenameExistingFieldNullValue() throws Exception {
 
     public void testRenameAtomicOperationSetFails() throws Exception {
         Map<String, Object> metadata = new HashMap<>();
-        metadata.put("list", List.of("item"));
-
-        IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(
-            metadata,
-            Map.of("new_field", new Metadata.FieldProperty<>(Object.class, true, true, (k, v) -> {
-                if (v != null) {
-                    throw new UnsupportedOperationException();
-                }
-            }), "list", new Metadata.FieldProperty<>(Object.class, true, true, null))
-        );
-        Processor processor = createRenameProcessor("list", "new_field", false, false);
+        metadata.put("_index", "foobar");
+
+        IngestDocument ingestDocument = TestIngestDocument.withDefaultVersion(metadata);
+        Processor processor = createRenameProcessor("_index", "_version_type", false, false);
         try {
             processor.execute(ingestDocument);
             fail("processor execute should have failed");
-        } catch (UnsupportedOperationException e) {
+        } catch (IllegalArgumentException e) {
             // the set failed, the old field has not been removed
-            assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
-            assertThat(ingestDocument.getSourceAndMetadata().containsKey("new_field"), equalTo(false));
+            assertThat(
+                e.getMessage(),
+                equalTo(
+                    "_version_type must be a null or one of [internal, external, external_gte] "
+                        + "but was [foobar] with type [java.lang.String]"
+                )
+            );
+            assertThat(ingestDocument.getSourceAndMetadata().containsKey("_index"), equalTo(true));
+            assertThat(ingestDocument.getSourceAndMetadata().containsKey("_version_type"), equalTo(false));
         }
     }
 
     public void testRenameAtomicOperationRemoveFails() throws Exception {
         Map<String, Object> metadata = new HashMap<>();
-        metadata.put("list", List.of("item"));
+        metadata.put("foo", "bar");
 
-        IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(
-            metadata,
-            Map.of("list", new Metadata.FieldProperty<>(Object.class, false, true, null))
-        );
-        Processor processor = createRenameProcessor("list", "new_field", false, false);
+        IngestDocument ingestDocument = TestIngestDocument.withDefaultVersion(metadata);
+        Processor processor = createRenameProcessor("_version", "new_field", false, false);
         try {
             processor.execute(ingestDocument);
             fail("processor execute should have failed");
         } catch (IllegalArgumentException e) {
-            // the set failed, the old field has not been removed
-            assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
+            // the remove failed, the old field has not been removed
+            assertThat(e.getMessage(), equalTo("_version cannot be removed"));
+            assertThat(ingestDocument.getSourceAndMetadata().containsKey("_version"), equalTo(true));
             assertThat(ingestDocument.getSourceAndMetadata().containsKey("new_field"), equalTo(false));
         }
     }

server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java

@@ -29,7 +29,7 @@
  *
  * The map is expected to be used by processors, server code should the typed getter and setters where possible.
  */
-class IngestCtxMap extends CtxMap<IngestDocMetadata> {
+final class IngestCtxMap extends CtxMap<IngestDocMetadata> {
 
     /**
      * Create an IngestCtxMap with the given metadata, source and default validators

server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java

@@ -19,7 +19,7 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 
-class IngestDocMetadata extends Metadata {
+final class IngestDocMetadata extends Metadata {
 
     static final Map<String, FieldProperty<?>> PROPERTIES = Map.of(
         INDEX,
@@ -42,18 +42,25 @@ class IngestDocMetadata extends Metadata {
         new FieldProperty<>(Map.class).withWritable().withNullable()
     );
 
+    private static final char UNDERSCORE = '_';
+    static {
+        // there's an optimization here in the overridden isAvailable below, but it only works if the first character of each of these
+        // keys starts with an underscore, since we know all the keys up front, though, we can just make sure that's always true
+        for (String key : PROPERTIES.keySet()) {
+            if (key.charAt(0) != UNDERSCORE) {
+                throw new IllegalArgumentException("IngestDocMetadata keys must begin with an underscore, but found [" + key + "]");
+            }
+        }
+    }
+
     protected final ZonedDateTime timestamp;
 
     IngestDocMetadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) {
         this(metadataMap(index, id, version, routing, versionType), timestamp);
     }
 
     IngestDocMetadata(Map<String, Object> metadata, ZonedDateTime timestamp) {
-        this(metadata, PROPERTIES, timestamp);
-    }
-
-    IngestDocMetadata(Map<String, Object> metadata, Map<String, FieldProperty<?>> properties, ZonedDateTime timestamp) {
-        super(metadata, properties);
+        super(metadata, PROPERTIES);
         this.timestamp = timestamp;
     }
 
@@ -100,4 +107,16 @@ private static void versionTypeValidator(String key, String value) {
                 + "]"
         );
     }
+
+    @Override
+    public boolean isAvailable(String key) {
+        // the key cannot be null or empty because of the nature of the calling code, and this is already validated in IngestDocument
+        assert key != null && key.isEmpty() == false;
+        // we can avoid a map lookup on most keys since we know that the only keys that are 'metadata keys' for an ingest document
+        // must be keys that start with an underscore
+        if (key.charAt(0) != UNDERSCORE) {
+            return false;
+        }
+        return super.isAvailable(key);
+    }
 }

server/src/main/java/org/elasticsearch/ingest/IngestService.java

@@ -1204,8 +1204,9 @@ private static void updateIndexRequestMetadata(final IndexRequest request, final
         request.id(metadata.getId());
         request.routing(metadata.getRouting());
         request.version(metadata.getVersion());
-        if (metadata.getVersionType() != null) {
-            request.versionType(VersionType.fromString(metadata.getVersionType()));
+        String versionType;
+        if ((versionType = metadata.getVersionType()) != null) {
+            request.versionType(VersionType.fromString(versionType));
         }
         Number number;
         if ((number = metadata.getIfSeqNo()) != null) {

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

@@ -14,6 +14,7 @@
 import java.util.AbstractCollection;
 import java.util.AbstractMap;
 import java.util.AbstractSet;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -150,10 +151,12 @@ public Object remove(Object key) {
     @Override
     public void clear() {
         // AbstractMap uses entrySet().clear(), it should be quicker to run through the validators, then call the wrapped maps clear
-        for (String key : metadata.keySet()) {
+        for (String key : new ArrayList<>(metadata.keySet())) { // copy the key set to get around the ConcurrentModificationException
             metadata.remove(key);
         }
-        // TODO: this is just bogus, there isn't any case where metadata won't trip a failure above?
+        // note: this is actually bogus in the general case, though! for this to work there must be some Metadata or subclass of Metadata
+        // for which all the FieldPoperty properties of the metadata are nullable and therefore could have been removed in the previous
+        // loop -- does such a class even exist? (that is, is there any *real* CtxMap for which the previous loop didn't throw?)
         source.clear();
     }
 

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

@@ -110,7 +110,7 @@ public void testRemoveSource() {
         source.put("abc", 123);
         source.put("def", 456);
         source.put("hij", 789);
-        map = new IngestCtxMap(source, new TestIngestCtxMetadata(new HashMap<>(), new HashMap<>()));
+        map = new IngestCtxMap(source, new IngestDocMetadata(new HashMap<>(Map.of("_version", 1L)), null));
 
         // Make sure there isn't a ConcurrentModificationException when removing a key from the iterator
         String removedKey = null;
@@ -129,31 +129,18 @@ public void testRemoveSource() {
     }
 
     public void testRemove() {
-        String cannotRemove = "cannotRemove";
-        String canRemove = "canRemove";
-        Map<String, Object> metadata = new HashMap<>();
-        metadata.put(cannotRemove, "value");
-        map = new IngestCtxMap(
-            new HashMap<>(),
-            new TestIngestCtxMetadata(
-                metadata,
-                Map.of(
-                    cannotRemove,
-                    new Metadata.FieldProperty<>(String.class, false, true, null),
-                    canRemove,
-                    new Metadata.FieldProperty<>(String.class, true, true, null)
-                )
-            )
-        );
-        String msg = "cannotRemove cannot be removed";
+        String cannotRemove = "_version"; // writable, but not *nullable*
+        String canRemove = "_id"; // writable, and *nullable*
+        map = new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(new HashMap<>(Map.of(cannotRemove, 1L)), null));
+        String msg = "_version cannot be removed";
         IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.remove(cannotRemove));
         assertEquals(msg, err.getMessage());
 
         err = expectThrows(IllegalArgumentException.class, () -> map.put(cannotRemove, null));
-        assertEquals("cannotRemove cannot be null", err.getMessage());
+        assertEquals("_version cannot be null", err.getMessage());
 
         err = expectThrows(IllegalArgumentException.class, () -> map.entrySet().iterator().next().setValue(null));
-        assertEquals("cannotRemove cannot be null", err.getMessage());
+        assertEquals("_version cannot be null", err.getMessage());
 
         err = expectThrows(IllegalArgumentException.class, () -> {
             Iterator<Map.Entry<String, Object>> it = map.entrySet().iterator();
@@ -176,6 +163,10 @@ public void testRemove() {
         err = expectThrows(IllegalArgumentException.class, () -> map.clear());
         assertEquals(msg, err.getMessage());
 
+        // depending on iteration order, this may have been removed, so put it back before checking the size
+        map.put(canRemove, "value");
+        assertEquals("value", map.get(canRemove));
+
         assertEquals(2, map.size());
 
         map.entrySet().remove(new TestEntry(canRemove, "value"));
@@ -205,7 +196,7 @@ public void testEntryAndIterator() {
         source.put("foo", "bar");
         source.put("baz", "qux");
         source.put("noz", "zon");
-        map = new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata));
+        map = new IngestCtxMap(source, new IngestDocMetadata(metadata, null));
         md = map.getMetadata();
 
         for (Map.Entry<String, Object> entry : map.entrySet()) {
@@ -240,8 +231,10 @@ public void testEntryAndIterator() {
         assertTrue(map.containsKey("noz"));
         assertEquals(3, map.entrySet().size());
         assertEquals(3, map.size());
-        map.clear();
-        assertEquals(0, map.size());
+
+        // since an IngestCtxMap must have a _version (and the _version cannot be null), we can't just .clear()
+        map.entrySet().removeIf(e -> e.getKey().equals("_version") == false);
+        assertEquals(1, map.size());
     }
 
     public void testContainsValue() {

test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java

@@ -10,10 +10,8 @@
 package org.elasticsearch.ingest;
 
 import org.elasticsearch.common.lucene.uid.Versions;
-import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.index.VersionType;
-import org.elasticsearch.script.Metadata;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.HashMap;
@@ -24,53 +22,30 @@
  */
 public class TestIngestDocument {
     public static final long DEFAULT_VERSION = 12345L;
-    private static String VERSION = IngestDocument.Metadata.VERSION.getFieldName();
-
-    /**
-     * Create an IngestDocument for testing that pass an empty mutable map for ingestMetaata
-     */
-    public static IngestDocument withNullableVersion(Map<String, Object> sourceAndMetadata) {
-        return ofIngestWithNullableVersion(sourceAndMetadata, new HashMap<>());
-    }
+    private static final String VERSION = IngestDocument.Metadata.VERSION.getFieldName();
 
     /**
      * Create an {@link IngestDocument} from the given sourceAndMetadata and ingestMetadata and a version validator that allows null
      * _versions.  Normally null _version is not allowed, but many tests don't care about that invariant.
      */
-    public static IngestDocument ofIngestWithNullableVersion(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
-        Map<String, Object> source = new HashMap<>(sourceAndMetadata);
-        Map<String, Object> metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length);
-        for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) {
-            String key = m.getFieldName();
-            if (sourceAndMetadata.containsKey(key)) {
-                metadata.put(key, source.remove(key));
-            }
-        }
-        return new IngestDocument(new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata)), ingestMetadata);
-    }
-
-    /**
-     * Create an {@link IngestDocument} with {@link #DEFAULT_VERSION} as the _version metadata, if _version is not already present.
-     */
-    public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata) {
+    public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
         if (sourceAndMetadata.containsKey(VERSION) == false) {
             sourceAndMetadata = new HashMap<>(sourceAndMetadata);
             sourceAndMetadata.put(VERSION, DEFAULT_VERSION);
         }
-        return new IngestDocument(sourceAndMetadata, new HashMap<>());
+        return new IngestDocument(sourceAndMetadata, ingestMetadata);
     }
 
     /**
-     * Create an IngestDocument with a metadata map and validators.  The metadata map is passed by reference, not copied, so callers
-     * can observe changes to the map directly.
+     * Create an {@link IngestDocument} with {@link #DEFAULT_VERSION} as the _version metadata, if _version is not already present.
      */
-    public static IngestDocument ofMetadataWithValidator(Map<String, Object> metadata, Map<String, Metadata.FieldProperty<?>> properties) {
-        return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, properties)), new HashMap<>());
+    public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata) {
+        return withDefaultVersion(sourceAndMetadata, new HashMap<>());
     }
 
     /**
      * Create an empty ingest document for testing.
-     *
+     * <p>
      * Adds the required {@code "_version"} metadata key with value {@link #DEFAULT_VERSION}.
      */
     public static IngestDocument emptyIngestDocument() {