Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Generate kibana inline docs #106782

Merged
merged 25 commits into from
Apr 9, 2024

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 26, 2024

This takes a stab at generating the markdown files that Kibana uses for its inline help. It doesn't include all of the examples because the @Example annotation is not filled in - we're tracking that in #104247 (comment)

There are some links in the output and they are in markdown syntax. We should figure out how to make them work for kibana.

This takes a stab at generating the markdown files that Kibana uses for
its inline help. It doesn't include all of the examples because the
`@Example` annotation is not filled in - we're tracking that in
elastic#104247 (comment)

There are some links in the output and they are in markdown syntax. We
should figure out how to make them work for kibana.
@nik9000 nik9000 added >docs General docs changes :Analytics/ES|QL AKA ESQL v8.14.0 labels Mar 26, 2024
@nik9000 nik9000 requested a review from a team as a code owner March 26, 2024 16:56
@elasticsearchmachine elasticsearchmachine added Team:Docs Meta label for docs team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Mar 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

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

@nik9000
Copy link
Member Author

nik9000 commented Mar 26, 2024

Wait. Silly test security exception. Fixing.

@nik9000
Copy link
Member Author

nik9000 commented Mar 26, 2024

Wait. Silly test security exception. Fixing.

Done.

builder.append(info.description()).append("\n\n");

if (info.examples().length > 0) {
Example example = info.examples()[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have all examples. But ok for the first one as first step.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to include all of them if you want. I just didn't see a place where you had more than one.

@nik9000 nik9000 requested a review from dej611 April 3, 2024 13:52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me.
auto_bucket has 4 arguments (field, buckets, from, to) while here there are 4 different signatures only with the field argument. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the auto_bucket is not ticked in the related issue. Is that because of it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll bet it's a funny way we're testing it. Checking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It's because we're testing this in a weird weird way. I should fix the way we test this.

Now, here's an interesting question - can I check these? If a function isn't variadic I can just assert that the sizes are the same.

@dej611
Copy link
Contributor

dej611 commented Apr 3, 2024

The shape looks correct to me, probably this requires also @drewdaemon to have a look as he's now in charge of the es|ql project on the kibana side.
I think it makes sense to merge this only when all definitions are complete: what worries me more than missing signature is actually incomplete signatures which can have a big impact on the validation side.

@nik9000
Copy link
Member Author

nik9000 commented Apr 3, 2024

I think it makes sense to merge this only when all definitions are complete: what worries me more than missing signature is actually incomplete signatures which can have a big impact on the validation side.

From my side it'd be better to merge it but have y'all not use it. That way we can look at what it builds and say "this bit isn't ready". But I'll do whatever's easiest for you.

@dej611
Copy link
Contributor

dej611 commented Apr 3, 2024

As long as we receive a ping on when it's complete I guess it's ok for us. But let's check @drewdaemon 's preference.

@drewdaemon
Copy link
Contributor

Thanks for the ping @dej611

@nik9000 feel free to merge whenever you want to. Like you say, we can always refine in further PRs as I start pulling this (amazing) work into Kibana.

Comment on lines 6 to 18
"signatures" : [
{
"params" : [
{
"name" : "field",
"type" : "datetime",
"optional" : false,
"description" : ""
}
],
"variadic" : false,
"returnType" : "datetime"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the docs, this function has 4 parameters but I only see one here.

@nik9000
Copy link
Member Author

nik9000 commented Apr 5, 2024

@luigidellaquila While working on this I found a bug in the way we decided whether or not to check if the annotations line up with the tested signatures - it'd only do it if there was more than one tested signature. So there were some cases where we weren't running it. That is my fault. I made the mistake a while back. Anyway! I reworked it. There's one test that was failing - CASE - it just has tests for KEYWORD fields. But I'll add better tests in a follow up.

Could you have a look at this PR?

Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 8, 2024
This improves the tests for AUTO_BUCKET marginally, specifically so that
it tests all valid combinations of arguments and generates a correct
types table. This'll combine nicely with elastic#106782 to generate the
signatures that kibana needs for it's editor.
@nik9000
Copy link
Member Author

nik9000 commented Apr 8, 2024

I'm going to merge main into this, rebuild everything, and then get this landed. We can start playing with it in Kibana from there.

@nik9000
Copy link
Member Author

nik9000 commented Apr 8, 2024

Thanks for all the feedback friends! We'll iterate on the fun around CASE and AUTO_BUCKET soon. And aggs. And operators.

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 8, 2024
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 9, 2024
This moves the test cases declared in the tests for ESQL's LOCATE
function to test cases which will cause elastic#106782 to properly generate all
of the available signatures. It also buys us all of testing for
incorrect parameter combinations.
elasticsearchmachine pushed a commit that referenced this pull request Apr 9, 2024
This improves the tests for AUTO_BUCKET marginally, specifically so that
it tests all valid combinations of arguments and generates a correct
types table. This'll combine nicely with #106782 to generate the
signatures that kibana needs for it's editor.
@nik9000
Copy link
Member Author

nik9000 commented Apr 9, 2024

Grumble grumble ci

nik9000 added a commit that referenced this pull request Apr 9, 2024
This moves the test cases declared in the tests for ESQL's LOCATE
function to test cases which will cause #106782 to properly generate all
of the available signatures. It also buys us all of testing for
incorrect parameter combinations.
@elasticsearchmachine elasticsearchmachine merged commit 96227a1 into elastic:main Apr 9, 2024
14 checks passed
@nik9000 nik9000 deleted the esql_kibana_inline branch April 9, 2024 18:20
@nik9000
Copy link
Member Author

nik9000 commented Apr 9, 2024

@drewdaemon - it looks like the signature we generate for the ROUND function isn't correct - it's missing a lot of supported types. Working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >docs General docs changes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Docs Meta label for docs team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants