Skip to content

Commit 4a14f83

Browse files
authored
ESQL: Fix enrich and lookup join resolution based on min transport version (#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.
1 parent c95dcea commit 4a14f83

File tree

27 files changed

+825
-232
lines changed

27 files changed

+825
-232
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-
ml_groq_inference_service,9230000
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
@@ -2764,7 +2764,8 @@ protected static MapMatcher getProfileMatcher() {
27642764
.entry("query", instanceOf(Map.class))
27652765
.entry("planning", instanceOf(Map.class))
27662766
.entry("drivers", instanceOf(List.class))
2767-
.entry("plans", instanceOf(List.class));
2767+
.entry("plans", instanceOf(List.class))
2768+
.entry("minimumTransportVersion", instanceOf(Integer.class));
27682769
}
27692770

27702771
protected static MapMatcher getResultMatcher(boolean includePartial, boolean includeDocumentsFound, boolean includeTimestamps) {

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

@@ -759,13 +760,9 @@ public DataType counter() {
759760
public void writeTo(StreamOutput out) throws IOException {
760761
if (supportedVersion.supportedOn(out.getTransportVersion(), Build.current().isSnapshot()) == false) {
761762
/*
762-
* TODO when we implement version aware planning flip this to an IllegalStateException
763-
* so we throw a 500 error. It'll be our bug then. Right now it's a sign that the user
764-
* tried to do something like `KNN(dense_vector_field, [1, 2])` against an old node.
765-
* Like, during the rolling upgrade that enables KNN or to a remote cluster that has
766-
* not yet been upgraded.
763+
* Throw a 500 error - this is a bug, we failed to account for an old node during planning.
767764
*/
768-
throw new IllegalArgumentException(
765+
throw new QlIllegalArgumentException(
769766
"remote node at version [" + out.getTransportVersion() + "] doesn't understand data type [" + this + "]"
770767
);
771768
}

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/PushExpressionToLoadIT.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,7 @@ private void test(
668668
.entry("plans", instanceOf(List.class))
669669
.entry("planning", matchesMap().extraOk())
670670
.entry("query", matchesMap().extraOk())
671+
.entry("minimumTransportVersion", instanceOf(Integer.class))
671672
),
672673
matchesList().item(matchesMap().entry("name", "test").entry("type", any(String.class))),
673674
matchesList().item(expectedValue)

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
@@ -366,6 +366,7 @@ private void testPushQuery(
366366
.entry("plans", instanceOf(List.class))
367367
.entry("planning", matchesMap().extraOk())
368368
.entry("query", matchesMap().extraOk())
369+
.entry("minimumTransportVersion", instanceOf(Integer.class))
369370
),
370371
matchesList().item(matchesMap().entry("name", "test").entry("type", anyOf(equalTo("text"), equalTo("keyword")))),
371372
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)