Eval REPLACE function by abatsakis · Pull Request #98909 · elastic/elasticsearch

abatsakis · 2023-08-26T03:49:21Z

The EVAL replace function that substitutes the new string for every occurrence of the regex in the source string.

cla-checker-service · 2023-08-26T03:49:25Z

❌ Author of the following commits did not sign a Contributor Agreement:
a0aacec, ff0117b, 95ca036, 26bc2fc, a4ff2c9

Please, read and sign the above mentioned agreement if you want to contribute to this project

github-actions · 2023-08-26T03:49:34Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2023-08-26T03:49:45Z

@abatsakis please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

elasticsearchmachine · 2023-08-26T03:49:45Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-08-26T03:49:45Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

luigidellaquila

I added a comment regarding a possible significant optimization.

luigidellaquila · 2023-08-28T06:58:51Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Replace.java

+    }
+
+    @Evaluator
+    static BytesRef process(BytesRef str, BytesRef regex, BytesRef newStr) {


The regex is a very good candidate to be optimized as a @Fixed parameter.
You can have an additional process method like

@Evaluator(extraName = "Constant") static BytesRef process(BytesRef str, @Fixed Pattern regex, BytesRef newStr) { ... }

(not sure it works in this order, maybe you'll have to switch second and third parameter)
and compile the regex Pattern only once in case the expression is foldable (that is likely the most common use case). It will make the execution much more efficient.
For a practical example see https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateFormat.java#L87

abdonpijpelink

Left a small docs suggestion (every command on a new line; remove the semicolon).

abdonpijpelink · 2023-08-28T09:20:15Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-spec

+//tag::replaceString[]
+ROW str = "Hello World" | EVAL str = REPLACE(str, "World", "Universe") | KEEP str;
+// end::replaceString[]


Suggested change

//tag::replaceString[]

ROW str = "Hello World" | EVAL str = REPLACE(str, "World", "Universe") | KEEP str;

// end::replaceString[]

//tag::replaceString[]

ROW str = "Hello World"

| EVAL str = REPLACE(str, "World", "Universe")

| KEEP str

// end::replaceString[]

;

luigidellaquila · 2023-09-05T08:54:16Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Replace.java

+        }
+
+        try {
+            return new BytesRef(str.utf8ToString().replaceAll(regex.pattern(), newStr.utf8ToString()));


Now that you have the compiled Pattern, you can use it for a more efficient replaceAll():

return new BytesRef(regex.matcher(str.utf8ToString()).replaceAll(newStr.utf8ToString()));

luigidellaquila

LGTM!

Thanks for your patience @abatsakis, I just left one last small comment about tests, but for the rest the PR is 👍 👍 👍

luigidellaquila · 2023-09-20T06:54:49Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/string.csv-spec

+;
+
+replace complex
+from employees | where emp_no <= 10010 | eval f_l = replace(replace(substring(last_name, 1, 20), "al", "AB"), "a", "Z")  | keep emp_no, last_name, f_l;


Please add a | sort emp_no somewhere in these two queries, otherwise they'll randomly fail in multi-node

astefan · 2023-09-27T15:44:42Z

@abatsakis I'll polish this one and push it over the finishing line, if that's ok with you.

…esql/replaceFunction

bpintea

LGTM

bpintea · 2023-09-29T08:57:41Z

...esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/package-info.java

- *         and click {@code Build->Recompile 'FunctionName.java'} in IntelliJ. This should generate
- *         an {@link org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator} implementation
+ *         and click {@code Build->Recompile 'FunctionName.java'} in IntelliJ or run the
+ *         {@code CsvTests} again.. This should generate an


Suggested change

* {@code CsvTests} again.. This should generate an

* {@code CsvTests} again. This should generate an

bpintea · 2023-09-29T08:58:55Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/string.csv-spec

+;
+
+replaceRegex
+from hosts | where host == "epsilon" | eval l1=replace(host_group, "\\s+", "") | sort l1 | keep l1;


just in case any other change might be applied: keep host_group too.

bpintea · 2023-09-29T09:01:53Z

docs/reference/esql/functions/replace.asciidoc

@@ -0,0 +1,16 @@
+[[esql-replace]]
+=== `REPLACE`
+This function substitutes the replacement string (3rd argument) for every occurrence of the regular expression (2nd argument) in the string (1st argument).


Optional alternative: The function substitutes in the string (1st argument) any match of the regular expression (2nd argument) with the replacement string (3rd argument).
I'd find it easier to "read the function" as it flows.

bpintea · 2023-09-29T09:10:43Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Replace.java

+                regexPattern = Pattern.compile(((BytesRef) regex.fold()).utf8ToString());
+            } catch (PatternSyntaxException pse) {
+                // TODO this is not right (inconsistent). See also https://github.com/elastic/elasticsearch/issues/100038
+                // this should generate a header warning and return null (as do the rest of this functionality in evaluators),


IMO throwing here (as it now happens) is what we should align to: the argument (the reg. expression) isn't correct and we should inform the user before execution; less so, but somewhat similar to a syntax error.
We could let that happen maybe even earlier (like when folding). So the try-catch could be dropped, IMO.
(But we can go ahead with merging this as is and revisiting when deciding on #100038).

I am letting this one as is for now. I reckon we'll have a discussion about this general approach (not only here) and whatever we decide then we'll be applied everywhere. I don't like the inconsistent behavior, but I do also see a reasoning in throwing an exception here. I'm very much in favor of the consistency approach, though.

bpintea · 2023-09-29T09:17:56Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/ReplaceTests.java

+        assertThat(process("I love cats and cats are amazing.", "\\bcats\\b", "dogs"), equalTo("I love dogs and dogs are amazing."));
+    }
+
+    public void testUnicode() {


luigidellaquila

LGTM

…esql/replaceFunction

astefan · 2023-09-29T12:34:51Z

@elasticmachine update branch

…esql/replaceFunction

Co-authored-by: Alexandros Batsakis <abatsakis@splunk.com> Co-authored-by: Andrei Stefan <andrei@elastic.co>

Alexandros Batsakis added 3 commits August 25, 2023 19:54

replace

a0aacec

replace regex

ff0117b

bug fixes

95ca036

abatsakis added Team:QL (Deprecated) Meta label for query languages team :Analytics/ES|QL AKA ESQL labels Aug 26, 2023

abatsakis requested review from luigidellaquila and nik9000 August 26, 2023 03:49

elasticsearchmachine added v8.11.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 26, 2023

abatsakis requested review from astefan and bpintea August 26, 2023 15:25

luigidellaquila reviewed Aug 28, 2023

View reviewed changes

abdonpijpelink reviewed Aug 28, 2023

View reviewed changes

Alexandros Batsakis added 2 commits September 2, 2023 11:51

fix conflicts

2673324

review comments

26bc2fc

luigidellaquila reviewed Sep 5, 2023

View reviewed changes

Alexandros Batsakis added 2 commits September 10, 2023 14:53

conflicts

026f493

review comments

a4ff2c9

luigidellaquila added the >enhancement label Sep 20, 2023

luigidellaquila approved these changes Sep 20, 2023

View reviewed changes

astefan added 4 commits September 28, 2023 14:00

Address reviews. Add one more test.

21d421f

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

0b7be69

…esql/replaceFunction

Adjust for handling warnings

a597957

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

27e6a83

…esql/replaceFunction

astefan requested a review from luigidellaquila September 29, 2023 08:27

astefan added the ES|QL-ui Impacts ES|QL UI label Sep 29, 2023

bpintea approved these changes Sep 29, 2023

View reviewed changes

Address reviews

eb4edf4

luigidellaquila approved these changes Sep 29, 2023

View reviewed changes

astefan added 2 commits September 29, 2023 14:38

Fix test

0c58d2a

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

ff664e3

…esql/replaceFunction

elasticmachine and others added 4 commits September 29, 2023 07:34

Merge branch 'main' into esql/replaceFunction

7d54145

Mute failing test

f647aa4

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

ef69970

…esql/replaceFunction

Use Block.Ref

86fc08d

astefan merged commit 2ccdae6 into main Sep 29, 2023

astefan deleted the esql/replaceFunction branch September 29, 2023 14:41

piergm pushed a commit to piergm/elasticsearch that referenced this pull request Oct 2, 2023

Eval REPLACE function (elastic#98909)

12dc934

Co-authored-by: Alexandros Batsakis <abatsakis@splunk.com> Co-authored-by: Andrei Stefan <andrei@elastic.co>

jakelandis pushed a commit to jakelandis/elasticsearch that referenced this pull request Oct 2, 2023

Eval REPLACE function (elastic#98909)

c17073e

Co-authored-by: Alexandros Batsakis <abatsakis@splunk.com> Co-authored-by: Andrei Stefan <andrei@elastic.co>

nik9000 mentioned this pull request Sep 10, 2024

ESQL: Functions! #98545

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eval REPLACE function#98909

Eval REPLACE function#98909
astefan merged 18 commits intomainfrom
esql/replaceFunction

abatsakis commented Aug 26, 2023 •

edited

Loading

cla-checker-service bot commented Aug 26, 2023 •

edited

Loading

github-actions bot commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

luigidellaquila left a comment

luigidellaquila Aug 28, 2023

abdonpijpelink left a comment

abdonpijpelink Aug 28, 2023

luigidellaquila Sep 5, 2023

luigidellaquila left a comment

luigidellaquila Sep 20, 2023

astefan commented Sep 27, 2023

bpintea left a comment

bpintea Sep 29, 2023

bpintea Sep 29, 2023

bpintea Sep 29, 2023

bpintea Sep 29, 2023

astefan Sep 29, 2023

bpintea Sep 29, 2023

luigidellaquila left a comment

astefan commented Sep 29, 2023

Labels

7 participants

	* {@code CsvTests} again.. This should generate an
	* {@code CsvTests} again. This should generate an

Conversation

abatsakis commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cla-checker-service bot commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

github-actions bot commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

elasticsearchmachine commented Aug 26, 2023

luigidellaquila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdonpijpelink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luigidellaquila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Sep 27, 2023

bpintea 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

luigidellaquila left a comment

Choose a reason for hiding this comment

astefan commented Sep 29, 2023

Labels

7 participants

abatsakis commented Aug 26, 2023 •

edited

Loading

cla-checker-service bot commented Aug 26, 2023 •

edited

Loading