[ES|QL] Annotate all functions #103686

dej611 · 2023-12-22T12:06:35Z

Annotate all functions to have full signature available in SHOW FUNCTIONS.

All available functions are now correctly annotated with types, and an additional isAggregation column has been added to the returned table.

Few assumptions made for the annotations:

all functions who accept text now have arguments of type keyword|text, unless they accepts enum strings in which case I've used the keyword type only.
in the case described above, I've tried to annotate as argument description the possible values (see date_extract)
I had to slightly tweak some description copy changing from commas to semi-colon as there's a bug in the CSV parser used in tests (thanks @luigidellaquila for debugging this) cc @abdonpijpelink

There are few notable exceptions to the annotation completeness who I have not been able to fix, so I've partially annotated them:

coalesce
~~mv_sum~~ updated with recent main commit.
round
mv_dedupe

I'll create a separate issue for them.

…ions-annotated

elasticsearchmachine · 2024-01-10T14:47:34Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

nik9000

How thoughtful! Thanks so much for this. I've left a small comment about your change to the length tests, but otherwise it's great.

I admit I haven't looked super closely at the words, but just having words is a huge improvement over where we are.

nik9000 · 2024-01-10T15:17:00Z

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

+            new TestCaseSupplier("6 bytes, 2 code points", () -> makeTestCaseAsKeyword("❗️", 2)),
+            new TestCaseSupplier("100 random alpha", () -> makeTestCaseAsKeyword(randomAlphaOfLength(100), 100)),
+            new TestCaseSupplier("100 random code points", () -> makeTestCaseAsKeyword(randomUnicodeOfCodepointLength(100), 100)),
+            new TestCaseSupplier("ascii string as text", () -> makeTestCaseAsText("clump", 5))


I suppose we could make both text and keyword in the function and return both.

It tends to be easier to do:

List<TestCase> cases = new ArrayList<>(); cases.addAll(makeTestCases("empty string", 0, () -> "")); cases.addAll(makeTestCases("100 random alpha", 100, () -> randomAlphaOfLength(100)));

I used to do with the List.of but for these it's just a pain.

I see what you mean, so makeTestCases would produce two tests. Ok, I'll update it.

Fixed by cf5061a . Thanks @luigidellaquila for support and patience 😅 .

nik9000 · 2024-01-10T15:18:53Z

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

-to_ver                   |"version to_ver(v:keyword|text|version)"                                                         |v   |"keyword|text|version"                                                                                     |                                                    |version                          |                                                                                       |false                       |false          
-to_version               |"version to_version(v:keyword|text|version)"                                                     |v   |"keyword|text|version"                                                                                     |                                                    |version                          |                                                                                       |false                       |false          
-trim                     |"keyword|text trim(str:keyword|text)"                   |str                      |"keyword|text"    | ""                                                 |"keyword|text"       |Removes leading and trailing whitespaces from a string.| false | false
+       name:keyword      |                        synopsis:keyword          |       argNames:keyword  | argTypes:keyword |             argDescriptions:keyword                |returnType:keyword   |    description:keyword  | optionalArgs:boolean | variadic:boolean | isAggregation:boolean               


I'm sorry for how this test ends up looking. It's quite a lot.

I was surprised to see that there's no update task for this kind of snapshot tests. I think it would be feasible to create one.

Yeah. I do these one at a time so I just manage it by hand. It isn't like this blob is really code reviewable either.

I do think we should do something about this. Or at least think about it.

nik9000 · 2024-01-10T15:19:13Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/show/ShowFunctions.java

@@ -63,6 +63,7 @@ public List<List<Object>> values(FunctionRegistry functionRegistry) {
            row.add(signature.description());
            row.add(collect(signature, EsqlFunctionRegistry.ArgSignature::optional));
            row.add(signature.variadic());
+            row.add(signature.isAggregation());


@luigidellaquila is this one ok with you?

Yes, absolutely fine

nik9000

Looks good to me.

nik9000 · 2024-01-10T19:36:07Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvCount.java

-                "cartesian_point" }
+                "text",
+                "unsigned_long",
+                "version" }


I wonder if we can write a checkstyle rule that forces these to be alphabetical!

luigidellaquila

LGTM, thank you very much Marco!

* Functions E-Z * Incorporate changes from #103686 * More functions * More functions * Update docs/reference/esql/functions/floor.asciidoc Co-authored-by: Liam Thompson <[email protected]> * Update docs/reference/esql/functions/left.asciidoc Co-authored-by: Liam Thompson <[email protected]> * Apply suggestions from code review Co-authored-by: Alexander Spies <[email protected]> * Review feedback * Fix geo_shape description * Change 'colum'/'field' into 'expressions' * Review feedback * One more --------- Co-authored-by: Liam Thompson <[email protected]> Co-authored-by: Alexander Spies <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…04623) * Functions E-Z * Incorporate changes from elastic#103686 * More functions * More functions * Update docs/reference/esql/functions/floor.asciidoc Co-authored-by: Liam Thompson <[email protected]> * Update docs/reference/esql/functions/left.asciidoc Co-authored-by: Liam Thompson <[email protected]> * Apply suggestions from code review Co-authored-by: Alexander Spies <[email protected]> * Review feedback * Fix geo_shape description * Change 'colum'/'field' into 'expressions' * Review feedback * One more --------- Co-authored-by: Liam Thompson <[email protected]> Co-authored-by: Alexander Spies <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

🔧 Initial annotations

7a614d3

elasticsearchmachine added v8.13.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 22, 2023

luigidellaquila added the :Analytics/ES|QL AKA ESQL label Dec 22, 2023

dej611 and others added 23 commits January 8, 2024 10:49

✏️ Add substring doc

9b82c4c

Merge remote-tracking branch 'upstream/main' into fix/esql-show-funct…

0573c2b

…ions-annotated

🏷️ Fix imports

7039041

✏️ Add annotations for string fns

b361996

♻️ Fix issues

8f3a131

:rotatin_light: Fix linting issues

0013bb8

✏️ Add more annotations

c53c6a1

:rotatin_light: Fix linting issues

5fa282d

♻️ Add missing import

c757569

✏️ Add more annotations

08eb187

Merge branch 'main' into fix/esql-show-functions-annotated

1d18eb0

🚨 Fix linting

f608cc8

✅ Fix many tests for new annotations

04fe821

✅ Fix more tests

9673fb0

Merge remote-tracking branch 'upstream/main' into fix/esql-show-funct…

b336779

…ions-annotated

🐛 Fix missing flag

7300e3e

✨ Annotate new functions

d0dc3d7

✅ fix more tests

2a6db92

✅ Fix signature issue

8d8285d

Merge remote-tracking branch 'upstream/main' into fix/esql-show-funct…

470b83c

…ions-annotated

✅ Make all csv tests pass

6102af2

✅ Fix remaining tests

d65b706

✨ New assets from annotations

8a45103

dej611 marked this pull request as ready for review January 10, 2024 14:47

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 10, 2024

luigidellaquila added the >non-issue label Jan 10, 2024

nik9000 reviewed Jan 10, 2024

View reviewed changes

✅ Refactor test

cf5061a

nik9000 approved these changes Jan 10, 2024

View reviewed changes

dej611 and others added 2 commits January 11, 2024 07:31

Merge branch 'main' into fix/esql-show-functions-annotated

ffe20e6

✅ Fix updated signature

553ebc0

This was referenced Jan 11, 2024

[ES|QL] Automate part of doc generation via annotations #104247

Open

[ES|QL] Complete annotation on remaining functions #104248

Closed

luigidellaquila approved these changes Jan 11, 2024

View reviewed changes

dej611 merged commit 3145e9f into elastic:main Jan 11, 2024
15 checks passed

This was referenced Jan 11, 2024

[ES|QL] Make type and function description required annotations #104249

Merged

[ES|QL][Meta] Keep in sync with ES elastic/kibana#173817

Closed

abdonpijpelink added a commit to abdonpijpelink/elasticsearch that referenced this pull request Jan 15, 2024

Incorporate changes from elastic#103686

0d2dfe1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Annotate all functions #103686

[ES|QL] Annotate all functions #103686

dej611 commented Dec 22, 2023 •

edited

Loading

elasticsearchmachine commented Jan 10, 2024

nik9000 left a comment

nik9000 Jan 10, 2024

dej611 Jan 10, 2024 •

edited

Loading

dej611 Jan 10, 2024

nik9000 Jan 10, 2024

dej611 Jan 10, 2024

nik9000 Jan 10, 2024

nik9000 Jan 10, 2024

luigidellaquila Jan 10, 2024

nik9000 left a comment

nik9000 Jan 10, 2024

luigidellaquila left a comment

[ES|QL] Annotate all functions #103686

[ES|QL] Annotate all functions #103686

Conversation

dej611 commented Dec 22, 2023 • edited Loading

elasticsearchmachine commented Jan 10, 2024

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 Jan 10, 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

Choose a reason for hiding this comment

nik9000 left a 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

dej611 commented Dec 22, 2023 •

edited

Loading

dej611 Jan 10, 2024 •

edited

Loading