Skip to content

Commit d0d4339

Browse files
alex-spiesalex-spies
authored
ESQL: Update mechanism for under construction data types (elastic#135697)
- Fix the test expectations for our AllSupportedFieldsITs according to elastic#135215 (comment). - Clean up our mechanism and docs for "under construction" data types: - For SNAPSHOT builds, go back to the old status quo where all types are considered supported and we just skip tests based on capabilities. - Re-brand the CreatedVersion for data types to SupportedVersion. For release builds, it doesn't matter when serialization code for a type was introduced; it matters only when it was actually released, and this is generally later than when the serialization code for the type was written. - Remove the old feature-flag based system for marking types as "under construction" as we don't plan to use feature flags, but minimum transport versions to determine if a remote node/cluster supports a type.
1 parent a99f604 commit d0d4339

File tree

12 files changed

+248
-181
lines changed

12 files changed

+248
-181
lines changed

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

Lines changed: 0 additions & 43 deletions
This file was deleted.

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

Lines changed: 113 additions & 95 deletions
Large diffs are not rendered by default.

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

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.core.type;
9+
10+
import org.elasticsearch.Build;
11+
import org.elasticsearch.TransportVersion;
12+
13+
public interface SupportedVersion {
14+
boolean supports(TransportVersion version);
15+
16+
default boolean supportedLocally() {
17+
return supports(TransportVersion.current());
18+
}
19+
20+
SupportedVersion SUPPORTED_ON_ALL_NODES = new SupportedVersion() {
21+
@Override
22+
public boolean supports(TransportVersion version) {
23+
return true;
24+
}
25+
26+
@Override
27+
public String toString() {
28+
return "SupportedOnAllVersions";
29+
}
30+
};
31+
32+
/**
33+
* Types that are actively being built. These types are
34+
* <ul>
35+
* <li>Not returned from Elasticsearch on release builds.</li>
36+
* <li>Not included in generated documentation</li>
37+
* <li>
38+
* Not tested by {@code ErrorsForCasesWithoutExamplesTestCase} subclasses.
39+
* When a function supports a type it includes a test case in its subclass
40+
* of {@code AbstractFunctionTestCase}. If a function does not support.
41+
* them like {@code TO_STRING} then the tests won't notice. See class javadoc
42+
* for instructions on adding new types, but that usually involves adding support
43+
* for that type to a handful of functions. Once you've done that you should be
44+
* able to remove your new type from UNDER_CONSTRUCTION and update a few error
45+
* messages.
46+
* </li>
47+
* </ul>
48+
* <p>
49+
* Snapshot builds treat these as always supported so that we can write tests before actually
50+
* turning on the support for the type. Mixed/multi cluster tests with older nodes have to be
51+
* skipped based on capabilites, as always.
52+
*/
53+
// We used to have a feature-flag based override, so that in-development types could be
54+
// turned on for testing in release builds. If needed, it's fine to bring this back, but we
55+
// need to make sure that other checks for types being under construction are also overridden.
56+
// Check usage of this constant to be sure.
57+
SupportedVersion UNDER_CONSTRUCTION = new SupportedVersion() {
58+
@Override
59+
public boolean supports(TransportVersion version) {
60+
return Build.current().isSnapshot();
61+
}
62+
63+
@Override
64+
public String toString() {
65+
return "UnderConstruction";
66+
}
67+
};
68+
69+
/**
70+
* Types that are supported starting with the given version.
71+
* <p>
72+
* Snapshot builds treat these as always supported, so that any existing tests using them
73+
* continue to work. Otherwise, we'd have to update bwc tests to skip older versions based
74+
* on a capability check, which can be error-prone and risks turning off an unrelated bwc test.
75+
*/
76+
static SupportedVersion supportedOn(TransportVersion supportedVersion) {
77+
return new SupportedVersion() {
78+
@Override
79+
public boolean supports(TransportVersion version) {
80+
return version.supports(supportedVersion) || Build.current().isSnapshot();
81+
}
82+
83+
@Override
84+
public String toString() {
85+
return "SupportedOn[" + supportedVersion + "]";
86+
}
87+
};
88+
}
89+
}

‎x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/AllSupportedFieldsTestCase.java‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
1111

1212
import org.apache.http.util.EntityUtils;
13+
import org.elasticsearch.Build;
1314
import org.elasticsearch.TransportVersion;
1415
import org.elasticsearch.client.Request;
1516
import org.elasticsearch.client.ResponseException;
@@ -46,22 +47,25 @@
4647
import static org.hamcrest.Matchers.any;
4748
import static org.hamcrest.Matchers.anyOf;
4849
import static org.hamcrest.Matchers.containsString;
49-
import static org.hamcrest.Matchers.either;
5050
import static org.hamcrest.Matchers.equalTo;
5151
import static org.hamcrest.Matchers.nullValue;
5252

5353
/**
54-
* Creates indices with all supported fields and fetches values from them.
54+
* Creates indices with all supported fields and fetches values from them to
55+
* confirm that release builds correctly handle data types, even if they were
56+
* introduced in later versions.
57+
* <p>
58+
* Entirely skipped in snapshot builds; data types that are under
59+
* construction are normally tested well enough in spec tests, skipping
60+
* old versions via {@link org.elasticsearch.xpack.esql.action.EsqlCapabilities}.
5561
* <p>
5662
* In a single cluster where all nodes are on a single version this is
5763
* just an "is it plugged in" style smoke test. In a mixed version cluster
5864
* this is testing the behavior of fetching potentially unsupported field
59-
* types. The same is true multi-cluster cases.
60-
* </p>
65+
* types. The same is true for multi-cluster cases.
6166
* <p>
6267
* This isn't trying to test complex interactions with field loading so we
6368
* load constant field values and have simple mappings.
64-
* </p>
6569
*/
6670
public class AllSupportedFieldsTestCase extends ESRestTestCase {
6771
private static final Logger logger = LogManager.getLogger(FieldExtractorTestCase.class);
@@ -71,6 +75,11 @@ public class AllSupportedFieldsTestCase extends ESRestTestCase {
7175

7276
@ParametersFactory(argumentFormatting = "pref=%s mode=%s")
7377
public static List<Object[]> args() {
78+
if (Build.current().isSnapshot()) {
79+
// We only test behavior in release builds. Snapshot builds will have data types enabled that are still under construction.
80+
return List.of();
81+
}
82+
7483
List<Object[]> args = new ArrayList<>();
7584
for (MappedFieldType.FieldExtractPreference extractPreference : Arrays.asList(
7685
null,
@@ -168,6 +177,7 @@ public void createIndices() throws IOException {
168177
}
169178
}
170179

180+
// TODO: Also add a test for _tsid once we can determine the minimum transport version of all nodes.
171181
public final void testFetchAll() throws IOException {
172182
Map<String, Object> response = esql("""
173183
, _id, _ignored, _index_mode, _score, _source, _version
@@ -204,7 +214,7 @@ public final void testFetchAll() throws IOException {
204214
if (supportedInIndex(type) == false) {
205215
continue;
206216
}
207-
expectedValues = expectedValues.entry(fieldName(type), expectedValue(nodeInfo.version, type));
217+
expectedValues = expectedValues.entry(fieldName(type), expectedValue(type));
208218
}
209219
expectedValues = expectedValues.entry("_id", any(String.class))
210220
.entry("_ignored", nullValue())
@@ -219,6 +229,7 @@ public final void testFetchAll() throws IOException {
219229
profileLogger.clearProfile();
220230
}
221231

232+
// Tests a workaround and will become obsolete once we can determine the actual minimum transport version of all nodes.
222233
public final void testFetchDenseVector() throws IOException {
223234
Map<String, Object> response;
224235
try {
@@ -400,7 +411,8 @@ private void createAllTypesDoc(RestClient client, String indexName) throws IOExc
400411
client.performRequest(request);
401412
}
402413

403-
private Matcher<?> expectedValue(TransportVersion version, DataType type) throws IOException {
414+
// This will become dependent on the minimum transport version of all nodes once we can determine that.
415+
private Matcher<?> expectedValue(DataType type) {
404416
return switch (type) {
405417
case BOOLEAN -> equalTo(true);
406418
case COUNTER_LONG, LONG, COUNTER_INTEGER, INTEGER, UNSIGNED_LONG, SHORT, BYTE -> equalTo(1);
@@ -419,19 +431,14 @@ private Matcher<?> expectedValue(TransportVersion version, DataType type) throws
419431
case GEO_SHAPE -> equalTo("POINT (-71.34 41.12)");
420432
case NULL -> nullValue();
421433
case AGGREGATE_METRIC_DOUBLE -> {
422-
if (denseVectorAggMetricDoubleIfFns()) {
423-
// If all versions are new we get null. If any are old, some *might* support aggregate_metric_double
424-
yield nullValue();
425-
}
426-
Matcher<String> expected = equalTo("{\"min\":-302.5,\"max\":702.3,\"sum\":200.0,\"value_count\":25}");
427-
yield anyOf(nullValue(), expected);
434+
// Currently, we cannot tell if all nodes support it or not so we treat it as unsupported.
435+
// TODO: Fix this once we know the node versions.
436+
yield nullValue();
428437
}
429438
case DENSE_VECTOR -> {
430-
if (denseVectorAggMetricDoubleIfFns()) {
431-
// If all versions are new we get null. If any are old, some *might* support dense_vector
432-
yield nullValue();
433-
}
434-
yield anyOf(nullValue(), expectedDenseVector(version));
439+
// Currently, we cannot tell if all nodes support it or not so we treat it as unsupported.
440+
// TODO: Fix this once we know the node versions.
441+
yield nullValue();
435442
}
436443
default -> throw new AssertionError("unsupported field type [" + type + "]");
437444
};
@@ -449,6 +456,7 @@ private Matcher<List<?>> expectedDenseVector(TransportVersion version) {
449456
private static boolean supportedInIndex(DataType t) {
450457
return switch (t) {
451458
// These are supported but implied by the index process.
459+
// TODO: current versions already support _tsid; update this once we can tell whether all nodes support it.
452460
case OBJECT, SOURCE, DOC_DATA_TYPE, TSID_DATA_TYPE,
453461
// Internal only
454462
UNSUPPORTED, PARTIAL_AGG,
@@ -500,7 +508,8 @@ private Map<String, Object> nameToValue(List<String> names, List<?> values) {
500508
return result;
501509
}
502510

503-
private Matcher<String> expectedType(DataType type) throws IOException {
511+
// This will become dependent on the minimum transport version of all nodes once we can determine that.
512+
private Matcher<String> expectedType(DataType type) {
504513
return switch (type) {
505514
case COUNTER_DOUBLE, COUNTER_LONG, COUNTER_INTEGER -> {
506515
if (indexMode == IndexMode.TIME_SERIES) {
@@ -512,13 +521,9 @@ private Matcher<String> expectedType(DataType type) throws IOException {
512521
case HALF_FLOAT, SCALED_FLOAT, FLOAT -> equalTo("double");
513522
case NULL -> equalTo("keyword");
514523
// Currently unsupported without TS command or KNN function
515-
case AGGREGATE_METRIC_DOUBLE, DENSE_VECTOR -> {
516-
if (denseVectorAggMetricDoubleIfFns()) {
517-
// If all versions are new we get null. If any are old, some *might* support dense_vector
518-
yield equalTo("unsupported");
519-
}
520-
yield either(equalTo("unsupported")).or(equalTo(type.esType()));
521-
}
524+
case AGGREGATE_METRIC_DOUBLE, DENSE_VECTOR ->
525+
// TODO: Fix this once we know the node versions.
526+
equalTo("unsupported");
522527
default -> equalTo(type.esType());
523528
};
524529
}

‎x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1024,7 +1024,7 @@ public void doTest() {
10241024
}
10251025

10261026
private boolean isValidDataType(DataType dataType) {
1027-
return UNDER_CONSTRUCTION.get(dataType) == null || UNDER_CONSTRUCTION.get(dataType).isEnabled();
1027+
return UNDER_CONSTRUCTION.contains(dataType) == false;
10281028
}
10291029

10301030
private static void saveJoinTypes(Supplier<Set<DocsV3Support.TypeSignature>> signatures) throws Exception {

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import org.elasticsearch.Build;
1111
import org.elasticsearch.common.Strings;
1212
import org.elasticsearch.common.util.CollectionUtils;
13-
import org.elasticsearch.common.util.FeatureFlag;
1413
import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
1514
import org.elasticsearch.xpack.esql.core.expression.Expression;
1615
import org.elasticsearch.xpack.esql.core.expression.MapExpression;
@@ -795,9 +794,9 @@ public static ArgSignature paramWithoutAnnotation(String name) {
795794
* Remove types that are being actively built.
796795
*/
797796
private static String[] removeUnderConstruction(String[] types) {
798-
for (Map.Entry<DataType, FeatureFlag> underConstruction : DataType.UNDER_CONSTRUCTION.entrySet()) {
799-
if (underConstruction.getValue().isEnabled() == false) {
800-
types = Arrays.stream(types).filter(t -> underConstruction.getKey().typeName().equals(t) == false).toArray(String[]::new);
797+
for (DataType underConstruction : DataType.UNDER_CONSTRUCTION) {
798+
if (underConstruction.supportedVersion().supportedLocally() == false) {
799+
types = Arrays.stream(types).filter(t -> underConstruction.typeName().equals(t) == false).toArray(String[]::new);
801800
}
802801
}
803802
return types;

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ public static Stream<DataType> validFunctionParameters() {
418418
// We don't test that functions don't take date_period or time_duration. We should.
419419
return false;
420420
}
421-
if (DataType.UNDER_CONSTRUCTION.containsKey(t)) {
421+
if (DataType.UNDER_CONSTRUCTION.contains(t)) {
422422
/*
423423
* Types under construction aren't checked because we're actively
424424
* adding support for them to functions. That’s *why* they are
@@ -763,19 +763,19 @@ public static void testFunctionInfo() {
763763
for (int i = 0; i < args.size() && i < types.size(); i++) {
764764
typesFromSignature.get(i).add(types.get(i).dataType().esNameIfPossible());
765765
}
766-
if (DataType.UNDER_CONSTRUCTION.containsKey(entry.returnType()) == false) {
766+
if (DataType.UNDER_CONSTRUCTION.contains(entry.returnType()) == false) {
767767
returnFromSignature.add(entry.returnType().esNameIfPossible());
768768
}
769769
}
770770

771771
for (int i = 0; i < args.size(); i++) {
772772
EsqlFunctionRegistry.ArgSignature arg = args.get(i);
773773
Set<String> annotationTypes = Arrays.stream(arg.type())
774-
.filter(t -> DataType.UNDER_CONSTRUCTION.containsKey(DataType.fromNameOrAlias(t)) == false)
774+
.filter(t -> DataType.UNDER_CONSTRUCTION.contains(DataType.fromNameOrAlias(t)) == false)
775775
.collect(Collectors.toCollection(TreeSet::new));
776776
Set<String> signatureTypes = typesFromSignature.get(i)
777777
.stream()
778-
.filter(t -> DataType.UNDER_CONSTRUCTION.containsKey(DataType.fromNameOrAlias(t)) == false)
778+
.filter(t -> DataType.UNDER_CONSTRUCTION.contains(DataType.fromNameOrAlias(t)) == false)
779779
.collect(Collectors.toCollection(TreeSet::new));
780780
if (signatureTypes.isEmpty()) {
781781
log.info("{}: skipping", arg.name());
@@ -796,7 +796,7 @@ public static void testFunctionInfo() {
796796
}
797797

798798
Set<String> returnTypes = Arrays.stream(description.returnType())
799-
.filter(t -> DataType.UNDER_CONSTRUCTION.containsKey(DataType.fromNameOrAlias(t)) == false)
799+
.filter(t -> DataType.UNDER_CONSTRUCTION.contains(DataType.fromNameOrAlias(t)) == false)
800800
.collect(Collectors.toCollection(TreeSet::new));
801801
assertEquals(returnFromSignature, returnTypes);
802802
}
@@ -1060,7 +1060,7 @@ static boolean shouldHideSignature(List<DocsV3Support.Param> argTypes, DataType
10601060
return true;
10611061
}
10621062

1063-
for (DataType dt : DataType.UNDER_CONSTRUCTION.keySet()) {
1063+
for (DataType dt : DataType.UNDER_CONSTRUCTION) {
10641064
if (returnType == dt || argTypes.stream().anyMatch(p -> p.dataType() == dt)) {
10651065
return true;
10661066
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/fulltext/AbstractMatchFullTextFunctionTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ private static void addQueryAsStringTestCases(List<TestCaseSupplier> suppliers)
338338

339339
private static void addStringTestCases(List<TestCaseSupplier> suppliers) {
340340
for (DataType fieldType : DataType.stringTypes()) {
341-
if (DataType.UNDER_CONSTRUCTION.containsKey(fieldType)) {
341+
if (DataType.UNDER_CONSTRUCTION.contains(fieldType)) {
342342
continue;
343343
}
344344
for (TestCaseSupplier.TypedDataSupplier queryDataSupplier : stringCases(fieldType)) {

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MatchPhraseTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ private static List<TestCaseSupplier> testCaseSuppliers() {
5555

5656
public static void addStringTestCases(List<TestCaseSupplier> suppliers) {
5757
for (DataType fieldType : DataType.stringTypes()) {
58-
if (DataType.UNDER_CONSTRUCTION.containsKey(fieldType)) {
58+
if (DataType.UNDER_CONSTRUCTION.contains(fieldType)) {
5959
continue;
6060
}
6161
for (TestCaseSupplier.TypedDataSupplier queryDataSupplier : stringCases(fieldType)) {

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ public class CaseTests extends AbstractScalarFunctionTestCase {
6464
).collect(Collectors.toList());
6565
if (Build.current().isSnapshot()) {
6666
t.addAll(
67-
DataType.UNDER_CONSTRUCTION.keySet()
68-
.stream()
67+
DataType.UNDER_CONSTRUCTION.stream()
6968
.filter(type -> type != DataType.AGGREGATE_METRIC_DOUBLE && type != DataType.DENSE_VECTOR)
7069
.toList()
7170
);

0 commit comments

Comments
 (0)