-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Mark new signatures in MIN and MAX #132980
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
Conversation
The `MIN` and `MAX` functions support unsigned longs in 9.2.0. The docs just show it's that unsigned long is supported but you can't tell for what version. This fixes that, applying the version. The version table is generated from the tests. So we have to annotate some test cases with `applies_to` and pass it through the test infrastructure.
|
Pinging @elastic/es-docs (Team:Docs) |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| | long | long | | ||
| | text | keyword | | ||
| | unsigned_long | unsigned_long | | ||
| | unsigned_long{applies_to}`stack: ga 9.2.0` | unsigned_long | |
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.
The actual generated changes look like this
| /** This test generates documentation for the supported output types of the lookup join. */ | ||
| public void testOutputSupportedTypes() throws Exception { | ||
| Map<List<DataType>, DataType> signatures = new LinkedHashMap<>(); | ||
| Map<List<DocsV3Support.Param>, DataType> signatures = new LinkedHashMap<>(); |
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.
All of these s/DataType/DocsV3Support.Param/ exist so I can plumb FunctionAppliesTo information through the test case instead of an annotation. That's because we generate the type signatures entirely from test cases - no annotation to scan.
| Stream.of( | ||
| MultiRowTestCaseSupplier.intCases(1, 1000, Integer.MIN_VALUE, Integer.MAX_VALUE, true), | ||
| MultiRowTestCaseSupplier.longCases(1, 1000, Long.MIN_VALUE, Long.MAX_VALUE, true), | ||
| MultiRowTestCaseSupplier.ulongCases(1, 1000, BigInteger.ZERO, UNSIGNED_LONG_MAX, true), |
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 is the actual fix - moving the signatures from here to below with appliesTo(FunctionAppliesToLifecycle.GA, "9.2.0", "", true) stuck to them.
| | long | long | | ||
| | text | keyword | | ||
| | unsigned_long | unsigned_long | | ||
| | unsigned_long{applies_to}`stack: ga 9.2.0` | unsigned_long | |
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.
total hygiene nit: I wonder if we'd want a whitespace here before the {applies_to}s ?
Again hope this wasn't too painful to wire up 😿
bpintea
left a comment
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.
Lgtm.
| if (functionAppliesTos.length > 0) { | ||
| appliesToText.append("```{applies_to}\n"); | ||
| if (false == functionAppliesTos.isEmpty()) { | ||
| appliesToText.append(oneLine ? "{applies_to}`" : "```{applies_to}\n"); |
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 would make the difference, as well as tick openings more obvious. But I guess the compact one-liner will do too.
| appliesToText.append(oneLine ? "{applies_to}`" : "```{applies_to}\n"); | |
| if (oneLine) { | |
| appliesToText.append("{applies_to}"); | |
| appliesToText.append("`"); | |
| } else { | |
| appliesToText.append("```"); | |
| appliesToText.append("{applies_to}"); | |
| } |
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.
If it makes it easier for you to read, I'm happy to do it.
…improv * upstream/main: (92 commits) ESQL: mark LOOKUP JOIN as ExecutesOn.Any by default (elastic#133064) Fix 404s in REST API landing page (elastic#133086) Fix release tests for OptimizerVerificationTests (elastic#133100) Make Glob non-recursive (elastic#132798) Update ES|QL function list for release versions (elastic#133096) Split transport version func test into abstract base (elastic#133035) Omit project ID from snapshot metrics (elastic#133098) Mute org.elasticsearch.xpack.esql.analysis.AnalyzerTests testNoDenseVectorFailsForMagnitude elastic#133013 Mute org.elasticsearch.xpack.esql.optimizer.OptimizerVerificationTests testRemoteEnrichAfterCoordinatorOnlyPlans elastic#133015 Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/160_exists_query/Test exists query on _id field} elastic#133097 Rename initial to unreferenced in transport versions (elastic#133082) Rename exception type header (elastic#133045) ESQL: Pluggable tests for Operator status (elastic#132876) ESQL: Mark new signatures in MIN and MAX (elastic#132980) Don't try to serialize half-baked cluster info (elastic#132756) migrate ml_rollover_legacy_indices transport version (elastic#133008) Enable `exclude_source_vectors` by default for new indices (elastic#131907) Expose APIs needed by flush during translog replay (elastic#132960) Change reporting_user role to leverage reserved kibana privileges (elastic#132766) Update TasksIT for batched execution (elastic#132762) ...
* upstream/main: (58 commits) ESQL: mark LOOKUP JOIN as ExecutesOn.Any by default (elastic#133064) Fix 404s in REST API landing page (elastic#133086) Fix release tests for OptimizerVerificationTests (elastic#133100) Make Glob non-recursive (elastic#132798) Update ES|QL function list for release versions (elastic#133096) Split transport version func test into abstract base (elastic#133035) Omit project ID from snapshot metrics (elastic#133098) Mute org.elasticsearch.xpack.esql.analysis.AnalyzerTests testNoDenseVectorFailsForMagnitude elastic#133013 Mute org.elasticsearch.xpack.esql.optimizer.OptimizerVerificationTests testRemoteEnrichAfterCoordinatorOnlyPlans elastic#133015 Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/160_exists_query/Test exists query on _id field} elastic#133097 Rename initial to unreferenced in transport versions (elastic#133082) Rename exception type header (elastic#133045) ESQL: Pluggable tests for Operator status (elastic#132876) ESQL: Mark new signatures in MIN and MAX (elastic#132980) Don't try to serialize half-baked cluster info (elastic#132756) migrate ml_rollover_legacy_indices transport version (elastic#133008) Enable `exclude_source_vectors` by default for new indices (elastic#131907) Expose APIs needed by flush during translog replay (elastic#132960) Change reporting_user role to leverage reserved kibana privileges (elastic#132766) Update TasksIT for batched execution (elastic#132762) ...

The
MINandMAXfunctions support unsigned longs in 9.2.0. The docs just show it's that unsigned long is supported but you can't tell for what version. This fixes that, applying the version.The version table is generated from the tests. So we have to annotate some test cases with
applies_toand pass it through the test infrastructure.