[ES|QL] add reverse function by drewdaemon · Pull Request #113297 · elastic/elasticsearch

drewdaemon · 2024-09-20T17:50:21Z

Part of #98545

Adds a REVERSE function to the ES|QL language.

REVERSE reverses the characters in a string. If you want to know more, read the docs!

elasticsearchmachine · 2024-09-23T20:33:48Z

Hi @drewdaemon, I've created a changelog YAML for you.

…n/elasticsearch into 98545/reverse-function

…98545/reverse-function

costin · 2024-09-23T23:23:18Z

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

+message:keyword | message_reversed:keyword
+😂😁😅😐🥺😢😭 | 😭😢🥺😐😅😁😂
+// end::reverseEmoji-result[]


Nice - small comment, I would use different emoticons to better signal visually the reversal: 🏡🚌✈ becomes ✈ 🚌🏡

Additionally add also a string that combines and intertwines regular chars + emoticons + extended ones (こんにちは) and check they are properly reversed (namely that the encoding is preserved).

Nice - small comment, I would use different emoticons to better signal visually the reversal

I think of the current ones like a sort of easter egg in the docs... you have to squint a bit and then you're rewarded with "getting it."

That said, I defer to you — LMK

cc @leemthompo in case he has input

I think Costin's right that it would be easier with a bit more visual variance in the emojis but we're knee deep in nit territory here 😆

Actually Liam this is the most important conversation going on in this PR right now 😆

Okay, I'll take another stab at emoji art 🖼️

My battle for these emojis is met with opposition at every turn... first it was our handling of unicode graphemes and now it's a custom font on the docs website... will I prevail?....

Screen.Recording.2024-09-26.at.4.01.39.PM.mov

drewdaemon · 2024-09-27T17:22:58Z

@nik9000 @ivancea I think I have addressed all your feedback. Would you take another look?

nik9000

👍

nik9000 · 2024-09-27T17:42:07Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java

-                def(Locate.class, Locate::new, "locate"),
-                def(Repeat.class, Repeat::new, "repeat"),
-                def(Space.class, Space::new, "space") },
+                def(Trim.class, Trim::new, "trim") },


Thanks for sorting!

All JetBrains :)

…98545/reverse-function

bpintea

Some minor/optional drive-by comments only.

bpintea · 2024-09-30T13:55:32Z

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

+        return reversed.toString();
+    }
+
+    private static boolean isOneByteUTF8(BytesRef ref) {


Could also be isASCII().

I understood ASCII vs unicode to be distinct from the UTF encoding so I tried to be specific.

Let me know if I misunderstood!

I've used words like "low ascii" or "one bytes utf-8" for the same thing. Either is fine by me. But in grand java tradition - what you've written is quite descriptive.

Maybe isUTF8ASCII or something?

I find isOneByteUTF8 clearer than other alternatives.
UTF8 encoding code points corresponding to ASCII on one byte is known, but maybe not "well enough" known. Let's leave it as is.

I think isOneByteUTF8 the best, yeah.

bpintea · 2024-09-30T13:59:35Z

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

+    private static void reverseArray(byte[] array, int start, int end) {
+        while (start < end) {
+            byte temp = array[start];
+            array[start] = array[end];
+            array[end] = temp;
+            start++;
+            end--;
+        }
+    }


Would be nice to add this back to org.elasticsearch.common.util.ArrayUtils.

Done in 6c09607

bpintea · 2024-09-30T14:00:19Z

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

+        if (isOneByteUTF8(val)) {
+            // this is the fast path. we know we can just reverse the bytes.
+            BytesRef reversed = BytesRef.deepCopyOf(val);
+            reverseArray(reversed.bytes, reversed.offset, reversed.offset + reversed.length - 1);


This scans an ASCII text for three times (even though not necessarily byte-by-byte).
I guess the first pass could attempt a reverse already, though that would imply allocation for non-ASCII text.
I guess it's fine as is, probably not making a practical difference either way, can be optimised if ever needed.

True. At least we're in O(n).

can be optimised if ever needed

Agreed

drewdaemon · 2024-09-30T14:51:01Z

Hmmm, these failing tests appear to be a Catch-22 centered on this CSV test.

When I run ./gradlew :x-pack:plugin:esql:test --tests "org.elasticsearch.xpack.esql.CsvTests" it expects both the first and second REVERSE calls the in the query to produce multi-field warnings. But when I run ./gradlew :x-pack:plugin:esql:qa:server:single-node:javaRestTest it expects only the second call in the query the produce the warnings.

I'm not really sure what to do about this. Could someone take a look?

nik9000 · 2024-09-30T14:58:27Z

Catch-22 centered

That's the best catch.

I'll look.

drewdaemon · 2024-09-30T20:47:01Z

Okay, now it looks like the warning returned in the multi-node scenario is not identifying the line and column number correctly:

Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.

In the CSV test, the warning is Line 1:53: evaluation of [REVERSE(job_positions)] failed, treating result as null. Only first 20 failures recorded. which is correct.

nik9000 · 2024-09-30T20:48:13Z

Ah! That's you actually! I'll link.

nik9000 · 2024-09-30T20:50:08Z

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

+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeNamedWriteable(field());


Add source.writeTo(out) and read it with this(Source.readFrom((PlanStreamInput) in), in.readNamedWriteable(Expression.class). Then you'll need to flip the test - it is setup with that alwaysempty thing.

After that the source should appear.

This is a left over from when serializing Source was really expensive. Back then we didn't serialize it. We should serialize it because it's really cheap now - just a few bytes.

I realized UnaryScalarFunction already takes care of this, so I simplified the constructor in ad7c68f

…98545/reverse-function

elasticsearchmachine · 2024-10-04T17:59:12Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Adds a REVERSE string function

first classes

88de8b7

elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v9.0.0 labels Sep 20, 2024

drewdaemon mentioned this pull request Sep 20, 2024

ESQL: Functions! #98545

Open

drewdaemon added 12 commits September 20, 2024 12:04

syntax error fix

9588aca

update some tests

5a5c383

register the writeable

590e0e4

implement reverse and add tests

2d0b623

add unit tests

5a588fe

update test

19c60ee

add some docs

6a7bf6b

more docs

23bc6ee

fix some docs

03e9c12

add capability test gating

67b228f

add serialization test

647902a

update instruction manual

bceb604

drewdaemon added >enhancement :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI and removed external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 23, 2024

drewdaemon added 7 commits September 23, 2024 14:33

Update docs/changelog/113297.yaml

27716dd

update labeling instructions

234b795

Merge branch '98545/reverse-function' of https://github.com/drewdaemo…

f1c4694

…n/elasticsearch into 98545/reverse-function

add an example

d900561

docs update

2c1bbf4

formatting fix

39f2e1b

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

ead4a09

…98545/reverse-function

costin reviewed Sep 23, 2024

View reviewed changes

drewdaemon marked this pull request as ready for review September 24, 2024 01:38

drewdaemon requested review from ivancea and nik9000 September 27, 2024 17:17

nik9000 approved these changes Sep 27, 2024

View reviewed changes

drewdaemon added 2 commits September 27, 2024 12:15

update order

8ce4406

update test

8cad604

ivancea approved these changes Sep 30, 2024

View reviewed changes

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

28c8d8b

…98545/reverse-function

drewdaemon added the auto-backport-and-merge label Sep 30, 2024

bpintea approved these changes Sep 30, 2024

View reviewed changes

narrow tests

e21f87f

nik9000 reviewed Sep 30, 2024

View reviewed changes

drewdaemon added 8 commits October 3, 2024 13:56

utilize UnaryScalarFunction methods where possible

ad7c68f

move reverseArray to the utils class

6c09607

spotless

b512026

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

247495a

…98545/reverse-function

update serialization tests

7763155

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

9041899

…98545/reverse-function

update evaluator call signature

43a63e7

spotless

9c11030

drewdaemon merged commit 147461f into elastic:main Oct 4, 2024

drewdaemon mentioned this pull request Oct 4, 2024

[8.x] [ES|QL] add reverse function (#113297) #114163

Merged

drewdaemon added a commit to drewdaemon/elasticsearch that referenced this pull request Oct 4, 2024

[ES|QL] add reverse function (elastic#113297)

43006a4

Adds a REVERSE string function

elasticsearchmachine pushed a commit that referenced this pull request Oct 4, 2024

[ES|QL] add reverse function (#113297) (#114163)

2046b49

Adds a REVERSE string function

matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 10, 2024

[ES|QL] add reverse function (elastic#113297)

dfc8fb3

Adds a REVERSE string function

Conversation

drewdaemon commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 23, 2024

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!

drewdaemon Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewdaemon commented Sep 27, 2024

Uh oh!

nik9000 left a comment

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!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewdaemon Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

drewdaemon commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Sep 30, 2024

Uh oh!

drewdaemon commented Sep 30, 2024

Uh oh!

nik9000 commented Sep 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 4, 2024

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

drewdaemon commented Sep 20, 2024 •

edited

Loading

drewdaemon Sep 26, 2024 •

edited

Loading

drewdaemon Sep 30, 2024 •

edited

Loading

drewdaemon commented Sep 30, 2024 •

edited

Loading