ESQL: Fetch min transport version we're going to target when resolving fields by nik9000 · Pull Request #135633 · elastic/elasticsearch

nik9000 · 2025-09-29T14:32:09Z

Fetch min transport version we're going to target when resolving fields.

elasticsearchmachine · 2025-09-29T14:32:34Z

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

nik9000 · 2025-09-29T14:41:37Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java

-            ),
-            listener
-        );
+        executeRequest(task, request, listener);


I made @dnhatn add this the last time we changed this. I have copy-and-paste blindness and I said "please don't copy and paste this file, please make hooks, even if it's more work." Because I can never see the changes buried in copy-and-pasted code. And he, being a good sport, did.

Now I'm making a change and copy and pasting. I tried making hooks and though "this is unreadable garbage". It would have been so much worse. Not sure if I was wrong, but I'm pretty sure the hooks are just too hard to read once you make a change like this.

Now! The big copy-and-pasted bit is doExecuteForked - I could make a class DoExecuteForked with places we could override. That feels like it might be worth it. I'm not sure! I didn't do it so folks could look here and have opinions.

…nto esql_fetch_transportversions

nik9000 · 2025-09-29T15:13:52Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/IndexResolver.java

-                )
-            )
+            listener.delegateFailureAndWrap((l, response) -> {
+                LOGGER.debug("minimum transport version {}", response.minTransportVersion());


This is all the PR does! It just plugs it in.

Er, well, it just leaves this here so we can plug it in after.

Is the idea adding the min version to the Configuration of the query? Some other mechanism to propagate it?
I'm thinking about how this will be implemented in the sum agg overflow case (Which happened to work based on the Configuration in the old PR)

Step 1 for me is to use it with fields right here. But step 2 is to drag it out, presumably via IndexResolution and then onwards however we decide. I'm not sure if it should be a thing that is serialized back down to the data nodes. I'd kind of like to keep it on the coordinating node so we're flexible about changing it later.

dnhatn

I left one comment, but this looks good. Thank you for taking care of this, Nik!

dnhatn · 2025-09-29T17:38:20Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlResolveFieldsAction.java

+                    if (response.minTransportVersion() == null) {
+                        minTransportVersion.set(null);
+                    } else {
+                        minTransportVersion.accumulateAndGet(response.minTransportVersion(), TransportVersion::min);


We may hit NullPointerException if we receive a response from an old cluster then another response from a new cluster. Also, can we handle both branches within a single accumulateAndGet call to avoid data races when processing two responses concurrently?

idegtiarenko · 2025-09-30T06:51:04Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlResolveFieldsAction.java

+                listener.onResponse(new FieldCapabilitiesResponse(Collections.emptyList(), Collections.emptyList()));
+            }
+        }
    }


I am afraid of idea of copying that much if TransportFieldCapabilitiesAction especially while it is being actively changed for cps/flat resolution.

Have we considered collecting versions using

for each cluster alias: transportService.getRemoteClusterService().getConnection(clusterAlias).getTransportVersion() and clusterService.state().getMinTransportVersion()

This should be possible after resolving all cluster aliases participating in the query.

This can return an incorrect minTransportVersion if the remote cluster is a mixed-version cluster.

idegtiarenko

I am not comfortable with copying such amount of code that is currently changing.
Lets try exploring alternative approach.

dnhatn · 2025-09-30T14:56:17Z

If you prefer to delay copying this code, we can add minTransportVersion to the existing field-caps response instead of the new resolve response.

nik9000 · 2025-10-01T12:19:09Z

If you prefer to delay copying this code, we can add minTransportVersion to the existing field-caps response instead of the new resolve response.

We talked about that and it didn't feel worth it. Only ESQL wants it and we're worried folks will see it and think "oh, this is how I get that version!" But the request is expensive. I'll see if I can work through the copy and paste today.

ivancea · 2025-10-02T12:17:46Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/IndexResolver.java

-                )
-            )
+            listener.delegateFailureAndWrap((l, response) -> {
+                LOGGER.debug("minimum transport version {}", response.minTransportVersion());


Is the idea adding the min version to the Configuration of the query? Some other mechanism to propagate it?
I'm thinking about how this will be implemented in the sum agg overflow case (Which happened to work based on the Configuration in the old PR)

ivancea · 2025-10-02T12:24:16Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlResolveFieldsResponse.java

+import java.io.IOException;
+
+public class EsqlResolveFieldsResponse extends ActionResponse {
+    private static final TransportVersion CREATED = TransportVersion.fromName("esql_resolve_fields_response_created");


nit: The field name feels quite generic. Even if it's only used locally, I would go with something like RESOLVE_FIELDS_RESPONSE_CREATED or RESOLVE_FIELDS_RESPONSE_CREATED_TV.
Even if the new TVs fields are safe as private fields, they're still quite important to end up mixing the names by some misunderstanding or typo.
Just my 2 cents!

ivancea · 2025-10-02T13:27:56Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlResolveFieldsAction.java

+            concreteIndices = indexNameExpressionResolver.concreteIndexNames(projectState.metadata(), localIndices);
+        }
+
+        if (concreteIndices.length == 0 && remoteClusterIndices.isEmpty()) {


We're not requesting the remote minTV if the remote has no indices? Do we assume that "no indices" means "no remote" or something like that?

We shouldn't be sending to that remote at all in that case. Checking.

This line is for "we didn't match any local indices and didn't detect any remotes to check".

But, in general, yeah, if we reach out to a remote to run the thing we use it's version. Even if no indices match the expression. Just trying to use the remote opts you in to checking its version.

nik9000 · 2025-10-03T12:32:14Z

@idegtiarenko and I've talked over slack and zoom and stuff. He's on holiday today so I'll summarize the conversation here.

He's convinced we need something, that does this, though not convinced this is the right way to do it. I'm more convinced than he is, but that's how conversations go. Either way, we can use this for now, and either keep using it for a long time (my guess) or modify it later (@idegtiarenko's guess). But, for now, this gives us something to fix a lot of the problems we have. So I'll try to get this in today or Monday.

nik9000 · 2025-10-07T15:59:05Z

I have an approval from Nhat and a passing build. I'll get this in. I think Github is confused because it's making me "bypass rules". Or maybe our rule configuration is confused. I dunno, but I think merging is good here.

elasticsearchmachine · 2025-10-07T16:00:39Z

💔 Backport failed

Status	Branch	Result
❌	9.2	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 135633

…g fields (elastic#135633) Relates elastic#131108 Fetch min transport version we're going to target when resolving fields.

…g fields (#135633) (#136132) Relates #131108 Fetch min transport version we're going to target when resolving fields.

idegtiarenko · 2025-10-24T09:59:03Z

Was this backported?

nik9000 added 5 commits September 25, 2025 15:43

vesion

6f1b8bb

Merge branch 'main' into esql_fetch_transportversions

39623f8

Fi

f4863ad

Merge branch 'main' into esql_fetch_transportversions

f32539a

Fetch

416fd21

nik9000 added >non-issue :Analytics/ES|QL AKA ESQL v9.2.0 labels Sep 29, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 29, 2025

Remove more nocommits

572bc91

nik9000 commented Sep 29, 2025

View reviewed changes

elasticsearchmachine and others added 2 commits September 29, 2025 14:51

[CI] Update transport version definitions

f10cc5f

javadoc

eaf99f4

nik9000 requested a review from dnhatn September 29, 2025 15:13

Merge remote-tracking branch 'nik9000/esql_fetch_transportversions' i…

6e96dee

…nto esql_fetch_transportversions

nik9000 commented Sep 29, 2025

View reviewed changes

dnhatn approved these changes Sep 29, 2025

View reviewed changes

idegtiarenko reviewed Sep 30, 2025

View reviewed changes

idegtiarenko requested changes Sep 30, 2025

View reviewed changes

alex-spies mentioned this pull request Sep 30, 2025

ESQL: Update mechanism for under construction data types #135697

Merged

nik9000 added 2 commits September 30, 2025 17:43

Merge branch 'main' into esql_fetch_transportversions

4d8a5bd

More tests

def8f1d

elasticsearchmachine added v9.3.0 and removed v9.2.0 labels Oct 2, 2025

ivancea reviewed Oct 2, 2025

View reviewed changes

nik9000 requested a review from idegtiarenko October 3, 2025 12:29

nik9000 added 4 commits October 3, 2025 08:32

Rename

bab5133

Merge branch 'main' into esql_fetch_transportversions

6a855a1

Merge branch 'main' into esql_fetch_transportversions

760211e

Mark match no indices

8d4b612

nik9000 added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) v9.2.1 labels Oct 3, 2025

elasticsearchmachine and others added 2 commits October 3, 2025 16:08

[CI] Update transport version definitions

4221a51

Merge branch 'main' into esql_fetch_transportversions

13b9bfc

nik9000 added auto-backport Automatically create backport pull requests when merged and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Oct 7, 2025

Merge branch 'main' into esql_fetch_transportversions

9220454

nik9000 merged commit 7720e63 into elastic:main Oct 7, 2025
34 checks passed

elasticsearchmachine added the backport pending label Oct 7, 2025

nik9000 added a commit that referenced this pull request Oct 15, 2025

ESQL: Fetch min transport version we're going to target when resolvin…

4f05a9c

…g fields (#135633) (#136132) Relates #131108 Fetch min transport version we're going to target when resolving fields.

idegtiarenko mentioned this pull request Oct 24, 2025

[9.2] ESQL: Fetch min transport version we're going to target when resolving fields (#135633) #136132

Merged

idegtiarenko removed the backport pending label Oct 24, 2025

Conversation

nik9000 commented Sep 29, 2025 • edited by alex-spies Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Sep 30, 2025

Uh oh!

nik9000 commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Oct 3, 2025

Uh oh!

nik9000 commented Oct 7, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 7, 2025

💔 Backport failed

Uh oh!

idegtiarenko commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nik9000 commented Sep 29, 2025 •

edited by alex-spies

Loading

idegtiarenko left a comment •

edited

Loading