Minimize code diff after Re-enable LOOKUP JOIN tests in 8.18 (#118373)

When we back-ported the LOOKUP JOIN PRs to 8.x (see #117967), we found it necessary to disable all csv-spec tests since they create indices with mode:lookup, which is illegal in the cluster state of mixed clusters where other nodes do not understand the new index mode. We need to re-enable the tests, and make sure the tests are only disabled in mixed clusters with node versions too old to handle the new mode.
elastic · Dec 12, 2024 · d53ccaf · d53ccaf
1 parent 78488c7
commit d53ccaf
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 20 deletions.
diff --git a/x-pack/plugin/esql/qa/server/mixed-cluster/build.gradle b/x-pack/plugin/esql/qa/server/mixed-cluster/build.gradle
@@ -27,6 +27,7 @@ restResources {
 dependencies {
   javaRestTestImplementation project(xpackModule('esql:qa:testFixtures'))
   javaRestTestImplementation project(xpackModule('esql:qa:server'))
+  javaRestTestImplementation project(xpackModule('esql'))
 }
 
 GradleUtils.extendSourceSet(project, "javaRestTest", "yamlRestTest")

diff --git a/...r/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java b/...r/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/mixed/MixedClusterEsqlSpecIT.java
@@ -18,8 +18,10 @@
 import org.junit.ClassRule;
 
 import java.io.IOException;
+import java.util.List;
 
 import static org.elasticsearch.xpack.esql.CsvTestUtils.isEnabled;
+import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.JOIN_LOOKUP_V4;
 import static org.elasticsearch.xpack.esql.qa.rest.EsqlSpecTestCase.Mode.ASYNC;
 
 public class MixedClusterEsqlSpecIT extends EsqlSpecTestCase {
@@ -92,6 +94,11 @@ protected boolean supportsInferenceTestService() {
         return false;
     }
 
+    @Override
+    protected boolean supportsIndexModeLookup() throws IOException {
+        return hasCapabilities(List.of(JOIN_LOOKUP_V4.capabilityName()));
+    }
+
     @Override
     protected boolean deduplicateExactWarnings() {
         /*

diff --git a/...i-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java b/...i-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
@@ -280,4 +280,11 @@ protected boolean enableRoundingDoubleValuesOnAsserting() {
     protected boolean supportsInferenceTestService() {
         return false;
     }
+
+    @Override
+    protected boolean supportsIndexModeLookup() throws IOException {
+        // CCS does not yet support JOIN_LOOKUP_V4 and clusters falsely report they have this capability
+        // return hasCapabilities(List.of(JOIN_LOOKUP_V4.capabilityName()));
+        return false;
+    }
 }
diff --git a/...n/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java b/...n/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java
@@ -135,8 +135,8 @@ public void setup() throws IOException {
             createInferenceEndpoint(client());
         }
 
-        if (indexExists(availableDatasetsForEs(client()).iterator().next().indexName()) == false) {
-            loadDataSetIntoEs(client());
+        if (indexExists(availableDatasetsForEs(client(), supportsIndexModeLookup()).iterator().next().indexName()) == false) {
+            loadDataSetIntoEs(client(), supportsIndexModeLookup());
         }
     }
 
@@ -182,12 +182,30 @@ protected void shouldSkipTest(String testName) throws IOException {
 
     protected static void checkCapabilities(RestClient client, TestFeatureService testFeatureService, String testName, CsvTestCase testCase)
         throws IOException {
-        if (testCase.requiredCapabilities.isEmpty()) {
+        if (hasCapabilities(client, testCase.requiredCapabilities)) {
             return;
         }
+
+        var features = new EsqlFeatures().getFeatures().stream().map(NodeFeature::id).collect(Collectors.toSet());
+
+        for (String feature : testCase.requiredCapabilities) {
+            var esqlFeature = "esql." + feature;
+            assumeTrue("Requested capability " + feature + " is an ESQL cluster feature", features.contains(esqlFeature));
+            assumeTrue("Test " + testName + " requires " + feature, testFeatureService.clusterHasFeature(esqlFeature));
+        }
+    }
+
+    protected static boolean hasCapabilities(List<String> requiredCapabilities) throws IOException {
+        return hasCapabilities(adminClient(), requiredCapabilities);
+    }
+
+    protected static boolean hasCapabilities(RestClient client, List<String> requiredCapabilities) throws IOException {
+        if (requiredCapabilities.isEmpty()) {
+            return true;
+        }
         try {
-            if (clusterHasCapability(client, "POST", "/_query", List.of(), testCase.requiredCapabilities).orElse(false)) {
-                return;
+            if (clusterHasCapability(client, "POST", "/_query", List.of(), requiredCapabilities).orElse(false)) {
+                return true;
             }
             LOGGER.info("capabilities API returned false, we might be in a mixed version cluster so falling back to cluster features");
         } catch (ResponseException e) {
@@ -206,20 +224,17 @@ protected static void checkCapabilities(RestClient client, TestFeatureService te
                 throw e;
             }
         }
-
-        var features = new EsqlFeatures().getFeatures().stream().map(NodeFeature::id).collect(Collectors.toSet());
-
-        for (String feature : testCase.requiredCapabilities) {
-            var esqlFeature = "esql." + feature;
-            assumeTrue("Requested capability " + feature + " is an ESQL cluster feature", features.contains(esqlFeature));
-            assumeTrue("Test " + testName + " requires " + feature, testFeatureService.clusterHasFeature(esqlFeature));
-        }
+        return false;
     }
 
     protected boolean supportsInferenceTestService() {
         return true;
     }
 
+    protected boolean supportsIndexModeLookup() throws IOException {
+        return true;
+    }
+
     protected final void doTest() throws Throwable {
         RequestObjectBuilder builder = new RequestObjectBuilder(randomFrom(XContentType.values()));
 

diff --git a/...ver/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java b/...ver/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java
@@ -46,7 +46,7 @@ public abstract class GenerativeRestTest extends ESRestTestCase {
     @Before
     public void setup() throws IOException {
         if (indexExists(CSV_DATASET_MAP.keySet().iterator().next()) == false) {
-            loadDataSetIntoEs(client());
+            loadDataSetIntoEs(client(), true);
         }
     }
 

diff --git a/...n/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestsDataLoader.java b/...n/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestsDataLoader.java
@@ -235,7 +235,7 @@ public static void main(String[] args) throws IOException {
         }
 
         try (RestClient client = builder.build()) {
-            loadDataSetIntoEs(client, (restClient, indexName, indexMapping, indexSettings) -> {
+            loadDataSetIntoEs(client, true, (restClient, indexName, indexMapping, indexSettings) -> {
                 // don't use ESRestTestCase methods here or, if you do, test running the main method before making the change
                 StringBuilder jsonBody = new StringBuilder("{");
                 if (indexSettings != null && indexSettings.isEmpty() == false) {
@@ -254,26 +254,28 @@ public static void main(String[] args) throws IOException {
         }
     }
 
-    public static Set<TestsDataset> availableDatasetsForEs(RestClient client) throws IOException {
+    public static Set<TestsDataset> availableDatasetsForEs(RestClient client, boolean supportsIndexModeLookup) throws IOException {
         boolean inferenceEnabled = clusterHasInferenceEndpoint(client);
 
         return CSV_DATASET_MAP.values()
             .stream()
             .filter(d -> d.requiresInferenceEndpoint == false || inferenceEnabled)
+            .filter(d -> supportsIndexModeLookup || d.indexName.endsWith("_lookup") == false) // TODO: use actual index settings
             .collect(Collectors.toCollection(HashSet::new));
     }
 
-    public static void loadDataSetIntoEs(RestClient client) throws IOException {
-        loadDataSetIntoEs(client, (restClient, indexName, indexMapping, indexSettings) -> {
+    public static void loadDataSetIntoEs(RestClient client, boolean supportsIndexModeLookup) throws IOException {
+        loadDataSetIntoEs(client, supportsIndexModeLookup, (restClient, indexName, indexMapping, indexSettings) -> {
             ESRestTestCase.createIndex(restClient, indexName, indexSettings, indexMapping, null);
         });
     }
 
-    private static void loadDataSetIntoEs(RestClient client, IndexCreator indexCreator) throws IOException {
+    private static void loadDataSetIntoEs(RestClient client, boolean supportsIndexModeLookup, IndexCreator indexCreator)
+        throws IOException {
         Logger logger = LogManager.getLogger(CsvTestsDataLoader.class);
 
         Set<String> loadedDatasets = new HashSet<>();
-        for (var dataset : availableDatasetsForEs(client)) {
+        for (var dataset : availableDatasetsForEs(client, supportsIndexModeLookup)) {
             load(client, dataset, logger, indexCreator);
             loadedDatasets.add(dataset.indexName);
         }