Skip to content

Commit a4e72ad

Browse files
committed
ESQL: Fix enrich and lookup join resolution based on min transport version (elastic#137431)
When deciding which types are supported, we did not use the correct minimum transport version during the enrich resolution in case of CCS and ROW queries. What's more, the EnrichPolicyResolver did not account for the fact that the node requesting resolution might be on a version that doesn't support the types in the resolved mapping, which led to serialization bugs surfacing when trying to enable the DATE_RANGE type. - Initialize the minimum transport version with the minimum version from the cluster state before any resolution steps. That makes ROW queries correct. - Send the determined minimum transport version along the enrich resolution request so that remote clusters don't send un-deserializable data types back. - Add the determined minimum transport version to the profile. - Add a bunch of tests. (cherry picked from commit 4a14f83) # Conflicts: # server/src/main/resources/transport/upper_bounds/9.3.csv # x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/PushExpressionToLoadIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/TransportEsqlQueryAction.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/IndexResolver.java
1 parent 8fa2c74 commit a4e72ad

File tree

26 files changed

+785
-213
lines changed

26 files changed

+785
-213
lines changed

docs/changelog/137431.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 137431
2+
summary: Fix enrich and lookup join resolution based on min transport version
3+
area: ES|QL
4+
type: bug
5+
issues: []
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
9231000,9185011
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
aggregate_metric_double_typed_block,9185010
1+
esql_use_minimum_version_for_enrich_resolution,9185011
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
aggregate_metric_double_typed_block,9227000
1+
esql_use_minimum_version_for_enrich_resolution,9231000

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2733,7 +2733,8 @@ protected static MapMatcher getProfileMatcher() {
27332733
.entry("query", instanceOf(Map.class))
27342734
.entry("planning", instanceOf(Map.class))
27352735
.entry("drivers", instanceOf(List.class))
2736-
.entry("plans", instanceOf(List.class));
2736+
.entry("plans", instanceOf(List.class))
2737+
.entry("minimumTransportVersion", instanceOf(Integer.class));
27372738
}
27382739

27392740
protected static MapMatcher getResultMatcher(boolean includePartial, boolean includeDocumentsFound) {

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.elasticsearch.index.IndexMode;
1616
import org.elasticsearch.index.mapper.SourceFieldMapper;
1717
import org.elasticsearch.index.mapper.TimeSeriesIdFieldMapper;
18+
import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
1819
import org.elasticsearch.xpack.esql.core.util.PlanStreamInput;
1920
import org.elasticsearch.xpack.esql.core.util.PlanStreamOutput;
2021

@@ -751,13 +752,9 @@ public DataType counter() {
751752
public void writeTo(StreamOutput out) throws IOException {
752753
if (supportedVersion.supportedOn(out.getTransportVersion(), Build.current().isSnapshot()) == false) {
753754
/*
754-
* TODO when we implement version aware planning flip this to an IllegalStateException
755-
* so we throw a 500 error. It'll be our bug then. Right now it's a sign that the user
756-
* tried to do something like `KNN(dense_vector_field, [1, 2])` against an old node.
757-
* Like, during the rolling upgrade that enables KNN or to a remote cluster that has
758-
* not yet been upgraded.
755+
* Throw a 500 error - this is a bug, we failed to account for an old node during planning.
759756
*/
760-
throw new IllegalArgumentException(
757+
throw new QlIllegalArgumentException(
761758
"remote node at version [" + out.getTransportVersion() + "] doesn't understand data type [" + this + "]"
762759
);
763760
}

x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/AllSupportedFieldsIT.java

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,18 @@ public AllSupportedFieldsIT(MappedFieldType.FieldExtractPreference extractPrefer
5050
public void createRemoteIndices() throws IOException {
5151
if (supportsNodeAssignment()) {
5252
for (Map.Entry<String, NodeInfo> e : remoteNodeToInfo().entrySet()) {
53-
createIndexForNode(remoteClient(), e.getKey(), e.getValue().id());
53+
createIndexForNode(remoteClient(), e.getKey(), e.getValue().id(), indexMode());
5454
}
5555
} else {
56-
createIndexForNode(remoteClient(), null, null);
56+
createIndexForNode(remoteClient(), null, null, indexMode());
57+
}
58+
59+
// We need a single lookup index that has the same name across all clusters, as well as a single enrich policy per cluster.
60+
// We create both only when we're testing LOOKUP mode.
61+
if (indexExists(remoteClient(), LOOKUP_INDEX_NAME) == false && indexMode() == IndexMode.LOOKUP) {
62+
createAllTypesIndex(remoteClient(), LOOKUP_INDEX_NAME, null, indexMode());
63+
createAllTypesDoc(remoteClient(), LOOKUP_INDEX_NAME);
64+
createEnrichPolicy(remoteClient(), LOOKUP_INDEX_NAME, ENRICH_POLICY_NAME);
5765
}
5866
}
5967

@@ -101,4 +109,16 @@ && clusterHasCapability(remoteClient(), "GET", "/_query", List.of(), List.of("DE
101109
false
102110
);
103111
}
112+
113+
@Override
114+
protected boolean fetchAllIsCrossCluster() {
115+
return true;
116+
}
117+
118+
public final void testFetchAllOnlyFromRemotes() throws IOException {
119+
doTestFetchAll(fromAllQuery("*:%mode%*", """
120+
, _id, _ignored, _index_mode, _score, _source, _version
121+
| LIMIT 1000
122+
"""), remoteNodeToInfo(), allNodeToInfo());
123+
}
104124
}

x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/PushQueriesIT.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ private void testPushQuery(
365365
.entry("plans", instanceOf(List.class))
366366
.entry("planning", matchesMap().extraOk())
367367
.entry("query", matchesMap().extraOk())
368+
.entry("minimumTransportVersion", instanceOf(Integer.class))
368369
),
369370
matchesList().item(matchesMap().entry("name", "test").entry("type", anyOf(equalTo("text"), equalTo("keyword")))),
370371
equalTo(found ? List.of(List.of(value)) : List.of())

x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/StoredFieldsSequentialIT.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ private void testQuery(Double percent, String query, int documentsFound, boolean
122122
.entry("plans", instanceOf(List.class))
123123
.entry("planning", matchesMap().extraOk())
124124
.entry("query", matchesMap().extraOk())
125+
.entry("minimumTransportVersion", instanceOf(Integer.class))
125126
)
126127
.extraOk()
127128
);

0 commit comments

Comments
 (0)