Script: Write Field API with basic path resolution by stu-elastic · Pull Request #89738 · elastic/elasticsearch

stu-elastic · 2022-08-30T20:23:32Z

Adds WriteField to the ingest context via field(<path>).

WriteField implements APIs:

String getName(): The path
boolean exists(): Does the path exist in the document?
WriteField set(def): Set the value at the path, creates nested path elements if necessary
WriteField append(def): Appends value to the path, creates nested path elements if necessary, the value at path is always a List after this call?
boolean isEmpty(): Does the path contain a value?
int size(): How many elements does the path contain?
Iterator iterator(): Iterate over all elements at the path.
def get(def): Get the value at the path if it exists, otherwise return the given default
def get(int, def): Get the value at the path and index if it exists, otherwise return the given default
boolean hasValue(Predicate): Is there a value at the path that passes the filter?
WriteField transform(Function): Change all values at the path
WriteField deduplicate(): Remove duplicates from the path
WriteField removeValuesIf(Predicate): Remove a values from the path that pass the filter
WriteField removeValue(int): Remove the index from the path, if it exists

Some APIs remain unimplemented:

void move(String)
void overwrite(String)
void remove()

This PR does not handle equivalent paths, which are paths that differ in the source but flatten to the same field in the indexed document.

Path resolution is basic, each path element is assumed to be a key in the current container, starting with the root. If there is not an entry in the map at a given level, the algorithm checks to see if the remaining path exists as a flat key. The nested then flat nature algorithm handles the common case of some or none nesting followed by flat keys.

Refs: #79155

…eld-fn-proto

elasticsearchmachine · 2022-08-30T20:23:55Z

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

elasticsearchmachine · 2022-08-30T20:24:19Z

Hi @stu-elastic, I've created a changelog YAML for you.

grcevski · 2022-08-31T14:52:29Z

server/src/test/java/org/elasticsearch/script/field/WriteFieldTests.java

+    }
+
+    public void testIsExists() {
+        Map<String, Object> a = new HashMap<>();


Can we please add some exists() tests for the null and missing container value?

I thought this was going to be changed to exists and we weren't going to worry about painless shortcuts.

Ahh, I missed the test, will rename.

grcevski · 2022-08-31T14:56:32Z

server/src/main/java/org/elasticsearch/script/field/WriteField.java

+            List<Object> values;
+            if (v == null) {
+                values = new ArrayList<>(4);
+            } else if (v instanceof List<?> list) {


Can someone supply a unmodifiable list? Should we copy the original list for unexpected side-effects?

Yeah, someone could pass an unmodifiable list, this came up during my testing, in fact.

Should we copy the original list for unexpected side-effects?

My thinking is that would violate expectations, if we attempt to append to an UnmodifiableList then we should propagate the UnsupportedOperationException.

It might be worth specializing here with a good error message as these things can be hard to track down in scripting.

As indicated by @grcevski's comments below, there are a number of places we'd have to specialize to handle UnmodifiableList (or UnmodifiableRandomAccessList) and, to be consistent, we may want to also do the same for unmodifiable Maps.

That's a lot of code to provide a marginally better error message. Do you think it's worth it?

Probably not worth it especially because there's no reasonable way to do this in a performant way.

grcevski · 2022-08-31T15:02:07Z

server/src/main/java/org/elasticsearch/script/field/WriteField.java

+        }
+
+        if (value instanceof List<?> list) {
+            // Assume modifiable list


Should this be an assert here, and perhaps related to my earlier comment, should we consider always making sure we have mutable list.

An assert is going to be turned off in production. The primary reason for using an UnmodifiableList is to error on modifications.

Is it worth having a better error to let the user know what happened here as well?

grcevski · 2022-08-31T15:02:58Z

server/src/main/java/org/elasticsearch/script/field/WriteField.java

+        }
+
+        if (value instanceof List<?> list) {
+            list.removeIf(filter);


assert for mutable list here too.

My thinking is we should let it fail in the standard way, what do you think?

grcevski · 2022-08-31T15:03:18Z

server/src/main/java/org/elasticsearch/script/field/WriteField.java

+        }
+
+        if (value instanceof List<?> list) {
+            if (index < list.size()) {


mutable list assert.

As above, an actual assert would be turned off and any attempt to modify behavior based on an UnmodifiableList works around the scripts intent. Thoughts?

…eld-fn-proto

jdconrad

This looks good to me as I already posted my major concern in the WIP PR about possible issues with out-of-sync write fields due to lack of a consistent backing structure. On the topic of an immutable list -- would it ever make sense for a user to set the value to one that already exists? An advanced user may work around this, so I guess the options are:

assert as suggested by @grcevski
better error message for the user
do nothing and let users encounter it as the built in error
copy values if we detect an immutable list though this probably fragile

stu-elastic · 2022-08-31T20:02:12Z

On the topic of an immutable list -- would it ever make sense for a user to set the value to one that already exists?

Since scripting users do not have access to List.of(...), they would have to be very intentional in their creation of such a List. My preference 3 for the time being.

do nothing and let users encounter it as the built in error

The thinking is there's no easy way to detect unmodifiable lists since they are implemented in multiple classes and not part of the java stdlib API contract. That would mean sprinkling try/catchs everywhere.

If users are tripped up by the errors, that is something we can add later.

grcevski · 2022-09-01T14:35:35Z

On the topic of an immutable list -- would it ever make sense for a user to set the value to one that already exists?

Since scripting users do not have access to List.of(...), they would have to be very intentional in their creation of such a List. My preference 3 for the time being.

do nothing and let users encounter it as the built in error

The thinking is there's no easy way to detect unmodifiable lists since they are implemented in multiple classes and not part of the java stdlib API contract. That would mean sprinkling try/catchs everywhere.

If users are tripped up by the errors, that is something we can add later.

If they don't have List.of then I think it's OK to assume the list will be mutable. 3. looks like a reasonable choice.

grcevski

LGTM!

Adds `WriteField` path manipulation APIs: * `WriteField` `move(String)`: Moves this path to the destination path. Throws an error if destination path exists. * `WriteField` `overwrite(String)`: Same as `move(String)` but overwrites the destination path if it exists; * `void` `remove()`: Remove the leaf value from this path. See also: #89738 Refs: #79155

stu-elastic added 8 commits August 23, 2022 09:50

Script: Write fields fn proto

eb3d3ef

Script tests

dbdfd66

size, iterator, dedupe, transform tests

f4116c7

removeValues and hasValue test

e4b03cf

Merge branch 'main' of /home/stu/src/elasticsearch-main into 0818-wfi…

399f69e

…eld-fn-proto

isExists -> exists, dedupe -> deduplicate

3851692

Add unit test coverage and yamlRestTests

052654e

Merge branch 'main' of github.com:elastic/elasticsearch into 0818-wfi…

13dbe12

…eld-fn-proto

stu-elastic added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Aug 30, 2022

stu-elastic requested review from grcevski and jdconrad August 30, 2022 20:23

elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v8.5.0 labels Aug 30, 2022

Update docs/changelog/89738.yaml

59560cf

stu-elastic mentioned this pull request Aug 30, 2022

Discuss: Script Write Field API with basic path resolution. #89580

Closed

grcevski reviewed Aug 31, 2022

View reviewed changes

stu-elastic added 4 commits August 31, 2022 12:20

exists tests for missing leaf and null mapping

664cd1d

Merge branch 'main' of github.com:elastic/elasticsearch into 0818-wfi…

13053d5

…eld-fn-proto

Remove yaml comments

dce2f3b

testIsExists -> testExists

1ac4f5a

jdconrad approved these changes Aug 31, 2022

View reviewed changes

grcevski approved these changes Sep 1, 2022

View reviewed changes

stu-elastic merged commit e3f34dc into elastic:main Sep 1, 2022

stu-elastic mentioned this pull request Sep 7, 2022

Script: Write Field API path manipulation #89889

Merged

Conversation

stu-elastic commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 30, 2022

Uh oh!

elasticsearchmachine commented Aug 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad left a comment

Choose a reason for hiding this comment

Uh oh!

stu-elastic commented Aug 31, 2022

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

grcevski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stu-elastic commented Aug 30, 2022 •

edited

Loading