Skip to content

Conversation

carlosdelest
Copy link
Member

After conversations in this PR, we'll be adding docs generation to functions available on snapshot builds.

This PR adds that capability, and includes the generated docs for the Categorize function

Copy link
Contributor

Documentation preview:

@carlosdelest carlosdelest requested a review from a team September 18, 2024 07:21
@carlosdelest carlosdelest added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >non-issue :Analytics/ES|QL AKA ESQL labels Sep 18, 2024
@carlosdelest carlosdelest marked this pull request as ready for review September 18, 2024 07:23
@elasticsearchmachine
Copy link
Collaborator

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

@ioanatia
Copy link
Contributor

cc @jan-elastic since the categorize function was added in #112860

there is also the rate function that is available in snapshots only - @dnhatn is it necessary we generate Kibana docs for rate as well?

@carlosdelest
Copy link
Member Author

there is also the rate function that is available in snapshots only - @dnhatn is it necessary we generate Kibana docs for rate as well?

Rate still has no AbstractFunctionTestCase implementation - no docs can be generated from it yet.

Copy link
Member

@fang-xing-esql fang-xing-esql left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @carlosdelest !

Copy link
Contributor

Choose a reason for hiding this comment

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

For this to work and be in the docs, it also has to be linked in one of the root asciidocs.
In this case, it would be this one: https://github.com/elastic/elasticsearch/blob/a21242054b87e456f6e301dfa017e612b83ebbd6/docs/reference/esql/functions/grouping-functions.asciidoc

That said, do we want to include snapshot-only functions in the global docs?

Copy link
Member

Choose a reason for hiding this comment

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

Making the files without plugging them in shouldn't hurt anything if that's what we want to do. I think we're ok doing that. I am. We should just make sure folks know having the generated asciidoc doesn't mean it's there - you have to look for the inclusion. For qstr I believe the plan is to include it only in snapshot docs. INLINESTATS works that way:

ifeval::["{release-state}"=="unreleased"]
* experimental:[] <<esql-inlinestats-by>>
endif::[]

...

ifeval::["{release-state}"=="unreleased"]
include::processing-commands/inlinestats.asciidoc[]
endif::[]

Copy link
Contributor

Choose a reason for hiding this comment

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

Making the files without plugging them in shouldn't hurt anything

Indeed, but I feel like adding those without actually including them leaves things half-done. And it's harder later to know which ones are missing.

Btw, didn't know we had those conditionals! I'd include it with them, and that's all. About making sure it's done, it feels a bit more complex, and I can't think of a good way that doesn't pass through autogenerating those files too 👀 (Which feels like the eventual solution...)

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct - for now, we're not including the docs as these functions are not production ready yet.

Including functions docs should be ok if they are not linked to the global docs - they can exist but not be connected until we do so for production readiness.

@carlosdelest carlosdelest merged commit 838b5a8 into elastic:main Sep 19, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants