Script: Ingest Metadata and CtxMap #88458

stu-elastic · 2022-07-12T02:27:16Z

Create a Metadata superclass for ingest and update contexts.

Create a CtxMap superclass for ctx backwards compatibility in ingest and update contexts. script.CtxMap was moved from ingest.IngestSourceAndMetadata

CtxMap takes a Metadata subclass and validates update via the FieldPropertys passed in.

Metadata provides typed getters and setters and implements a Map-like interface, making it easy for a class containing CtxMap to implement the full Map interface.

The FieldProperty record that configures how to validate fields. Fields have a type, are writeable or read-only, and nullable or not and may have an additional validation useful for Set/Enum validation.

…_ctx_map

…return vt validator, doc matcher only checks metadata map

…gest_ctx_map

Adds FieldProperties record that configures how to validate fields. Fields have a type, are writeable or read-only, and nullable or not and may have an additional validation useful for Set/Enum validation. Splits IngestMetadata from Metadata in preparation for new Metdata subclasses.

… into ingest_ctx_map_field_prop

… into ingest_sm_to_ctx_map

…search into ingest_ctx_map_field_prop

… be updateable

elasticmachine · 2022-07-12T04:27:07Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticmachine · 2022-07-12T04:27:08Z

Pinging @elastic/es-data-management (Team:Data Management)

stu-elastic · 2022-07-12T10:51:54Z

@elasticsearchmachine run elasticsearch-ci/part-2

…gest_ctx_map_field_prop

rjernst

This looks fine. I don't quite see yet how this will get to what we discussed before (subclasses of Metadata passing the operations they support for keys, ie read-only vs read-write), but I know there are more followups. This PR on it's own looks like an improvement.

rjernst · 2022-07-13T13:32:15Z

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

+        return null;
+    }
+
+    static class IngestMetadata extends Metadata {


Since this is static, let's move it out to a tope level class. It can still be package private.

There is already an IngestMetadata that "Holds the ingest pipelines that are available in the cluster".

I'm going to rename this to IngestDocMetadata when I make it top-level.

rjernst · 2022-07-13T13:37:53Z

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

-    public static Tuple<Map<String, Object>, Map<String, Object>> splitSourceAndMetadata(Map<String, Object> sourceAndMetadata) {
-        if (sourceAndMetadata instanceof IngestSourceAndMetadata ingestSourceAndMetadata) {
-            return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata.getMap()));
+    public static Tuple<Map<String, Object>, Map<String, Object>> splitSourceAndMetadata(


Why is this method necessary when the source and metadata are already split internally? Couldn't this be two separate calls to member methods, to get the source map and the Metadata object?

rjernst · 2022-07-13T13:39:41Z

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

-        }
-        return metadata;
-    }
-
    /**
     * Check that all metadata map contains only valid metadata and no extraneous keys and source map contains no metadata


nit: I think this comment is off? there is no source map

rjernst · 2022-07-13T13:41:28Z

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

@@ -172,7 +119,7 @@ public void setVersion(long version) {
    }

    public ZonedDateTime getTimestamp() {


This can move completely to IngestMetadata right? We shouldn't need it to exist at all on other Metadata subclasses.

It is available in 2 of 4 write contexts, ingest and update. It is missing in reindex and update by query

It's here because that allows scripts to do Metadata m = metadata(); m.timestamp. Otherwise, the script would have to name the subtype.

It is possible to add timestamp to reindex and update by query, both have access to the thread pool and pull a long supplier from it.

We could duck-type the Metadata subclasses just for timestamp.

If it’s relevant to all the other contexts, then it makes sense to stay completely in this class. But I don’t think we should have it defined here but implemented in subclasses.

I'll change the implementation here in the Update PR.

…x validate javadoc

jdconrad

LGTM. A few minor comments.

jdconrad · 2022-07-13T16:37:43Z

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

+        ZonedDateTime timestamp,
+        Map<String, Object> source
+    ) {
+        super(new HashMap<>(source), new IngestDocMetadata(index, id, version, routing, versionType, timestamp));


Since we do a deepcopy for source when calling this constructor, do we then again need to wrap source in a new HashMap?

A bunch of tests call into IngestDocument which calls this constructor, those test all do Map.of or SingletonMap and fail if we don't perform this copy.

jdconrad · 2022-07-13T16:42:45Z

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

@@ -763,7 +751,7 @@ public static Object deepCopy(Object value) {
            for (Map.Entry<?, ?> entry : mapValue.entrySet()) {
                copy.put(entry.getKey(), deepCopy(entry.getValue()));
            }
-            // TODO(stu): should this check for IngestSourceAndMetadata in addition to Map?
+            // TODO(stu): should this check for IngestCtxMap in addition to Map?


Did you mean to leave this as part of the PR?

It shows up because of the rename, it's still relevant question for now.

jdconrad · 2022-07-13T16:44:33Z

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

@@ -172,7 +119,7 @@ public void setVersion(long version) {
    }

    public ZonedDateTime getTimestamp() {


We could duck-type the Metadata subclasses just for timestamp.

This reverts commit a08afb8.

stu-elastic added 21 commits July 8, 2022 15:18

WIP

bf2085f

Split Metadata off off IngestSourceAndMetadata

e0ff58a

Update metadata javadoc

4ea1791

Add hashcode and equals to make the WritableIngestDocumentTests pass

7244010

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

76022a6

…_ctx_map

keys() -> getKeys(), don't cache entrySet, check metadata in source, …

75f85bd

…return vt validator, doc matcher only checks metadata map

Return raw map

aebad9a

Better javadoc for metadata

0c2e023

Merge branch 'master' of github.com:stu-elastic/elasticsearch into in…

267ebde

…gest_ctx_map

Script: Move Map impl from IngestSourceAndMetadata to CtxMap

20d8019

Fix hashCode in CtxMap

29dd6ec

Fix hashCode in CtxMap

472b824

Check metadata map in testSimulate

bdf6bfd

Merge branch 'ingest_ctx_map' of github.com:stu-elastic/elasticsearch…

1a0ef93

… into ingest_ctx_map_field_prop

Merge branch 'ingest_ctx_map' of github.com:stu-elastic/elasticsearch…

1d74d86

… into ingest_sm_to_ctx_map

keySet(), Sets.intersection, isAvailable, protected validator & getters

1600725

Merge branch 'ingest_ctx_map' of github.com:stu-elastic/elasticsearch…

ae733b7

… into ingest_sm_to_ctx_map

Merge branch 'ingest_sm_to_ctx_map' of github.com:stu-elastic/elastic…

48c62de

…search into ingest_ctx_map_field_prop

Fix unit tests

f9ac7ae

FieldProperty javadoc

15328b1

stu-elastic added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >refactoring labels Jul 12, 2022

elasticsearchmachine added the v8.4.0 label Jul 12, 2022

stu-elastic added 4 commits July 11, 2022 21:38

Implement getTimestamp() in IngestMetadata

824cdc8

Use extended validators, add hashcode and equals for Metadata

c39bb9d

_type can be updated

e81f5fa

Fix testExecutePropagateAllMetadataUpdates rather than allow _type to…

7276bb0

… be updateable

stu-elastic marked this pull request as ready for review July 12, 2022 04:27

elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team labels Jul 12, 2022

stu-elastic mentioned this pull request Jul 12, 2022

Ingest: Start separating Metadata from IngestSourceAndMetadata #88401

Merged

stu-elastic requested review from jdconrad and rjernst July 12, 2022 14:52

stu-elastic added 4 commits July 12, 2022 10:12

Merge branch 'master' of github.com:stu-elastic/elasticsearch into in…

7be1f41

…gest_ctx_map_field_prop

Merge branch 'master' of github.com:stu-elastic/elasticsearch into in…

e27d73a

…gest_ctx_map_field_prop

Remove getIngestSourceAndMetadata

9c7b389

Don't clone properties, add MetadataTests

5a45f97

rjernst approved these changes Jul 13, 2022

View reviewed changes

IngestMetadata -> IngestDocMetadata, remove splitSourceAndMetadta, fi…

dc1fc58

…x validate javadoc

jdconrad approved these changes Jul 13, 2022

View reviewed changes

stu-elastic added 2 commits July 13, 2022 13:03

Don't copy source

a08afb8

Revert "Don't copy source"

8eb42c6

This reverts commit a08afb8.

stu-elastic merged commit 85b8d3d into elastic:master Jul 13, 2022

stu-elastic mentioned this pull request Jul 13, 2022

Script: Metadata for update context #88333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script: Ingest Metadata and CtxMap #88458

Script: Ingest Metadata and CtxMap #88458

stu-elastic commented Jul 12, 2022 •

edited

Loading

elasticmachine commented Jul 12, 2022

elasticmachine commented Jul 12, 2022

stu-elastic commented Jul 12, 2022

rjernst left a comment

rjernst Jul 13, 2022

stu-elastic Jul 13, 2022

rjernst Jul 13, 2022

stu-elastic Jul 13, 2022

rjernst Jul 13, 2022

stu-elastic Jul 13, 2022

rjernst Jul 13, 2022

stu-elastic Jul 13, 2022

jdconrad Jul 13, 2022

rjernst Jul 13, 2022

stu-elastic Jul 13, 2022

jdconrad left a comment

jdconrad Jul 13, 2022

stu-elastic Jul 13, 2022

jdconrad Jul 13, 2022

stu-elastic Jul 13, 2022

jdconrad Jul 13, 2022

		@@ -172,7 +119,7 @@ public void setVersion(long version) {
		}

		public ZonedDateTime getTimestamp() {

Script: Ingest Metadata and CtxMap #88458

Script: Ingest Metadata and CtxMap #88458

Conversation

stu-elastic commented Jul 12, 2022 • edited Loading

elasticmachine commented Jul 12, 2022

elasticmachine commented Jul 12, 2022

stu-elastic commented Jul 12, 2022

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-elastic commented Jul 12, 2022 •

edited

Loading