-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Refine ESQL docs handling of applies_to #125835
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
Refine ESQL docs handling of applies_to #125835
Conversation
|
Pinging @elastic/es-docs (Team:Docs) |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| ::::""", | ||
| type = FunctionType.AGGREGATE, | ||
| examples = @Example(file = "string", tag = "values-grouped"), | ||
| appliesTo = { @FunctionAppliesTo(lifeCycle = FunctionAppliesToLifecycle.PREVIEW) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has both the old and new approaches to tech-preview, so I removed the new one until we make a final decision on how to deal with tech-preview warnings in general. They are all over the place, not just here, so this function was just an experiment to see what it looked like.
Any class that extends Function needs to correctly implement replaceChildren and info() methods, because it will be found and tested!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, I didn't deeply review the java code but the generated output looks good to me (as a non-docs person).
| :::{note} | ||
| ###### COMING 9.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to say that this looks less confusing to me than having the COMING 9.1.0 inside the tech preview warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
...lugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/DocsV3Support.java
Outdated
Show resolved
Hide resolved
...lugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/DocsV3Support.java
Show resolved
Hide resolved
…icsearch into esql_docs_applies_to
…expression/function/DocsV3Support.java Co-authored-by: Alexander Spies <[email protected]>
…icsearch into esql_docs_applies_to
| stack: preview 9.0, coming 9.1 | ||
| serverless: preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this maybe shouldn't be in the tech preview warning, but I don't know if these tags should be removed altogether. Maybe @leemthompo can suggest a better position?
|
|
||
| ::::{warning} | ||
| ```{applies_to} | ||
| stack: preview 9.0, coming 9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch. This is in preview way longer than 9.0, and it's not coming to 9.1 as far as I know.
MV_EXPAND was already in place in 8.11, when ESQL went into tech preview - at least there's an 8.11 PR that mentions it: #100598
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these originally as examples to discuss with people, but failed to remove them. Based on current discussions, we should remove any version statements, even if the feature is available in serverless, as there is no guarrantee if it will come out in any particular stateful version.
| ::::{warning} | ||
| ```{applies_to} | ||
| stack: preview 9.0, coming 9.1 | ||
| serverless: preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this tag should still apply (but I agree this shouldn't be placed here).
💔 Backport failed
You can use sqren/backport to manually backport by running |
This primarily splits the old preview:true warning from the newer applies_to approach. Since all of our current applies_to examples are actually just behaviour modifications of current functions, we do not use the official docs {applies_to} syntax. However there is code to make use of that in the case where we have an entirely new function which will appear in a new version.
Co-authored-by: Alexander Spies <[email protected]>
This primarily splits the old preview:true warning from the newer applies_to approach. Since all of our current applies_to examples are actually just behaviour modifications of current functions, we do not use the official docs {applies_to} syntax. However there is code to make use of that in the case where we have an entirely new function which will appear in a new version.
Co-authored-by: Alexander Spies <[email protected]>
|
Manual backport in #125923 |
This primarily splits the old
preview:truewarning from the newerapplies_toapproach. Since all of our currentapplies_toexamples are actually just behaviour modifications of current functions, we do not use the official docs{applies_to}syntax. However there is code to make use of that in the case where we have an entirely new function which will appear in a new version.Examples:
An example of a function which has both preview and applies to is:
There are many pre-existing examples of preview only, which we've left as is.
An example of applies_to without preview is: