ESQL: Add createdVersion for types (#137432) (#139098)

---------

Co-authored-by: Alexander Spies <[email protected]>
Co-authored-by: elasticsearchmachine <[email protected]>

Author: 3 people

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

@@ -47,20 +47,25 @@
  * {@link DataType#OBJECT})
  *
  * <h2>Process for adding a new data type</h2>
+ * We assume that the data type is already supported in ES indices, but not in
+ * ES|QL. Types that aren't yet enabled in ES will require some adjustments to
+ * the process.
+ * <p>
  * Note: it is not expected that all the following steps be done in a single PR.
  * Use capabilities to gate tests as you go, and use as many PRs as you think
  * appropriate. New data types are complex, and smaller PRs will make reviews
  * easier.
  * <ul>
  *     <li>
  *         Create a new data type and mark it as under construction using
- *         {@link Builder#underConstruction()}. This makes the type available on
+ *         {@link Builder#underConstruction(TransportVersion)}. This makes the type available on
  *         SNAPSHOT builds, only, prevents some tests from running and prevents documentation
  *         for the new type to be built.</li>
  *     <li>
  *         New tests using the type will require a new {@code EsqlCapabilities} entry,
  *         otherwise bwc tests will fail (even in SNAPSHOT builds) because old nodes don't
- *         know about the new type.</li>
+ *         know about the new type. This capability needs to be SNAPSHOT-only as long as
+ *         the type is under construction</li>
  *     <li>
  *         Create a new CSV test file for the new type. You'll either need to
  *         create a new data file as well, or add values of the new type to
@@ -115,20 +120,30 @@
  *         EsqlDataTypeConverter#commonType, individual function type checking, the
  *         verifier rules, or other places. We suggest starting with CSV tests and
  *         seeing where they fail.</li>
+ *     <li>
+ *         Ensure the new type doesn't break {@code FROM idx | KEEP *} queries by
+ *         updating AllSupportedFieldsTestCase. Make sure to run this test in bwc
+ *         configurations in release mode.
+ *         </li>
  * </ul>
- * There are some additional steps that should be taken when removing the
- * feature flag and getting ready for a release:
+ * There are some additional steps that should be taken when getting ready for a release:
  * <ul>
  *     <li>
  *         Ensure the capabilities for this type are always enabled.</li>
  *     <li>
  *         Mark the type with a new transport version via
- *         {@link Builder#supportedSince(TransportVersion)}. This will enable the type on
- *         non-SNAPSHOT builds as long as all nodes in the cluster (and remote clusters)
- *         support it.</li>
+ *         {@link Builder#supportedSince(TransportVersion, TransportVersion)}.
+ *         This will enable the type on non-SNAPSHOT builds as long as all nodes in the cluster
+ *         (and remote clusters) support it.
+ *         Use the under-construction transport version for the {@code createdVersion} here so that
+ *         existing tests continue to run.
+ *         </li>
  *     <li>
  *         Fix new test failures related to declared function types.</li>
  *     <li>
+ *         Update the expectations in AllSupportedFieldsTestCase and make sure it
+ *         passes in release builds.</li>
+ *     <li>
  *         Make sure to run the full test suite locally via gradle to generate
  *         the function type tables and helper files with the new type. Ensure all
  *         the functions that support the type have appropriate docs for it.</li>
@@ -299,12 +314,23 @@ public enum DataType implements Writeable {
     // mixed/multi clusters with remotes that don't support these types. This is low-ish risk because these types require specific
     // geo functions to turn up in the query, and those types aren't available before 9.2.0 either.
     GEOHASH(
-        builder().esType("geohash").typeName("GEOHASH").estimatedSize(Long.BYTES).supportedSince(DataTypesTransportVersions.INDEX_SOURCE)
+        builder().esType("geohash")
+            .typeName("GEOHASH")
+            .estimatedSize(Long.BYTES)
+            .supportedSince(DataTypesTransportVersions.INDEX_SOURCE, DataTypesTransportVersions.INDEX_SOURCE)
     ),
     GEOTILE(
-        builder().esType("geotile").typeName("GEOTILE").estimatedSize(Long.BYTES).supportedSince(DataTypesTransportVersions.INDEX_SOURCE)
+        builder().esType("geotile")
+            .typeName("GEOTILE")
+            .estimatedSize(Long.BYTES)
+            .supportedSince(DataTypesTransportVersions.INDEX_SOURCE, DataTypesTransportVersions.INDEX_SOURCE)
+    ),
+    GEOHEX(
+        builder().esType("geohex")
+            .typeName("GEOHEX")
+            .estimatedSize(Long.BYTES)
+            .supportedSince(DataTypesTransportVersions.INDEX_SOURCE, DataTypesTransportVersions.INDEX_SOURCE)
     ),
-    GEOHEX(builder().esType("geohex").typeName("GEOHEX").estimatedSize(Long.BYTES).supportedSince(DataTypesTransportVersions.INDEX_SOURCE)),
 
     /**
      * Fields with this type represent a Lucene doc id. This field is a bit magic in that:
@@ -328,7 +354,10 @@ public enum DataType implements Writeable {
     // mixed/multi clusters with remotes that don't support these types. This is low-ish risk because _tsid requires specifically being
     // used in `FROM idx METADATA _tsid` or in the `TS` command, which both weren't available before 9.2.0.
     TSID_DATA_TYPE(
-        builder().esType("_tsid").estimatedSize(Long.BYTES * 2).docValues().supportedSince(DataTypesTransportVersions.INDEX_SOURCE)
+        builder().esType("_tsid")
+            .estimatedSize(Long.BYTES * 2)
+            .docValues()
+            .supportedSince(DataTypesTransportVersions.INDEX_SOURCE, DataTypesTransportVersions.INDEX_SOURCE)
     ),
     /**
      * Fields with this type are the partial result of running a non-time-series aggregation
@@ -339,17 +368,25 @@ public enum DataType implements Writeable {
     AGGREGATE_METRIC_DOUBLE(
         builder().esType("aggregate_metric_double")
             .estimatedSize(Double.BYTES * 3 + Integer.BYTES)
-            .supportedSince(DataTypesTransportVersions.ESQL_AGGREGATE_METRIC_DOUBLE_CREATED_VERSION)
+            .supportedSince(
+                DataTypesTransportVersions.ESQL_AGGREGATE_METRIC_DOUBLE_CREATED_VERSION,
+                DataTypesTransportVersions.ESQL_AGGREGATE_METRIC_DOUBLE_CREATED_VERSION
+            )
     ),
     /**
      * Fields with this type are dense vectors, represented as an array of float values.
      */
     DENSE_VECTOR(
-        builder().esType("dense_vector").estimatedSize(4096).supportedSince(DataTypesTransportVersions.ESQL_DENSE_VECTOR_CREATED_VERSION)
+        builder().esType("dense_vector")
+            .estimatedSize(4096)
+            .supportedSince(
+                DataTypesTransportVersions.ESQL_DENSE_VECTOR_CREATED_VERSION,
+                DataTypesTransportVersions.ESQL_DENSE_VECTOR_CREATED_VERSION
+            )
     );
 
     public static final Set<DataType> UNDER_CONSTRUCTION = Arrays.stream(DataType.values())
-        .filter(t -> t.supportedVersion() == SupportedVersion.UNDER_CONSTRUCTION)
+        .filter(t -> t.supportedVersion().underConstruction())
         .collect(Collectors.toSet());
 
     private final String typeName;
@@ -952,13 +989,15 @@ Builder counter(DataType counter) {
         }
 
         /**
-         * The version from when on a {@link DataType} is supported. When a query tries to use a data type
-         * not supported on any of the nodes it runs on, it is invalid.
+         * Marks a type that is supported in production since {@code supportedVersion}.
+         * When a query tries to use a data type not supported on the nodes it runs on, this is a bug.
          * <p>
-         * Generally, we should add a dedicated transport version when a type is enabled on release builds.
+         * On snapshot builds, the {@code createdVersion} is used instead, so that existing tests continue
+         * to work after release if the type was previously {@link #underConstruction(TransportVersion)};
+         * the under-construction version should be used as the {@code createdVersion}.
          */
-        Builder supportedSince(TransportVersion supportedVersion) {
-            this.supportedVersion = SupportedVersion.supportedSince(supportedVersion);
+        Builder supportedSince(TransportVersion createdVersion, TransportVersion supportedVersion) {
+            this.supportedVersion = SupportedVersion.supportedSince(createdVersion, supportedVersion);
             return this;
         }
 
@@ -967,8 +1006,12 @@ Builder supportedOnAllNodes() {
             return this;
         }
 
-        Builder underConstruction() {
-            this.supportedVersion = SupportedVersion.UNDER_CONSTRUCTION;
+        /**
+         * Marks a type that is not supported in production yet, but is supported in snapshot builds
+         * starting with the given version.
+         */
+        Builder underConstruction(TransportVersion createdVersion) {
+            this.supportedVersion = SupportedVersion.underConstruction(createdVersion);
             return this;
         }
     }

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

@@ -17,6 +17,10 @@ default boolean supportedLocally() {
         return supportedOn(TransportVersion.current(), Build.current().isSnapshot());
     }
 
+    default boolean underConstruction() {
+        return false;
+    }
+
     SupportedVersion SUPPORTED_ON_ALL_NODES = new SupportedVersion() {
         @Override
         public boolean supportedOn(TransportVersion version, boolean currentBuildIsSnapshot) {
@@ -37,47 +41,55 @@ public String toString() {
      *     <li>
      *         Not tested by {@code ErrorsForCasesWithoutExamplesTestCase} subclasses.
      *         When a function supports a type it includes a test case in its subclass
-     *         of {@code AbstractFunctionTestCase}. If a function does not support.
+     *         of {@code AbstractFunctionTestCase}. If a function does not support
      *         them like {@code TO_STRING} then the tests won't notice. See class javadoc
      *         for instructions on adding new types, but that usually involves adding support
      *         for that type to a handful of functions. Once you've done that you should be
-     *         able to remove your new type from UNDER_CONSTRUCTION and update a few error
-     *         messages.
+     *         able to turn your new type from under construction into released and update
+     *         a few error messages.
      *     </li>
      * </ul>
      * <p>
-     *     Snapshot builds treat these as always supported so that we can write tests before actually
-     *     turning on the support for the type. Mixed/multi cluster tests with older nodes have to be
-     *     skipped based on capabilites, as always.
+     *     Snapshot builds treat these as supported starting from the version they were created on,
+     *     so that we can write tests before actually turning on the support for the type.
+     *     Mixed/multi cluster tests with older nodes should be skipped based on SNAPSHOT-only
+     *     capabilites, as always.
      */
     // We used to have a feature-flag based override, so that in-development types could be
     // turned on for testing in release builds. If needed, it's fine to bring this back, but we
     // need to make sure that other checks for types being under construction are also overridden.
-    // Check usage of this constant to be sure.
-    SupportedVersion UNDER_CONSTRUCTION = new SupportedVersion() {
-        @Override
-        public boolean supportedOn(TransportVersion version, boolean currentBuildIsSnapshot) {
-            return currentBuildIsSnapshot;
-        }
+    // Check usage of this method to be sure.
+    static SupportedVersion underConstruction(TransportVersion createdVersion) {
+        return new SupportedVersion() {
+            @Override
+            public boolean supportedOn(TransportVersion version, boolean currentBuildIsSnapshot) {
+                return currentBuildIsSnapshot && version.supports(createdVersion);
+            }
 
-        @Override
-        public String toString() {
-            return "UnderConstruction";
-        }
-    };
+            @Override
+            public String toString() {
+                return "UnderConstruction";
+            }
+
+            @Override
+            public boolean underConstruction() {
+                return true;
+            }
+        };
+    }
 
     /**
      * Types that are supported starting with the given version.
      * <p>
-     *     Snapshot builds treat these as always supported, so that any existing tests using them
-     *     continue to work. Otherwise, we'd have to update bwc tests to skip older versions based
-     *     on a capability check, which can be error-prone and risks turning off an unrelated bwc test.
+     * Snapshot builds treat these as supported from their created version onward, so that any existing tests
+     * using them should continue to work.
      */
-    static SupportedVersion supportedSince(TransportVersion supportedVersion) {
+    static SupportedVersion supportedSince(TransportVersion createdVersion, TransportVersion supportedVersion) {
+        assert supportedVersion.onOrAfter(createdVersion) : "support for a type cannot be enabled before its initial creation";
         return new SupportedVersion() {
             @Override
             public boolean supportedOn(TransportVersion version, boolean currentBuildIsSnapshot) {
-                return version.supports(supportedVersion) || currentBuildIsSnapshot;
+                return currentBuildIsSnapshot ? version.supports(createdVersion) : version.supports(supportedVersion);
             }
 
             @Override