ESQL: Introduce language versioning to REST API #106824

alex-spies · 2024-03-27T15:30:52Z

First part of #104890

For the _query endpoint, add a parameter for the language version to the JSON payload. This will become a required parameter in a follow-up PR; in this PR, this parameter is only validated with no further action.

not-napoleon

Thanks for tackling this! I think this is a great first draft. I've left a bunch of comments, but I think this is the right direction.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

not-napoleon · 2024-03-27T18:11:15Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

+    NIGHTLY(Integer.MAX_VALUE, Integer.MAX_VALUE, "\uD83D\uDE34"),
+    PARTY_POPPER(2024, 4, "\uD83C\uDF89");
+
+    EsqlVersion(int year, int month, String emoji) {


If we think we might need to serialize this, we should explicitly pass in an ordinal value for serialization (that way it's proof against re-ordering the constants or deleting one). I don't know that we'll have to serialize these, but in case you aren't familiar with that pattern, I wanted to call it out.

Discussed with Mark: will add a disambiguation counter for good measure, so versions will be 2024.04.01.🎉 and if we end up really having to release two language versions in a month, there may be a 2024.04.02.💥.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

not-napoleon · 2024-03-27T18:20:09Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java

+                    "[" + RequestXContent.ESQL_VERSION_FIELD + "] has invalid value [" + esqlVersion + "]",
+                    validationException
+                );
+            } else if (version == EsqlVersion.NIGHTLY && Build.current().isSnapshot() == false) {


Thinking out loud - does the snapshot check belong here, or in the EsqlVersion.parse method? I can see arguments for both.

Hm, personally I do think it belongs here - EsqlVersion shouldn't need to worry about where we permit certain versions to be used IMHO, and why; also, this allows to keep EsqlVersion.parse simple: either the string is a valid version or it's not - otherwise we'd have to throw an exception containing the reason why parsing failed and catch it in EsqlQueryRequest.validate(). We might also parse versions coming from elsewhere in the future, and wouldn't want to prevent parsing of "nightly" just because we're not on a snapshot version.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequest.java

not-napoleon · 2024-03-27T18:29:59Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

@@ -46,10 +46,12 @@ final class RequestXContent {
        PARAM_PARSER.declareString(constructorArg(), TYPE);
    }

-    private static final ParseField QUERY_FIELD = new ParseField("query");
+    // TODO: Maybe esql_version would be better? This looks like there's a JSON attribute `esql` with subattribute `version`.


yeah, we should probably think about that a bit. Worth discussing with Kibana as well. For future proofing, I wonder if we want to set up a whole esql object, something like

"esql": { "version": "2024.04.🎉 ", "whatever_future_setting": "goes_here" }

Discussed with the team: probably just {"query": "...", "version": "2024.04.01.🎉"} is fine.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/version/EsqlVersionTests.java

elasticsearchmachine · 2024-03-28T14:46:33Z

Hi @alex-spies, I've created a changelog YAML for you.

elasticsearchmachine · 2024-03-28T16:20:36Z

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

costin · 2024-03-28T22:06:02Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

+import java.util.LinkedHashMap;
+import java.util.Map;
+
+public enum EsqlVersion implements VersionId<EsqlVersion> {


Better make this a class similar to Version - enum are quite restrictive and impossible to extend.
Having a class gives us future extension points - in the worse case we won't use them and the class will be like an enum.

I think we can flip it to a class with constants when we need that. If we can get away with an enum for a while I'd prefer that.

astefan · 2024-04-02T09:57:20Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

+    }
+
+    private int year;
+    private int month;


month can be made byte.

Makes sense; I'd like to still use an int in the constructor. though, for easier readability if that's good with you?

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java

astefan · 2024-04-02T11:18:49Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

+        for (EsqlVersion version : EsqlVersion.values()) {
+            EsqlVersion existingVersionForKey = null;
+            existingVersionForKey = stringToVersion.put(version.versionStringWithoutEmoji(), version);
+            assert existingVersionForKey == null;
+            existingVersionForKey = stringToVersion.put(version.toString(), version);
+            assert existingVersionForKey == null;
+        }


Suggested change

for (EsqlVersion version : EsqlVersion.values()) {

EsqlVersion existingVersionForKey = null;

existingVersionForKey = stringToVersion.put(version.versionStringWithoutEmoji(), version);

assert existingVersionForKey == null;

existingVersionForKey = stringToVersion.put(version.toString(), version);

assert existingVersionForKey == null;

}

for (EsqlVersion version : EsqlVersion.values()) {

assert null == stringToVersion.put(version.versionStringWithoutEmoji(), version);

assert null == stringToVersion.put(version.toString(), version);

}

I think I actually prefer an explicit throw here rather than just an assert. This should run once per instance of Elasticsearch, I don't think we need the performance gain of being able to disable asserts in production.

I'm not entirely sure how assert works, but I'd be worried that putting these onto one line will make it not perform the put if assertions are disabled. FWIW, it doesn't really matter if it runs it or not - it'll slow down a reader either way. But the assert on it's own line is fin.

You could make it a hard test. The extra test on startup is probably not going to hurt us.

This suggestion looks cleaner, but is not equivalent; disabling assertions breaks the version parsing altogether as no version strings are added to the stringToVersion map anymore.

I'll add an explicit throw new AssertionError(...).

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java

astefan

Also, I think you should have tests for the functionality inherited from VersionId interface, ie comparison of versions based on their id.

not-napoleon · 2024-04-02T14:17:22Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/version/EsqlVersion.java

+
+    @Override
+    public int id() {
+        return this == NIGHTLY ? Integer.MAX_VALUE : (10000 * year + 100 * month + revision);


Maybe we should have a test that generates the IDs for all the versions and makes sure there are no duplicates. There shouldn't be, but if someone typos a month I want something to fail.

No dups are already enforced in the construction of the map String -> Version. I'll add a test, though, that ensures a strict ordering of the versions based on their id.

astefan

LGTM

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java

Might cause trouble for people on a terminal.

alex-spies · 2024-04-10T10:54:30Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

Questions ?

Please refer to the Backport tool documentation

For the _query endpoint, add a parameter for the ESQL language version to the JSON payload. For now, it is optional and is only validated with no further action. (cherry picked from commit 147f5a0) # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequest.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java

For the _query endpoint, add a parameter for the ESQL language version to the JSON payload. For now, it is optional and is only validated with no further action.

For the _query endpoint, add a parameter for the ESQL language version to the JSON payload. For now, it is optional and is only validated with no further action. (cherry picked from commit 147f5a0) # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequest.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java

alex-spies · 2024-04-12T14:13:32Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12
✅	8.11

Questions ?

Please refer to the Backport tool documentation

For the _query endpoint, add a parameter for the ESQL language version to the JSON payload. For now, it is optional and is only validated with no further action. (cherry picked from commit 147f5a0) # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequest.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestBuilder.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/RequestXContent.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java

Docs for #106824. Does not cover the [REST API specs](https://github.com/elastic/elasticsearch/blob/main/rest-api-spec/src/main/resources/rest-api-spec/api/esql.query.json) as these don't cover the request body.

alex-spies added 3 commits March 27, 2024 12:02

Add version enum

1f45d65

Parse + validate esql.version

c4d5a8e

Add tests

26b9fe6

elasticsearchmachine added the v8.14.0 label Mar 27, 2024

alex-spies added 3 commits March 27, 2024 16:37

Refactor EsqlVersion.toString

887759a

Make checkstyle happy

da6c0e1

Avoid forbidden APIs

8e6de63

not-napoleon reviewed Mar 27, 2024

View reviewed changes

alex-spies added 4 commits March 28, 2024 14:00

Use proper emojis

98b816f

Add disambiguation counter to version

d5bef79

Make parsing stricter

f0a2aae

Rename esql.version -> version

9e97e43

alex-spies added >enhancement :Analytics/ES|QL AKA ESQL labels Mar 28, 2024

alex-spies added 3 commits March 28, 2024 15:46

Update docs/changelog/106824.yaml

11ba353

Remove obsolete TODO

a29fc71

Spotless

85576a7

alex-spies marked this pull request as ready for review March 28, 2024 16:20

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

alex-spies mentioned this pull request Mar 28, 2024

ESQL: use version in ITs #106879

Closed

costin reviewed Mar 28, 2024

View reviewed changes

astefan reviewed Apr 2, 2024

View reviewed changes

astefan self-requested a review April 2, 2024 11:34

astefan reviewed Apr 2, 2024

View reviewed changes

alex-spies added 3 commits April 2, 2024 15:06

Refactor month and numberThisMonth

5920994

Add validation tests for non-snapshot builds

a633632

Merge remote-tracking branch 'upstream/main' into esql-version

c8c9684

not-napoleon reviewed Apr 2, 2024

View reviewed changes

alex-spies added 3 commits April 2, 2024 16:37

Merge remote-tracking branch 'upstream/main' into esql-version

de6304e

Use randomization to test both sync/async

be3f255

Add empty version string to test

5b15c30

astefan approved these changes Apr 3, 2024

View reviewed changes

alex-spies added 6 commits April 3, 2024 11:43

Address feedback

c6f06db

Improve assertions

eb795fb

Rename nightly -> snapshot

8a9c0f8

Add latest version to error message when missing

0b5fb1f

Add latest version to error message if invalid

962f16b

Add latest version in error if using snapshot

3b7c031

nik9000 reviewed Apr 3, 2024

View reviewed changes

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java Outdated Show resolved Hide resolved

alex-spies added 5 commits April 3, 2024 15:03

Remove emoji from error message

20118c6

Might cause trouble for people on a terminal.

Throw IAE instead of AssertionError

037f1f7

Use rocket emoji for first released version

ab3be62

Merge remote-tracking branch 'upstream/main' into esql-version

b181c7c

Fix test

acb2703

alex-spies mentioned this pull request Apr 3, 2024

ESQL: Introduce language versioning #104890

Closed

6 tasks

alex-spies merged commit 147f5a0 into elastic:main Apr 3, 2024
14 checks passed

alex-spies deleted the esql-version branch April 3, 2024 19:23

This was referenced Apr 8, 2024

ESQL: Add version to docs #107225

Merged

[8.13] ESQL: Introduce language versioning to REST API (#106824) #107301

Merged

alex-spies mentioned this pull request Apr 12, 2024

[8.12] ESQL: Introduce language versioning to REST API (#106824) #107422

Closed

alex-spies mentioned this pull request Apr 12, 2024

[8.11] ESQL: Introduce language versioning to REST API (#106824) #107424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Introduce language versioning to REST API #106824

ESQL: Introduce language versioning to REST API #106824

alex-spies commented Mar 27, 2024 •

edited

Loading

not-napoleon left a comment

not-napoleon Mar 27, 2024

alex-spies Mar 28, 2024

not-napoleon Mar 27, 2024

alex-spies Mar 28, 2024

not-napoleon Mar 27, 2024

alex-spies Mar 28, 2024

elasticsearchmachine commented Mar 28, 2024

elasticsearchmachine commented Mar 28, 2024

costin Mar 28, 2024

nik9000 Apr 2, 2024

astefan Apr 2, 2024

alex-spies Apr 2, 2024

astefan Apr 2, 2024

not-napoleon Apr 2, 2024

nik9000 Apr 2, 2024

alex-spies Apr 2, 2024

alex-spies Apr 3, 2024

astefan left a comment

not-napoleon Apr 2, 2024

alex-spies Apr 3, 2024

astefan left a comment

alex-spies commented Apr 10, 2024

alex-spies commented Apr 12, 2024

ESQL: Introduce language versioning to REST API #106824

ESQL: Introduce language versioning to REST API #106824

Conversation

alex-spies commented Mar 27, 2024 • edited Loading

not-napoleon left a 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

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 28, 2024

elasticsearchmachine commented Mar 28, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

alex-spies commented Apr 10, 2024

💚 All backports created successfully

Questions ?

alex-spies commented Apr 12, 2024

💚 All backports created successfully

Questions ?

alex-spies commented Mar 27, 2024 •

edited

Loading