Add ES|QL bit_length function #115792

timgrein · 2024-10-28T16:23:27Z

(Spacetime project) :-)

Adds one new string function aka BIT_LENGTH.

Example:

PUT test_index
{
  "mappings": {
    "properties": {
      "keyword_field": {
        "type": "keyword"
      },
      "text_field": {
        "type": "text"
      }
    }
  }
}

POST test_index/_doc
{
    "keyword_field": "abc",
    "text_field": "☕"
}

POST _query
{
  "query": "FROM test_index | EVAL x = bit_length(keyword_field) | EVAL y = bit_length(text_field)"
}

github-actions · 2024-10-28T16:23:40Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-10-28T16:23:51Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-10-28T16:23:52Z

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

timgrein · 2024-10-28T16:25:12Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

+        /**
+         * Support for function {@code GREATEST}. Done in #98630.
+         */
+        FN_GREATEST,


Added this for a bit_length test case, where I use the new function inside another function (see new test case bitLengthInsideOtherFunction in string.csv-spec)

Hey! You shouldn't need this. Those capabilities at the CSV tests are just used to know if a node knows about a feature or not. If a node has BIT_LENGTH, it knows about GREATEST too, so just using BIT_LENGTH should be enough.

Also, GREATEST has been in ESQL since long ago, so adding this now would be weird 👀

Thanks for the explanation :) Adjusted it with Remove FN_GREATEST capability and only use the most recent in bitLeng…

… into esql-string-bit-length-function

timgrein · 2024-10-29T09:45:32Z

@elasticmachine update branch

… into esql-string-bit-length-function

…expression/function/scalar/string/BitLengthTests.java Co-authored-by: Iván Cea Fontenla <[email protected]>

bpintea

Left one noteworthy remark, LG otherwise.

bpintea · 2024-10-30T14:58:56Z

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

+
+    @Evaluator
+    static int process(BytesRef val) {
+        return val.length * Byte.SIZE;


We should use Math.multiplyExact(), as this'll overflow with docs under 300MB. Such large docs might hit other limits before this, but better be safe.
We'll then need to declare the thrown exception in the decorator (and thus let it be transformed into a null by the evaluator).

Good point! Adjusted with Catch ArithmeticException in Evaluator and turn expression result into null value.

Really cool meta-programming stuff you've built with code generation based on annotations 👏

bpintea · 2024-10-30T14:59:47Z

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

+        if (childrenResolved() == false) {
+            return new TypeResolution("Unresolved children");
+        }
+
+        return isString(field(), sourceText(), DEFAULT);


Nit/style optional: could use the ternary operator.

Fair point, adjusted with Use ternary operator for type resolution

bpintea · 2024-10-30T15:02:13Z

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

+    public static Iterable<Object[]> parameters() {
+        List<TestCaseSupplier> suppliers = new ArrayList<>();
+
+        for (DataType stringType : new DataType[] { DataType.KEYWORD, DataType.TEXT }) {


Could use DataType.STRING_TYPES.

Adjusted with Use stringTypes() instead of manually constructing the data type array 👍

bpintea · 2024-10-30T15:06:29Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml

@@ -30,7 +30,7 @@ setup:
        - method: POST
          path: /_query
          parameters: []
-          capabilities: [ snapshot_test_for_telemetry ]
+          capabilities: [ snapshot_test_for_telemetry, fn_bit_length ]


This actually doesn't seem to cut it, still running into the issue that rest compatibility tests for older versions 8.x fail with:

> Task :distribution:bwc:minor:checkoutBwcBranch Performing checkout of elastic/8.x... Checkout hash for :distribution:bwc:minor is 9f8cf46bec9afc64bd4f9d63cb547bea6f65fc51 [...] REPRODUCE WITH: ./gradlew ":x-pack:plugin:yamlRestCompatTest" --tests "org.elasticsearch.xpack.test.rest.XPackRestIT.test {p0=esql/60_usage/Basic ESQL usage output (telemetry) snapshot version}" -Dtests.seed=4FAF1A8CF690062D -Dtests.locale=az-Latn-AZ -Dtests.timezone=Etc/GMT-5 -Druntime.java=22 XPackRestIT > test {p0=esql/60_usage/Basic ESQL usage output (telemetry) snapshot version} FAILED java.lang.AssertionError: Failure at [esql/60_usage:164]: field [esql.functions] doesn't have length [117] Expected: <117> but: was <118>

Any additional ideas @bpintea ?

My first idea would have been to use only fn_bit_length. Gut-feeling wise, that should be the only one needed, but I am not certain and I would try it like this as well and maybe dig a bit deeper to understand exactly why XPackRestIT fails.

Adjusted with Only use most recent capability in usage tests. Let's see ... 👀 If it still fails I'll try to dig in to see what's going wrong

Just to clarify my own understanding of bwc rest compability tests...How does this exactly work? Does the old usage test (from branch 8.x...; basically mimicking an "old" client) gets executed against current main? If so, the observed error would make sense as main has now one ES|QL function more, while the old test expects |esql functions on main| - |functions which were added after 8.x.|. Also adding the fn_bit_length capability wouldn't solve the problem, because this change is not present on the older branch, right?

One potential solution could be to rewrite the test to assert that the response contains (at least) an expected explicit set of functions, rather than asserting on the number of functions. If you keep the number of functions assertion you could also change it to a greater or equal than assertion, which account for the fact that new functions can be added, but no function should be removed in later versions?

Does the old usage test (from branch 8.x...; basically mimicking an "old" client) gets executed against current main?

Yes. This is documented here: "The build system will download the latest prior version of the YAML rest tests and execute them against the current cluster version.".
(I thought it would work as the other BWC tests, that run new queries against old code, but it's the other way around. TIL.)
So indeed, updating the test on main (9.x) has no effect on the test run: we need to update the 8.x test.
@astefan, I think we might need to either skip these tests in REST BWC tests on main, or update the way we check for the functions count on 8.x (which might be tricky, but probably checking against a lower limit might be safe).

Also, this s/snapshot_test_for_telemetry/fn_bit_length is probably then no longer necessary.

bpintea · 2024-10-30T15:13:43Z

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

+
+foldBitLength
+required_capability: fn_bit_length
+row a = 1 | eval b = bit_length("hello");


nit: could be row b = bit_length("hello"), but can stay as is too.

bpintea · 2024-10-30T15:15:13Z

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

+// tag::bitLength[]
+FROM employees
+| KEEP first_name, last_name
+| EVAL fn_bit_length = BIT_LENGTH(first_name)


nit: could add a fn_length = LENGTH(first_name) for a quick /8 (reviewing) check, but can stay as is too.

As these are docs examples it's probably fine to let it solely be focused on bit_length IMHO.

…o a null value

astefan · 2024-10-31T12:43:02Z

@timgrein add the following to elasticsearch\x-pack\plugin\build.gradle in tasks.named("yamlRestCompatTestTransform").configure({ task -> block:

  task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry) snapshot version", "The number of functions is constantly increasing")
  task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry) non-snapshot version", "The number of functions is constantly increasing")

The final block should probably look like

tasks.named("yamlRestCompatTestTransform").configure({ task ->
  task.skipTest("security/10_forbidden/Test bulk response with invalid credentials", "warning does not exist for compatibility")
  task.skipTest("inference/inference_crud/Test get all", "Assertions on number of inference models break due to default configs")
  task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry)", "The telemetry output changed. We dropped a column. That's safe.")
  task.skipTest("esql/80_text/reverse text", "The output type changed from TEXT to KEYWORD.")
  task.skipTest("esql/80_text/values function", "The output type changed from TEXT to KEYWORD.")
  task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry) snapshot version", "The number of functions is constantly increasing")
  task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry) non-snapshot version", "The number of functions is constantly increasing")
})

bpintea · 2024-10-31T12:57:38Z

The final block should probably look like
task.skipTest("esql/60_usage/Basic ESQL usage output (telemetry)", "The telemetry output changed. We dropped a column. That's safe.")

@timgrein, this one line above might be superfluous, after adding the two Andrei suggests. Should be easy to check locally (and then through the CI).

…t number of functions won't work on older versions as soon as you add at least one new function on main.

astefan · 2024-10-31T14:09:41Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml

@@ -30,7 +30,7 @@ setup:
        - method: POST
          path: /_query
          parameters: []
-          capabilities: [ snapshot_test_for_telemetry ]
+          capabilities: [ fn_bit_length ]


@timgrein apologies, I forgot that when I added the two different capabilities I did it with a twist:

SNAPSHOT_TEST_FOR_TELEMETRY(Build.current().isSnapshot()) NON_SNAPSHOT_TEST_FOR_TELEMETRY(Build.current().isSnapshot() == false)

I think these two capabilities must always be placed in those two tests, together with the new capability: capabilities: [ snapshot_test_for_telemetry, fn_bit_length ]

timgrein · 2024-10-31T16:39:49Z

@elasticmachine update branch

astefan

LGTM

astefan · 2024-11-01T08:19:32Z

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

@@ -656,3 +656,21 @@ FROM sample_data

 @timestamp:date | client_ip:ip | event_duration:long | message:keyword
 ;
+
+docsBitLength


I think you need required_capability: fn_bit_length.

… into esql-string-bit-length-function

elasticsearchmachine · 2024-11-07T07:52:48Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Add ES|QL BIT_LENGTH support.

6f59227

timgrein added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.0.0 labels Oct 28, 2024

Update docs/changelog/115792.yaml

3de0143

timgrein commented Oct 28, 2024

View reviewed changes

timgrein added 2 commits October 28, 2024 17:26

Use "str" as typed data name in BitLengthTests

7b69556

Merge remote-tracking branch 'origin/esql-string-bit-length-function'…

a5da6c4

… into esql-string-bit-length-function

timgrein mentioned this pull request Oct 28, 2024

ESQL: Functions! #98545

Open

75 tasks

Update number of functions in 60_usage.yml

7a2ecf9

timgrein added the v8.17.0 label Oct 28, 2024

Merge branch 'main' into esql-string-bit-length-function

cbcd332

timgrein removed the v8.17.0 label Oct 29, 2024

Merge branch 'main' into esql-string-bit-length-function

2621b3c

timgrein added auto-backport Automatically create backport pull requests when merged v8.16.0 labels Oct 29, 2024

Merge branch 'main' into esql-string-bit-length-function

ae82768

timgrein added v8.17.0 auto-backport Automatically create backport pull requests when merged v9.0.0 and removed auto-backport Automatically create backport pull requests when merged v8.16.0 v9.0.0 labels Oct 29, 2024

timgrein added 2 commits October 29, 2024 11:00

Add FN_BIT_LENGTH capability and guard csv tests accordingly

2583805

Merge remote-tracking branch 'origin/esql-string-bit-length-function'…

4626195

… into esql-string-bit-length-function

timgrein and others added 2 commits October 30, 2024 13:22

Update x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/…

b800d4f

…expression/function/scalar/string/BitLengthTests.java Co-authored-by: Iván Cea Fontenla <[email protected]>

Add bit length capability to usage integration tests

db02eb7

bpintea approved these changes Oct 30, 2024

View reviewed changes

timgrein added 4 commits October 30, 2024 17:07

Catch ArithmeticException in Evaluator and turn expression result int…

3cdde43

…o a null value

Use stringTypes() instead of manually constructing the data type array

f4c0abe

Use ternary operator for type resolution

a9cfaf0

Only use most recent capability in usage tests

762be47

timgrein added 2 commits October 31, 2024 14:19

Ignore ESQL usage output tests on older branches as checking the exac…

9c9511b

…t number of functions won't work on older versions as soon as you add at least one new function on main.

Keep ignore old test for now

6dbadf2

astefan reviewed Oct 31, 2024

View reviewed changes

timgrein added 2 commits October 31, 2024 15:36

Fix expected function number for current main

b88922e

Reintroduce snapshot capabilities into usage tests

d868e08

Merge branch 'main' into esql-string-bit-length-function

989c770

astefan approved these changes Nov 1, 2024

View reviewed changes

elasticmachine and others added 4 commits November 6, 2024 16:42

Merge branch 'main' into esql-string-bit-length-function

b36fa7b

Add required_capability: fn_bit_length to docs.csv-spec

c708715

Merge remote-tracking branch 'origin/esql-string-bit-length-function'…

7f480bd

… into esql-string-bit-length-function

Merge branch 'main' into esql-string-bit-length-function

28ad3e9

timgrein merged commit 81fd1de into elastic:main Nov 7, 2024
16 checks passed

timgrein mentioned this pull request Nov 7, 2024

[8.x] Add ES|QL bit_length function (#115792) #116378

Merged

timgrein added a commit to timgrein/elasticsearch that referenced this pull request Nov 7, 2024

Add ES|QL bit_length function (elastic#115792)

a2e3b2f

elasticsearchmachine pushed a commit that referenced this pull request Nov 7, 2024

Add ES|QL bit_length function (#115792) (#116378)

b7951c5

kderusso pushed a commit to kderusso/elasticsearch that referenced this pull request Nov 7, 2024

Add ES|QL bit_length function (elastic#115792)

232ff28

luigidellaquila mentioned this pull request Nov 8, 2024

ES|QL: Fix funciton Telemetry tests #116470

Merged

jozala pushed a commit that referenced this pull request Nov 13, 2024

Add ES|QL bit_length function (#115792)

2655bc6

alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024

Add ES|QL bit_length function (elastic#115792)

430c485

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ES|QL bit_length function #115792

Add ES|QL bit_length function #115792

timgrein commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024

elasticsearchmachine commented Oct 28, 2024

elasticsearchmachine commented Oct 28, 2024

timgrein Oct 28, 2024

ivancea Oct 29, 2024

timgrein Oct 30, 2024

timgrein commented Oct 29, 2024

bpintea left a comment

bpintea Oct 30, 2024

timgrein Oct 30, 2024

bpintea Oct 30, 2024

timgrein Oct 30, 2024

bpintea Oct 30, 2024

timgrein Oct 30, 2024

bpintea Oct 30, 2024

timgrein Oct 30, 2024

astefan Oct 30, 2024

timgrein Oct 30, 2024

timgrein Oct 31, 2024 •

edited

Loading

bpintea Oct 31, 2024

bpintea Oct 31, 2024

bpintea Oct 30, 2024

bpintea Oct 30, 2024

timgrein Oct 30, 2024

astefan commented Oct 31, 2024

bpintea commented Oct 31, 2024

astefan Oct 31, 2024

timgrein commented Oct 31, 2024

astefan left a comment

astefan Nov 1, 2024

elasticsearchmachine commented Nov 7, 2024

Add ES|QL bit_length function #115792

Add ES|QL bit_length function #115792

Conversation

timgrein commented Oct 28, 2024 • edited Loading

github-actions bot commented Oct 28, 2024

elasticsearchmachine commented Oct 28, 2024

elasticsearchmachine commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgrein commented Oct 29, 2024

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

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

timgrein Oct 31, 2024 • edited Loading

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

astefan commented Oct 31, 2024

bpintea commented Oct 31, 2024

Choose a reason for hiding this comment

timgrein commented Oct 31, 2024

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Nov 7, 2024

💚 Backport successful

timgrein commented Oct 28, 2024 •

edited

Loading

timgrein Oct 31, 2024 •

edited

Loading