From 9b50f2bfca0068a6c7792d6bc1ea2ce6dabd3e4c Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 1 Apr 2025 17:26:16 +0200 Subject: [PATCH 01/11] Integration test for LOOKUP JOIN between various types --- .../xpack/esql/action/LookupJoinTypesIT.java | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java new file mode 100644 index 0000000000000..27d901f1b70ee --- /dev/null +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -0,0 +1,137 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql.action; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.ESIntegTestCase.ClusterScope; +import org.elasticsearch.xpack.esql.core.type.DataType; +import org.elasticsearch.xpack.esql.plugin.EsqlPlugin; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.elasticsearch.test.ESIntegTestCase.Scope.SUITE; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.equalTo; + +@ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) +public class LookupJoinTypesIT extends ESIntegTestCase { + + private static final Map compatibleJoinTypes = Map.of( + DataType.KEYWORD, + DataType.KEYWORD, + DataType.TEXT, + DataType.KEYWORD, + DataType.INTEGER, + DataType.INTEGER, + DataType.FLOAT, + DataType.FLOAT, + DataType.DOUBLE, + DataType.DOUBLE + ); + + protected Collection> nodePlugins() { + return List.of(EsqlPlugin.class); + } + + public void testLookupJoinTypes() { + initIndexes(); + initData(); + for (Map.Entry entry : compatibleJoinTypes.entrySet()) { + String query = String.format( + Locale.ROOT, + "FROM index | LOOKUP JOIN %s ON field_%s | KEEP other", + indexName(entry.getKey(), entry.getValue()), + entry.getKey().esType() + ); + try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { + Iterator results = response.response().column(0).iterator(); + assertTrue("Expected at least one result for query: " + query, results.hasNext()); + Object indexedResult = response.response().column(0).iterator().next(); + assertThat("Expected valid result: " + query, indexedResult, equalTo("value")); + } + } + } + + private void initIndexes() { + // The main index will have many fields, one of each type to use in later type specific joins + StringBuilder mainFields = new StringBuilder("{\n \"properties\" : {\n"); + mainFields.append( + compatibleJoinTypes.keySet() + .stream() + .map((l) -> "\"field_" + l.esType() + "\": { \"type\" : \"" + l.esType() + "\" }") + .collect(Collectors.joining(",\n ")) + ); + mainFields.append(" }\n}\n"); + assertAcked(prepareCreate("index").setMapping(mainFields.toString())); + + Settings.Builder settings = Settings.builder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + .put("index.mode", "lookup"); + compatibleJoinTypes.forEach( + // Each lookup index will get a document with a field to join on, and a results field to get back + (l, r) -> { assertAcked(prepareCreate(indexName(l, r)).setSettings(settings.build()).setMapping(String.format(Locale.ROOT, """ + { + "properties" : { + "field_%s": { "type" : "%s" }, + "other": { "type" : "keyword" } + } + } + """, l.esType(), r.esType()))); } + ); + } + + private String indexName(DataType mainType, DataType lookupType) { + return "index_" + mainType.esType() + "_" + lookupType.esType(); + } + + private void initData() { + List mainProperties = new ArrayList<>(); + int docId = 0; + for (Map.Entry entry : compatibleJoinTypes.entrySet()) { + DataType mainType = entry.getKey(); + DataType lookupType = entry.getValue(); + String index = indexName(mainType, lookupType); + String field = "field_" + mainType.esType(); + String value = sampleDataFor(lookupType); + String doc = String.format(Locale.ROOT, """ + { + "%s": %s, + "other": "value" + } + """, field, value); + mainProperties.add(String.format(Locale.ROOT, "\"%s\": %s", field, value)); + index(index, "" + (++docId), doc); + refresh(index); + } + index("index", "1", String.format(Locale.ROOT, """ + { + %s + } + """, String.join(",\n", mainProperties))); + refresh("index"); + } + + private String sampleDataFor(DataType type) { + return switch (type) { + case KEYWORD -> "\"key\""; + case TEXT -> "\"key text\""; + case INTEGER -> "1"; + case FLOAT, DOUBLE -> "1.0"; + default -> throw new IllegalArgumentException("Unsupported type: " + type); + }; + } +} From b296e3426d8cd89174094ec2ea941cc528d50261 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 1 Apr 2025 17:41:43 +0200 Subject: [PATCH 02/11] Refactored to be easier to read and extend the tests --- .../xpack/esql/action/LookupJoinTypesIT.java | 82 ++++++++++--------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index 27d901f1b70ee..f8f5e3606acd6 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -17,9 +17,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; -import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import static org.elasticsearch.test.ESIntegTestCase.Scope.SUITE; @@ -29,18 +30,32 @@ @ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) public class LookupJoinTypesIT extends ESIntegTestCase { - private static final Map compatibleJoinTypes = Map.of( - DataType.KEYWORD, - DataType.KEYWORD, - DataType.TEXT, - DataType.KEYWORD, - DataType.INTEGER, - DataType.INTEGER, - DataType.FLOAT, - DataType.FLOAT, - DataType.DOUBLE, - DataType.DOUBLE - ); + private static final Set compatibleJoinTypes = new LinkedHashSet<>(); + static { + addConfig(DataType.KEYWORD, DataType.KEYWORD, true); + addConfig(DataType.TEXT, DataType.KEYWORD, true); + addConfig(DataType.INTEGER, DataType.INTEGER, true); + addConfig(DataType.FLOAT, DataType.FLOAT, true); + addConfig(DataType.DOUBLE, DataType.DOUBLE, true); + } + + private static void addConfig(DataType mainType, DataType lookupType, boolean passes) { + compatibleJoinTypes.add(new TestConfig(mainType, lookupType, passes)); + } + + record TestConfig(DataType mainType, DataType lookupType, boolean passes) { + private String indexName() { + return "index_" + mainType.esType() + "_" + lookupType.esType(); + } + + private String fieldName() { + return "field_" + mainType.esType(); + } + + private String mainProperty() { + return "\"" + fieldName() + "\": { \"type\" : \"" + mainType.esType() + "\" }"; + } + } protected Collection> nodePlugins() { return List.of(EsqlPlugin.class); @@ -49,12 +64,12 @@ protected Collection> nodePlugins() { public void testLookupJoinTypes() { initIndexes(); initData(); - for (Map.Entry entry : compatibleJoinTypes.entrySet()) { + for (TestConfig config : compatibleJoinTypes) { String query = String.format( Locale.ROOT, - "FROM index | LOOKUP JOIN %s ON field_%s | KEEP other", - indexName(entry.getKey(), entry.getValue()), - entry.getKey().esType() + "FROM index | LOOKUP JOIN %s ON %s | KEEP other", + config.indexName(), + config.fieldName() ); try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { Iterator results = response.response().column(0).iterator(); @@ -68,12 +83,7 @@ public void testLookupJoinTypes() { private void initIndexes() { // The main index will have many fields, one of each type to use in later type specific joins StringBuilder mainFields = new StringBuilder("{\n \"properties\" : {\n"); - mainFields.append( - compatibleJoinTypes.keySet() - .stream() - .map((l) -> "\"field_" + l.esType() + "\": { \"type\" : \"" + l.esType() + "\" }") - .collect(Collectors.joining(",\n ")) - ); + mainFields.append(compatibleJoinTypes.stream().map(TestConfig::mainProperty).collect(Collectors.joining(",\n "))); mainFields.append(" }\n}\n"); assertAcked(prepareCreate("index").setMapping(mainFields.toString())); @@ -83,39 +93,31 @@ private void initIndexes() { .put("index.mode", "lookup"); compatibleJoinTypes.forEach( // Each lookup index will get a document with a field to join on, and a results field to get back - (l, r) -> { assertAcked(prepareCreate(indexName(l, r)).setSettings(settings.build()).setMapping(String.format(Locale.ROOT, """ + (c) -> { assertAcked(prepareCreate(c.indexName()).setSettings(settings.build()).setMapping(String.format(Locale.ROOT, """ { "properties" : { - "field_%s": { "type" : "%s" }, + "%s": { "type" : "%s" }, "other": { "type" : "keyword" } } } - """, l.esType(), r.esType()))); } + """, c.fieldName(), c.lookupType.esType()))); } ); } - private String indexName(DataType mainType, DataType lookupType) { - return "index_" + mainType.esType() + "_" + lookupType.esType(); - } - private void initData() { List mainProperties = new ArrayList<>(); int docId = 0; - for (Map.Entry entry : compatibleJoinTypes.entrySet()) { - DataType mainType = entry.getKey(); - DataType lookupType = entry.getValue(); - String index = indexName(mainType, lookupType); - String field = "field_" + mainType.esType(); - String value = sampleDataFor(lookupType); + for (TestConfig config : compatibleJoinTypes) { + String value = sampleDataFor(config.lookupType()); String doc = String.format(Locale.ROOT, """ { "%s": %s, "other": "value" } - """, field, value); - mainProperties.add(String.format(Locale.ROOT, "\"%s\": %s", field, value)); - index(index, "" + (++docId), doc); - refresh(index); + """, config.fieldName(), value); + mainProperties.add(String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), value)); + index(config.indexName(), "" + (++docId), doc); + refresh(config.indexName()); } index("index", "1", String.format(Locale.ROOT, """ { From 19bb2a501a19536ddb42efa0a771b9db85e07c7f Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 2 Apr 2025 15:44:44 +0200 Subject: [PATCH 03/11] Support many combinations with errors and empty results --- .../xpack/esql/action/LookupJoinTypesIT.java | 333 +++++++++++++++--- 1 file changed, 280 insertions(+), 53 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index f8f5e3606acd6..e354aea061ef4 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -11,49 +11,256 @@ import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; +import org.elasticsearch.xpack.core.esql.action.ColumnInfo; +import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.plugin.EsqlPlugin; -import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Set; +import java.util.function.Consumer; import java.util.stream.Collectors; import static org.elasticsearch.test.ESIntegTestCase.Scope.SUITE; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; +/** + * This test suite tests the lookup join functionality in ESQL with various data types. + * For each pair of types being tested, it builds a main index called "index" containing a single document with as many fields as + * types being tested on the left of the pair, and then creates that many other lookup indexes, each with a single document containing + * exactly two fields: the field to join on, and a field to return. + * For example, if we are testing the pairs (double, double), (double, float), (float, double) and (float, float), + * we will create the following indexes: + *
+ *
index_double_double: containing
+ *
Index containing da single document with a field of type 'double' like: 
+ *         {
+ *             "field_double": 1.0,  // this is mapped as type 'double'
+ *             "other": "value"
+ *         }
+ *
+ *
index_double_float
+ *
Index containing a single document with a field of type 'float' like: 
+ *         {
+ *             "field_double": 1.0,  // this is mapped as type 'float'
+ *             "other": "value"
+ *         }
+ *
+ *
index_float_double
+ *
Index containing a single document with a field of type 'double' like: 
+ *         {
+ *             "field_float": 1.0,  // this is mapped as type 'double'
+ *             "other": "value"
+ *         }
+ *
+ *
index_float_float
+ *
Index containing single document with a field of type 'float' like: 
+ *         {
+ *             "field_float": 1.0,  // this is mapped as type 'float'
+ *             "other": "value"
+ *         }
+ *
+ *
index
+ *
Index containing document like: 
+ *         {
+ *             "field_double": 1.0,
+ *             "field_float": 1.0
+ *         }
+ *
+ *
+ * Note that the lookup indexes have fields with a name that matches the type in the main index, and not the type actually used in the + * lookup index. Instead, the mapped type should be the type of the right-hand side of the pair being tested. + * Then we can run queries like: + * 
+ *     FROM index | LOOKUP JOIN index_double_float ON field_double | KEEP other
+ *
+ * And assert that the result exists and is equal to "value". + */ @ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) public class LookupJoinTypesIT extends ESIntegTestCase { - private static final Set compatibleJoinTypes = new LinkedHashSet<>(); + private static final Map testConfigurations = new HashMap<>(); static { - addConfig(DataType.KEYWORD, DataType.KEYWORD, true); - addConfig(DataType.TEXT, DataType.KEYWORD, true); - addConfig(DataType.INTEGER, DataType.INTEGER, true); - addConfig(DataType.FLOAT, DataType.FLOAT, true); - addConfig(DataType.DOUBLE, DataType.DOUBLE, true); + // Initialize the test configurations for string tests + { + TestConfigs configs = testConfigurations.computeIfAbsent("strings", k -> new TestConfigs(k, new LinkedHashSet<>())); + configs.addPasses(DataType.KEYWORD, DataType.KEYWORD); + configs.addPasses(DataType.TEXT, DataType.KEYWORD); + configs.addFailsText(DataType.KEYWORD, DataType.TEXT); + configs.addFailsText(DataType.TEXT, DataType.TEXT); + } + + // Test integer types + { + TestConfigs configs = testConfigurations.computeIfAbsent("integers", k -> new TestConfigs(k, new LinkedHashSet<>())); + var integerTypes = List.of(DataType.BYTE, DataType.SHORT, DataType.INTEGER); + for (DataType mainType : integerTypes) { + for (DataType lookupType : integerTypes) { + configs.addPasses(mainType, lookupType); + } + // Long is currently treated differently in the validation, but we could consider changing that + configs.addFails(mainType, DataType.LONG); + configs.addFails(DataType.LONG, mainType); + } + } + + // Test float and double + { + TestConfigs configs = testConfigurations.computeIfAbsent("floats", k -> new TestConfigs(k, new LinkedHashSet<>())); + var floatTypes = List.of(DataType.FLOAT, DataType.DOUBLE); + for (DataType mainType : floatTypes) { + for (DataType lookupType : floatTypes) { + configs.addPasses(mainType, lookupType); + } + } + } + + // TODO: Add tests for mixed groups (should mostly fail, but might be some implicit casting to consider) + + // Make sure we have never added two configurations with the same index name + Set knownTypes = new HashSet<>(); + for (TestConfigs configs : testConfigurations.values()) { + for (TestConfig config : configs.configs()) { + if (knownTypes.contains(config.indexName())) { + throw new IllegalArgumentException("Duplicate index name: " + config.indexName()); + } + knownTypes.add(config.indexName()); + } + } } - private static void addConfig(DataType mainType, DataType lookupType, boolean passes) { - compatibleJoinTypes.add(new TestConfig(mainType, lookupType, passes)); + private record TestConfigs(String group, Set configs) { + + private void addPasses(DataType mainType, DataType lookupType) { + configs.add(new TestConfigPasses(mainType, lookupType, true)); + } + + private void addEmptyResult(DataType mainType, DataType lookupType) { + configs.add(new TestConfigPasses(mainType, lookupType, false)); + } + + private void addFails(DataType mainType, DataType lookupType) { + String fieldName = "field_" + mainType.esType(); + String errorMessage = String.format( + Locale.ROOT, + "JOIN left field [%s] of type [%s] is incompatible with right field [%s] of type [%s]", + fieldName, + mainType.widenSmallNumeric(), + fieldName, + lookupType.widenSmallNumeric() + ); + configs.add( + new TestConfigFails<>( + mainType, + lookupType, + VerificationException.class, + e -> assertThat(e.getMessage(), containsString(errorMessage)) + ) + ); + } + + private void addFailsText(DataType mainType, DataType lookupType) { + String fieldName = "field_" + mainType.esType(); + String errorMessage = String.format(Locale.ROOT, "JOIN with right field [%s] of type [TEXT] is not supported", fieldName); + configs.add( + new TestConfigFails<>( + mainType, + lookupType, + VerificationException.class, + e -> assertThat(e.getMessage(), containsString(errorMessage)) + ) + ); + } + + private void addFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) { + configs.add(new TestConfigFails<>(mainType, lookupType, exception, assertion)); + } + } + + interface TestConfig { + DataType mainType(); + + DataType lookupType(); + + default String indexName() { + return "index_" + mainType().esType() + "_" + lookupType().esType(); + } + + default String fieldName() { + return "field_" + mainType().esType(); + } + + default String mainPropertySpec() { + return "\"" + fieldName() + "\": { \"type\" : \"" + mainType().esType() + "\" }"; + } + + /** Make sure the left index has the expected fields and types */ + default void validateMainIndex() { + validateIndex("index", fieldName(), sampleDataFor(mainType())); + } + + /** Make sure the lookup index has the expected fields and types */ + default void validateLookupIndex() { + validateIndex(indexName(), fieldName(), sampleDataFor(lookupType())); + } + + void testQuery(String query); } - record TestConfig(DataType mainType, DataType lookupType, boolean passes) { - private String indexName() { - return "index_" + mainType.esType() + "_" + lookupType.esType(); + private static void validateIndex(String indexName, String fieldName, Object expectedValue) { + String query = String.format(Locale.ROOT, "FROM %s | KEEP %s", indexName, fieldName); + try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { + ColumnInfo info = response.response().columns().getFirst(); + assertThat("Expected index '" + indexName + "' to have column '" + fieldName + ": " + query, info.name(), is(fieldName)); + Iterator results = response.response().column(0).iterator(); + assertTrue("Expected at least one result for query: " + query, results.hasNext()); + Object indexedResult = response.response().column(0).iterator().next(); + assertThat("Expected valid result: " + query, indexedResult, is(expectedValue)); } + } - private String fieldName() { - return "field_" + mainType.esType(); + record TestConfigPasses(DataType mainType, DataType lookupType, boolean hasResults) implements TestConfig { + @Override + public void testQuery(String query) { + try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { + Iterator results = response.response().column(0).iterator(); + assertTrue("Expected at least one result for query: " + query, results.hasNext()); + Object indexedResult = response.response().column(0).iterator().next(); + if (hasResults) { + assertThat("Expected valid result: " + query, indexedResult, equalTo("value")); + } else { + assertThat("Expected empty results for query: " + query, indexedResult, is(nullValue())); + } + } } + } - private String mainProperty() { - return "\"" + fieldName() + "\": { \"type\" : \"" + mainType.esType() + "\" }"; + record TestConfigFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) + implements + TestConfig { + @Override + public void testQuery(String query) { + E e = expectThrows( + exception(), + "Expected exception " + exception().getSimpleName() + " but no exception was thrown: " + query, + () -> { + try (var ignored = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { + // We use try-with-resources to ensure the request is closed if the exception is not thrown (less cluttered errors) + } + } + ); + assertion().accept(e); } } @@ -61,78 +268,98 @@ protected Collection> nodePlugins() { return List.of(EsqlPlugin.class); } - public void testLookupJoinTypes() { - initIndexes(); - initData(); - for (TestConfig config : compatibleJoinTypes) { + public void testLookupJoinStrings() { + testLookupJoinTypes("strings"); + } + + public void testLookupJoinIntegers() { + testLookupJoinTypes("integers"); + } + + public void testLookupJoinFloats() { + testLookupJoinTypes("floats"); + } + + private void testLookupJoinTypes(String group) { + initIndexes(group); + initData(group); + for (TestConfig config : testConfigurations.get(group).configs()) { String query = String.format( Locale.ROOT, "FROM index | LOOKUP JOIN %s ON %s | KEEP other", config.indexName(), config.fieldName() ); - try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { - Iterator results = response.response().column(0).iterator(); - assertTrue("Expected at least one result for query: " + query, results.hasNext()); - Object indexedResult = response.response().column(0).iterator().next(); - assertThat("Expected valid result: " + query, indexedResult, equalTo("value")); - } + config.validateMainIndex(); + config.validateLookupIndex(); + config.testQuery(query); } } - private void initIndexes() { + private void initIndexes(String group) { + Set configs = testConfigurations.get(group).configs; // The main index will have many fields, one of each type to use in later type specific joins - StringBuilder mainFields = new StringBuilder("{\n \"properties\" : {\n"); - mainFields.append(compatibleJoinTypes.stream().map(TestConfig::mainProperty).collect(Collectors.joining(",\n "))); - mainFields.append(" }\n}\n"); - assertAcked(prepareCreate("index").setMapping(mainFields.toString())); + String mainFields = "{\n \"properties\" : {\n" + + configs.stream().map(TestConfig::mainPropertySpec).distinct().collect(Collectors.joining(",\n ")) + + " }\n}\n"; + assertAcked(prepareCreate("index").setMapping(mainFields)); Settings.Builder settings = Settings.builder() .put("index.number_of_shards", 1) .put("index.number_of_replicas", 0) .put("index.mode", "lookup"); - compatibleJoinTypes.forEach( + configs.forEach( // Each lookup index will get a document with a field to join on, and a results field to get back - (c) -> { assertAcked(prepareCreate(c.indexName()).setSettings(settings.build()).setMapping(String.format(Locale.ROOT, """ - { - "properties" : { - "%s": { "type" : "%s" }, - "other": { "type" : "keyword" } - } - } - """, c.fieldName(), c.lookupType.esType()))); } + (c) -> assertAcked( + prepareCreate(c.indexName()).setSettings(settings.build()) + .setMapping(c.fieldName(), "type=" + c.lookupType().esType(), "other", "type=keyword") + ) ); } - private void initData() { - List mainProperties = new ArrayList<>(); + private void initData(String group) { + Set configs = testConfigurations.get(group).configs; int docId = 0; - for (TestConfig config : compatibleJoinTypes) { - String value = sampleDataFor(config.lookupType()); + for (TestConfig config : configs) { String doc = String.format(Locale.ROOT, """ { - "%s": %s, + %s, "other": "value" } - """, config.fieldName(), value); - mainProperties.add(String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), value)); + """, lookupPropertyFor(config)); index(config.indexName(), "" + (++docId), doc); refresh(config.indexName()); } + List mainProperties = configs.stream().map(this::mainPropertyFor).distinct().collect(Collectors.toList()); index("index", "1", String.format(Locale.ROOT, """ { %s } - """, String.join(",\n", mainProperties))); + """, String.join(",\n ", mainProperties))); refresh("index"); } - private String sampleDataFor(DataType type) { + private String lookupPropertyFor(TestConfig config) { + return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.lookupType())); + } + + private String mainPropertyFor(TestConfig config) { + return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.mainType())); + } + + private static String sampleDataTextFor(DataType type) { + return switch (type) { + case KEYWORD, TEXT -> "\"" + sampleDataFor(type) + "\""; + default -> String.valueOf(sampleDataFor(type)); + }; + } + + private static Object sampleDataFor(DataType type) { return switch (type) { - case KEYWORD -> "\"key\""; - case TEXT -> "\"key text\""; - case INTEGER -> "1"; - case FLOAT, DOUBLE -> "1.0"; + case KEYWORD, TEXT -> "key"; + case BYTE, SHORT, INTEGER -> 1; + case LONG -> 1L; + case FLOAT, DOUBLE -> 1.0; default -> throw new IllegalArgumentException("Unsupported type: " + type); }; } From a388663071b7db1864751765c56d505a6de91e9c Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 2 Apr 2025 15:49:46 +0200 Subject: [PATCH 04/11] Reorder with private records at the bottom --- .../xpack/esql/action/LookupJoinTypesIT.java | 203 +++++++++--------- 1 file changed, 101 insertions(+), 102 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index e354aea061ef4..dc68bbbbe1349 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -89,6 +89,9 @@ */ @ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) public class LookupJoinTypesIT extends ESIntegTestCase { + protected Collection> nodePlugins() { + return List.of(EsqlPlugin.class); + } private static final Map testConfigurations = new HashMap<>(); static { @@ -140,6 +143,102 @@ public class LookupJoinTypesIT extends ESIntegTestCase { } } + public void testLookupJoinStrings() { + testLookupJoinTypes("strings"); + } + + public void testLookupJoinIntegers() { + testLookupJoinTypes("integers"); + } + + public void testLookupJoinFloats() { + testLookupJoinTypes("floats"); + } + + private void testLookupJoinTypes(String group) { + initIndexes(group); + initData(group); + for (TestConfig config : testConfigurations.get(group).configs()) { + String query = String.format( + Locale.ROOT, + "FROM index | LOOKUP JOIN %s ON %s | KEEP other", + config.indexName(), + config.fieldName() + ); + config.validateMainIndex(); + config.validateLookupIndex(); + config.testQuery(query); + } + } + + private void initIndexes(String group) { + Set configs = testConfigurations.get(group).configs; + // The main index will have many fields, one of each type to use in later type specific joins + String mainFields = "{\n \"properties\" : {\n" + + configs.stream().map(TestConfig::mainPropertySpec).distinct().collect(Collectors.joining(",\n ")) + + " }\n}\n"; + assertAcked(prepareCreate("index").setMapping(mainFields)); + + Settings.Builder settings = Settings.builder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + .put("index.mode", "lookup"); + configs.forEach( + // Each lookup index will get a document with a field to join on, and a results field to get back + (c) -> assertAcked( + prepareCreate(c.indexName()).setSettings(settings.build()) + .setMapping(c.fieldName(), "type=" + c.lookupType().esType(), "other", "type=keyword") + ) + ); + } + + private void initData(String group) { + Set configs = testConfigurations.get(group).configs; + int docId = 0; + for (TestConfig config : configs) { + String doc = String.format(Locale.ROOT, """ + { + %s, + "other": "value" + } + """, lookupPropertyFor(config)); + index(config.indexName(), "" + (++docId), doc); + refresh(config.indexName()); + } + List mainProperties = configs.stream().map(this::mainPropertyFor).distinct().collect(Collectors.toList()); + index("index", "1", String.format(Locale.ROOT, """ + { + %s + } + """, String.join(",\n ", mainProperties))); + refresh("index"); + } + + private String lookupPropertyFor(TestConfig config) { + return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.lookupType())); + } + + private String mainPropertyFor(TestConfig config) { + return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.mainType())); + } + + private static String sampleDataTextFor(DataType type) { + return switch (type) { + case KEYWORD, TEXT -> "\"" + sampleDataFor(type) + "\""; + default -> String.valueOf(sampleDataFor(type)); + }; + } + + private static Object sampleDataFor(DataType type) { + return switch (type) { + case KEYWORD, TEXT -> "key"; + case BYTE, SHORT, INTEGER -> 1; + case LONG -> 1L; + case FLOAT, DOUBLE -> 1.0; + default -> throw new IllegalArgumentException("Unsupported type: " + type); + }; + } + private record TestConfigs(String group, Set configs) { private void addPasses(DataType mainType, DataType lookupType) { @@ -230,7 +329,7 @@ private static void validateIndex(String indexName, String fieldName, Object exp } } - record TestConfigPasses(DataType mainType, DataType lookupType, boolean hasResults) implements TestConfig { + private record TestConfigPasses(DataType mainType, DataType lookupType, boolean hasResults) implements TestConfig { @Override public void testQuery(String query) { try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { @@ -246,7 +345,7 @@ public void testQuery(String query) { } } - record TestConfigFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) + private record TestConfigFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) implements TestConfig { @Override @@ -263,104 +362,4 @@ public void testQuery(String query) { assertion().accept(e); } } - - protected Collection> nodePlugins() { - return List.of(EsqlPlugin.class); - } - - public void testLookupJoinStrings() { - testLookupJoinTypes("strings"); - } - - public void testLookupJoinIntegers() { - testLookupJoinTypes("integers"); - } - - public void testLookupJoinFloats() { - testLookupJoinTypes("floats"); - } - - private void testLookupJoinTypes(String group) { - initIndexes(group); - initData(group); - for (TestConfig config : testConfigurations.get(group).configs()) { - String query = String.format( - Locale.ROOT, - "FROM index | LOOKUP JOIN %s ON %s | KEEP other", - config.indexName(), - config.fieldName() - ); - config.validateMainIndex(); - config.validateLookupIndex(); - config.testQuery(query); - } - } - - private void initIndexes(String group) { - Set configs = testConfigurations.get(group).configs; - // The main index will have many fields, one of each type to use in later type specific joins - String mainFields = "{\n \"properties\" : {\n" - + configs.stream().map(TestConfig::mainPropertySpec).distinct().collect(Collectors.joining(",\n ")) - + " }\n}\n"; - assertAcked(prepareCreate("index").setMapping(mainFields)); - - Settings.Builder settings = Settings.builder() - .put("index.number_of_shards", 1) - .put("index.number_of_replicas", 0) - .put("index.mode", "lookup"); - configs.forEach( - // Each lookup index will get a document with a field to join on, and a results field to get back - (c) -> assertAcked( - prepareCreate(c.indexName()).setSettings(settings.build()) - .setMapping(c.fieldName(), "type=" + c.lookupType().esType(), "other", "type=keyword") - ) - ); - } - - private void initData(String group) { - Set configs = testConfigurations.get(group).configs; - int docId = 0; - for (TestConfig config : configs) { - String doc = String.format(Locale.ROOT, """ - { - %s, - "other": "value" - } - """, lookupPropertyFor(config)); - index(config.indexName(), "" + (++docId), doc); - refresh(config.indexName()); - } - List mainProperties = configs.stream().map(this::mainPropertyFor).distinct().collect(Collectors.toList()); - index("index", "1", String.format(Locale.ROOT, """ - { - %s - } - """, String.join(",\n ", mainProperties))); - refresh("index"); - } - - private String lookupPropertyFor(TestConfig config) { - return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.lookupType())); - } - - private String mainPropertyFor(TestConfig config) { - return String.format(Locale.ROOT, "\"%s\": %s", config.fieldName(), sampleDataTextFor(config.mainType())); - } - - private static String sampleDataTextFor(DataType type) { - return switch (type) { - case KEYWORD, TEXT -> "\"" + sampleDataFor(type) + "\""; - default -> String.valueOf(sampleDataFor(type)); - }; - } - - private static Object sampleDataFor(DataType type) { - return switch (type) { - case KEYWORD, TEXT -> "key"; - case BYTE, SHORT, INTEGER -> 1; - case LONG -> 1L; - case FLOAT, DOUBLE -> 1.0; - default -> throw new IllegalArgumentException("Unsupported type: " + type); - }; - } } From 048c423382812d8bab9b8bad92d5cb5dbc2d02f1 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 2 Apr 2025 17:56:35 +0200 Subject: [PATCH 05/11] Support much wider range of types and mixed types --- .../xpack/esql/action/LookupJoinTypesIT.java | 141 ++++++++++++++---- .../xpack/esql/plan/logical/join/Join.java | 8 +- 2 files changed, 123 insertions(+), 26 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index dc68bbbbe1349..13dfc7bba8977 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.esql.action; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.mapper.extras.MapperExtrasPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; @@ -20,7 +21,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -30,6 +31,17 @@ import static org.elasticsearch.test.ESIntegTestCase.Scope.SUITE; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN; +import static org.elasticsearch.xpack.esql.core.type.DataType.BYTE; +import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE; +import static org.elasticsearch.xpack.esql.core.type.DataType.FLOAT; +import static org.elasticsearch.xpack.esql.core.type.DataType.HALF_FLOAT; +import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER; +import static org.elasticsearch.xpack.esql.core.type.DataType.IP; +import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD; +import static org.elasticsearch.xpack.esql.core.type.DataType.LONG; +import static org.elasticsearch.xpack.esql.core.type.DataType.SHORT; +import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -90,38 +102,38 @@ @ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) public class LookupJoinTypesIT extends ESIntegTestCase { protected Collection> nodePlugins() { - return List.of(EsqlPlugin.class); + return List.of(EsqlPlugin.class, MapperExtrasPlugin.class); } private static final Map testConfigurations = new HashMap<>(); static { // Initialize the test configurations for string tests { - TestConfigs configs = testConfigurations.computeIfAbsent("strings", k -> new TestConfigs(k, new LinkedHashSet<>())); - configs.addPasses(DataType.KEYWORD, DataType.KEYWORD); - configs.addPasses(DataType.TEXT, DataType.KEYWORD); - configs.addFailsText(DataType.KEYWORD, DataType.TEXT); - configs.addFailsText(DataType.TEXT, DataType.TEXT); + TestConfigs configs = testConfigurations.computeIfAbsent("strings", TestConfigs::new); + configs.addPasses(KEYWORD, KEYWORD); + configs.addPasses(TEXT, KEYWORD); + configs.addFailsText(KEYWORD, TEXT); + configs.addFailsText(TEXT, TEXT); } // Test integer types + var integerTypes = List.of(BYTE, SHORT, INTEGER); { - TestConfigs configs = testConfigurations.computeIfAbsent("integers", k -> new TestConfigs(k, new LinkedHashSet<>())); - var integerTypes = List.of(DataType.BYTE, DataType.SHORT, DataType.INTEGER); + TestConfigs configs = testConfigurations.computeIfAbsent("integers", TestConfigs::new); for (DataType mainType : integerTypes) { for (DataType lookupType : integerTypes) { configs.addPasses(mainType, lookupType); } // Long is currently treated differently in the validation, but we could consider changing that - configs.addFails(mainType, DataType.LONG); - configs.addFails(DataType.LONG, mainType); + configs.addFails(mainType, LONG); + configs.addFails(LONG, mainType); } } // Test float and double + var floatTypes = List.of(HALF_FLOAT, FLOAT, DOUBLE); { - TestConfigs configs = testConfigurations.computeIfAbsent("floats", k -> new TestConfigs(k, new LinkedHashSet<>())); - var floatTypes = List.of(DataType.FLOAT, DataType.DOUBLE); + TestConfigs configs = testConfigurations.computeIfAbsent("floats", TestConfigs::new); for (DataType mainType : floatTypes) { for (DataType lookupType : floatTypes) { configs.addPasses(mainType, lookupType); @@ -129,12 +141,53 @@ protected Collection> nodePlugins() { } } + // Tests for mixed-numerical types + { + TestConfigs configs = testConfigurations.computeIfAbsent("mixed-numerical", TestConfigs::new); + for (DataType mainType : integerTypes) { + for (DataType lookupType : floatTypes) { + // TODO: We should probably allow this, but we need to change the validation code in Join.java + configs.addFails(mainType, lookupType); + configs.addFails(lookupType, mainType); + } + } + } + + // Tests for all types where left and right are the same type + // DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, DATE_NANOS, IP, KEYWORD }; + DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, IP, KEYWORD }; + { + Collection existing = testConfigurations.values(); + TestConfigs configs = testConfigurations.computeIfAbsent("same", TestConfigs::new); + for (DataType type : all) { + if (existingIndex(existing, type, type)) { + // Skip existing configurations + continue; + } + configs.addPasses(type, type); + } + } + + // Tests for all other type combinations + { + Collection existing = testConfigurations.values(); + TestConfigs configs = testConfigurations.computeIfAbsent("others", TestConfigs::new); + for (DataType mainType : all) { + for (DataType lookupType : all) { + if (existingIndex(existing, mainType, lookupType)) { + // Skip existing configurations + continue; + } + configs.addFails(mainType, lookupType); + } + } + } // TODO: Add tests for mixed groups (should mostly fail, but might be some implicit casting to consider) // Make sure we have never added two configurations with the same index name Set knownTypes = new HashSet<>(); for (TestConfigs configs : testConfigurations.values()) { - for (TestConfig config : configs.configs()) { + for (TestConfig config : configs.configs.values()) { if (knownTypes.contains(config.indexName())) { throw new IllegalArgumentException("Duplicate index name: " + config.indexName()); } @@ -143,6 +196,11 @@ protected Collection> nodePlugins() { } } + private static boolean existingIndex(Collection existing, DataType mainType, DataType lookupType) { + String indexName = "index_" + mainType.esType() + "_" + lookupType.esType(); + return existing.stream().anyMatch(c -> c.exists(indexName)); + } + public void testLookupJoinStrings() { testLookupJoinTypes("strings"); } @@ -155,10 +213,22 @@ public void testLookupJoinFloats() { testLookupJoinTypes("floats"); } + public void testLookupJoinMixedNumerical() { + testLookupJoinTypes("mixed-numerical"); + } + + public void testLookupJoinSame() { + testLookupJoinTypes("same"); + } + + public void testLookupJoinOthers() { + testLookupJoinTypes("others"); + } + private void testLookupJoinTypes(String group) { initIndexes(group); initData(group); - for (TestConfig config : testConfigurations.get(group).configs()) { + for (TestConfig config : testConfigurations.get(group).configs.values()) { String query = String.format( Locale.ROOT, "FROM index | LOOKUP JOIN %s ON %s | KEEP other", @@ -172,7 +242,7 @@ private void testLookupJoinTypes(String group) { } private void initIndexes(String group) { - Set configs = testConfigurations.get(group).configs; + Collection configs = testConfigurations.get(group).configs.values(); // The main index will have many fields, one of each type to use in later type specific joins String mainFields = "{\n \"properties\" : {\n" + configs.stream().map(TestConfig::mainPropertySpec).distinct().collect(Collectors.joining(",\n ")) @@ -193,7 +263,7 @@ private void initIndexes(String group) { } private void initData(String group) { - Set configs = testConfigurations.get(group).configs; + Collection configs = testConfigurations.get(group).configs.values(); int docId = 0; for (TestConfig config : configs) { String doc = String.format(Locale.ROOT, """ @@ -224,29 +294,50 @@ private String mainPropertyFor(TestConfig config) { private static String sampleDataTextFor(DataType type) { return switch (type) { - case KEYWORD, TEXT -> "\"" + sampleDataFor(type) + "\""; + case KEYWORD, TEXT, DATETIME, DATE_NANOS, IP -> "\"" + sampleDataFor(type) + "\""; default -> String.valueOf(sampleDataFor(type)); }; } private static Object sampleDataFor(DataType type) { return switch (type) { + case BOOLEAN -> true; + case DATETIME, DATE_NANOS -> "2025-04-02T12:00:00.000Z"; + case IP -> "127.0.0.1"; case KEYWORD, TEXT -> "key"; case BYTE, SHORT, INTEGER -> 1; case LONG -> 1L; - case FLOAT, DOUBLE -> 1.0; + case HALF_FLOAT, FLOAT, DOUBLE -> 1.0; default -> throw new IllegalArgumentException("Unsupported type: " + type); }; } - private record TestConfigs(String group, Set configs) { + private static class TestConfigs { + final String group; + final Map configs; + + TestConfigs(String group) { + this.group = group; + this.configs = new LinkedHashMap<>(); + } + + private boolean exists(String indexName) { + return configs.containsKey(indexName); + } + + private void add(TestConfig config) { + if (configs.containsKey(config.indexName())) { + throw new IllegalArgumentException("Duplicate index name: " + config.indexName()); + } + configs.put(config.indexName(), config); + } private void addPasses(DataType mainType, DataType lookupType) { - configs.add(new TestConfigPasses(mainType, lookupType, true)); + add(new TestConfigPasses(mainType, lookupType, true)); } private void addEmptyResult(DataType mainType, DataType lookupType) { - configs.add(new TestConfigPasses(mainType, lookupType, false)); + add(new TestConfigPasses(mainType, lookupType, false)); } private void addFails(DataType mainType, DataType lookupType) { @@ -259,7 +350,7 @@ private void addFails(DataType mainType, DataType lookupType) { fieldName, lookupType.widenSmallNumeric() ); - configs.add( + add( new TestConfigFails<>( mainType, lookupType, @@ -272,7 +363,7 @@ private void addFails(DataType mainType, DataType lookupType) { private void addFailsText(DataType mainType, DataType lookupType) { String fieldName = "field_" + mainType.esType(); String errorMessage = String.format(Locale.ROOT, "JOIN with right field [%s] of type [TEXT] is not supported", fieldName); - configs.add( + add( new TestConfigFails<>( mainType, lookupType, @@ -283,7 +374,7 @@ private void addFailsText(DataType mainType, DataType lookupType) { } private void addFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) { - configs.add(new TestConfigFails<>(mainType, lookupType, exception, assertion)); + add(new TestConfigFails<>(mainType, lookupType, exception, assertion)); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index 15aa469f02007..49c7aa13a89b6 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -217,7 +217,7 @@ public void postAnalysisVerification(Failures failures) { for (int i = 0; i < config.leftFields().size(); i++) { Attribute leftField = config.leftFields().get(i); Attribute rightField = config.rightFields().get(i); - if (leftField.dataType().noText() != rightField.dataType().noText()) { + if (comparableTypes(leftField, rightField) == false) { failures.add( fail( leftField, @@ -236,4 +236,10 @@ public void postAnalysisVerification(Failures failures) { } } } + + private static boolean comparableTypes(Attribute left, Attribute right) { + // TODO: Consider allowing more valid types + // return left.dataType().noText() == right.dataType().noText() || left.dataType().isNumeric() == right.dataType().isNumeric(); + return left.dataType().noText() == right.dataType().noText(); + } } From bfc6ae3d729e23f5756bf6dff1e5e46476e19872 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Thu, 3 Apr 2025 09:44:33 +0200 Subject: [PATCH 06/11] Some fixes the javadocs --- .../xpack/esql/action/LookupJoinTypesIT.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index 13dfc7bba8977..fd71b7fd5ffb6 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -52,11 +52,16 @@ * For each pair of types being tested, it builds a main index called "index" containing a single document with as many fields as * types being tested on the left of the pair, and then creates that many other lookup indexes, each with a single document containing * exactly two fields: the field to join on, and a field to return. + * The assertion is that for valid combinations, the return result should exist, and for invalid combinations an exception should be thrown. + * If no exception is thrown, and no result is returned, our validation rules are not aligned with the internal behaviour (ie. a bug). + * Since the `LOOKUP JOIN` command requires the match field name to be the same between the main index and the lookup index, + * we will have field names that correctly represent the type of the field in the main index, but not the type of the field + * in the lookup index. This can be confusing, but it is important to remember that the field names are not the same as the types. * For example, if we are testing the pairs (double, double), (double, float), (float, double) and (float, float), * we will create the following indexes: *
- *
index_double_double: containing
- *
Index containing da single document with a field of type 'double' like: 
+ *     
index_double_double

+ *     
Index containing a single document with a field of type 'double' like: 
  *         {
  *             "field_double": 1.0,  // this is mapped as type 'double'
  *             "other": "value"
@@ -65,14 +70,14 @@
  *     
index_double_float

  *     
Index containing a single document with a field of type 'float' like: 
  *         {
- *             "field_double": 1.0,  // this is mapped as type 'float'
+ *             "field_double": 1.0,  // this is mapped as type 'float' (a float with the name of the main index field)
  *             "other": "value"
  *         }
  *     

  *     
index_float_double

  *     
Index containing a single document with a field of type 'double' like: 
  *         {
- *             "field_float": 1.0,  // this is mapped as type 'double'
+ *             "field_float": 1.0,  // this is mapped as type 'double' (a double with the name of the main index field)
  *             "other": "value"
  *         }
  *     

@@ -86,8 +91,8 @@
  *     
index

  *     
Index containing document like: 
  *         {
- *             "field_double": 1.0,
- *             "field_float": 1.0
+ *             "field_double": 1.0,  // this is mapped as type 'double'
+ *             "field_float": 1.0    // this is mapped as type 'float'
  *         }
  *     

  *
From 3b774a3d90643e665a8ea85b6e3cea8c101a2493 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Thu, 3 Apr 2025 09:47:39 +0200 Subject: [PATCH 07/11] Remove warnings --- .../xpack/esql/action/LookupJoinTypesIT.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index fd71b7fd5ffb6..7adb3b1c3dcda 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -117,8 +117,8 @@ protected Collection> nodePlugins() { TestConfigs configs = testConfigurations.computeIfAbsent("strings", TestConfigs::new); configs.addPasses(KEYWORD, KEYWORD); configs.addPasses(TEXT, KEYWORD); - configs.addFailsText(KEYWORD, TEXT); - configs.addFailsText(TEXT, TEXT); + configs.addFailsText(KEYWORD); + configs.addFailsText(TEXT); } // Test integer types @@ -187,7 +187,8 @@ protected Collection> nodePlugins() { } } } - // TODO: Add tests for mixed groups (should mostly fail, but might be some implicit casting to consider) + + // TODO: Add tests for more types, eg. unsigned_long, version, spatial types, date/temporal types. // Make sure we have never added two configurations with the same index name Set knownTypes = new HashSet<>(); @@ -365,13 +366,13 @@ private void addFails(DataType mainType, DataType lookupType) { ); } - private void addFailsText(DataType mainType, DataType lookupType) { + private void addFailsText(DataType mainType) { String fieldName = "field_" + mainType.esType(); String errorMessage = String.format(Locale.ROOT, "JOIN with right field [%s] of type [TEXT] is not supported", fieldName); add( new TestConfigFails<>( mainType, - lookupType, + DataType.TEXT, VerificationException.class, e -> assertThat(e.getMessage(), containsString(errorMessage)) ) @@ -450,6 +451,7 @@ public void testQuery(String query) { exception(), "Expected exception " + exception().getSimpleName() + " but no exception was thrown: " + query, () -> { + // noinspection EmptyTryBlock try (var ignored = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { // We use try-with-resources to ensure the request is closed if the exception is not thrown (less cluttered errors) } From fc1b1265936aedcb40f49b788b7b8b9898895c39 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Thu, 3 Apr 2025 10:10:11 +0200 Subject: [PATCH 08/11] Added tests for DateTime --- .../elasticsearch/xpack/esql/action/LookupJoinTypesIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index 7adb3b1c3dcda..f8c555cce7968 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -33,6 +33,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN; import static org.elasticsearch.xpack.esql.core.type.DataType.BYTE; +import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME; import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE; import static org.elasticsearch.xpack.esql.core.type.DataType.FLOAT; import static org.elasticsearch.xpack.esql.core.type.DataType.HALF_FLOAT; @@ -159,8 +160,7 @@ protected Collection> nodePlugins() { } // Tests for all types where left and right are the same type - // DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, DATE_NANOS, IP, KEYWORD }; - DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, IP, KEYWORD }; + DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, IP, KEYWORD }; { Collection existing = testConfigurations.values(); TestConfigs configs = testConfigurations.computeIfAbsent("same", TestConfigs::new); From 8708f9bb38bb2b54cf18a44864f81aef28a019ac Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 6 May 2025 18:32:46 +0200 Subject: [PATCH 09/11] Add negative tests for all unsupported types --- x-pack/plugin/esql/build.gradle | 1 + .../xpack/esql/action/LookupJoinTypesIT.java | 103 ++++++++++++++---- .../xpack/esql/plan/logical/join/Join.java | 49 ++++++++- 3 files changed, 133 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/esql/build.gradle b/x-pack/plugin/esql/build.gradle index ce58a84f75438..28c3e4d2b20cb 100644 --- a/x-pack/plugin/esql/build.gradle +++ b/x-pack/plugin/esql/build.gradle @@ -53,6 +53,7 @@ dependencies { testImplementation project(path: xpackModule('enrich')) testImplementation project(path: xpackModule('spatial')) testImplementation project(path: xpackModule('kql')) + testImplementation project(path: xpackModule('mapper-unsigned-long')) testImplementation project(path: ':modules:reindex') testImplementation project(path: ':modules:parent-join') diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index f8c555cce7968..383b037db61b4 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -15,8 +15,13 @@ import org.elasticsearch.xpack.core.esql.action.ColumnInfo; import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.core.type.DataType; +import org.elasticsearch.xpack.esql.plan.logical.join.Join; import org.elasticsearch.xpack.esql.plugin.EsqlPlugin; +import org.elasticsearch.xpack.spatial.SpatialPlugin; +import org.elasticsearch.xpack.unsignedlong.UnsignedLongMapperPlugin; +import org.elasticsearch.xpack.versionfield.VersionFieldPlugin; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -31,9 +36,11 @@ import static org.elasticsearch.test.ESIntegTestCase.Scope.SUITE; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.xpack.esql.core.type.DataType.AGGREGATE_METRIC_DOUBLE; import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN; import static org.elasticsearch.xpack.esql.core.type.DataType.BYTE; import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME; +import static org.elasticsearch.xpack.esql.core.type.DataType.DOC_DATA_TYPE; import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE; import static org.elasticsearch.xpack.esql.core.type.DataType.FLOAT; import static org.elasticsearch.xpack.esql.core.type.DataType.HALF_FLOAT; @@ -41,8 +48,10 @@ import static org.elasticsearch.xpack.esql.core.type.DataType.IP; import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD; import static org.elasticsearch.xpack.esql.core.type.DataType.LONG; +import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; import static org.elasticsearch.xpack.esql.core.type.DataType.SHORT; import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; +import static org.elasticsearch.xpack.esql.core.type.DataType.TSID_DATA_TYPE; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -108,7 +117,13 @@ @ClusterScope(scope = SUITE, numClientNodes = 1, numDataNodes = 1) public class LookupJoinTypesIT extends ESIntegTestCase { protected Collection> nodePlugins() { - return List.of(EsqlPlugin.class, MapperExtrasPlugin.class); + return List.of( + EsqlPlugin.class, + MapperExtrasPlugin.class, + VersionFieldPlugin.class, + UnsignedLongMapperPlugin.class, + SpatialPlugin.class + ); } private static final Map testConfigurations = new HashMap<>(); @@ -118,8 +133,7 @@ protected Collection> nodePlugins() { TestConfigs configs = testConfigurations.computeIfAbsent("strings", TestConfigs::new); configs.addPasses(KEYWORD, KEYWORD); configs.addPasses(TEXT, KEYWORD); - configs.addFailsText(KEYWORD); - configs.addFailsText(TEXT); + configs.addFailsUnsupported(KEYWORD, TEXT); } // Test integer types @@ -158,13 +172,37 @@ protected Collection> nodePlugins() { } } } + // Tests for all unsupported types + DataType[] unsupported = Join.UNSUPPORTED_TYPES; + { + Collection existing = testConfigurations.values(); + TestConfigs configs = testConfigurations.computeIfAbsent("unsupported", TestConfigs::new); + for (DataType type : unsupported) { + if (type == NULL + || type == DOC_DATA_TYPE + || type == TSID_DATA_TYPE + || type == AGGREGATE_METRIC_DOUBLE + || type.esType() == null + || type.isCounter() + || DataType.isRepresentable(type) == false) { + // Skip unmappable types, or types not supported in ES|QL in general + continue; + } + if (existingIndex(existing, type, type)) { + // Skip existing configurations + continue; + } + configs.addFailsUnsupported(type, type); + } + } // Tests for all types where left and right are the same type - DataType[] all = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, IP, KEYWORD }; + DataType[] supported = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, IP, KEYWORD }; { Collection existing = testConfigurations.values(); TestConfigs configs = testConfigurations.computeIfAbsent("same", TestConfigs::new); - for (DataType type : all) { + for (DataType type : supported) { + assertThat("Claiming supported for unsupported type: " + type, List.of(unsupported).contains(type), is(false)); if (existingIndex(existing, type, type)) { // Skip existing configurations continue; @@ -173,12 +211,28 @@ protected Collection> nodePlugins() { } } + // Assert that unsupported types are not in the supported list + for (DataType type : unsupported) { + assertThat("Claiming supported for unsupported type: " + type, List.of(supported).contains(type), is(false)); + } + + // Assert that unsupported+supported covers all types: + List missing = new ArrayList<>(); + for (DataType type : DataType.values()) { + boolean isUnsupported = List.of(unsupported).contains(type); + boolean isSupported = List.of(supported).contains(type); + if (isUnsupported == false && isSupported == false) { + missing.add(type); + } + } + assertThat(missing + " are not in the supported or unsupported list", missing.size(), is(0)); + // Tests for all other type combinations { Collection existing = testConfigurations.values(); TestConfigs configs = testConfigurations.computeIfAbsent("others", TestConfigs::new); - for (DataType mainType : all) { - for (DataType lookupType : all) { + for (DataType mainType : supported) { + for (DataType lookupType : supported) { if (existingIndex(existing, mainType, lookupType)) { // Skip existing configurations continue; @@ -188,8 +242,6 @@ protected Collection> nodePlugins() { } } - // TODO: Add tests for more types, eg. unsigned_long, version, spatial types, date/temporal types. - // Make sure we have never added two configurations with the same index name Set knownTypes = new HashSet<>(); for (TestConfigs configs : testConfigurations.values()) { @@ -227,6 +279,10 @@ public void testLookupJoinSame() { testLookupJoinTypes("same"); } + public void testLookupJoinUnsupported() { + testLookupJoinTypes("unsupported"); + } + public void testLookupJoinOthers() { testLookupJoinTypes("others"); } @@ -263,7 +319,7 @@ private void initIndexes(String group) { // Each lookup index will get a document with a field to join on, and a results field to get back (c) -> assertAcked( prepareCreate(c.indexName()).setSettings(settings.build()) - .setMapping(c.fieldName(), "type=" + c.lookupType().esType(), "other", "type=keyword") + .setMapping(c.fieldName(), "type=" + c.lookupType().esType().replace("cartesian_", ""), "other", "type=keyword") ) ); } @@ -299,10 +355,11 @@ private String mainPropertyFor(TestConfig config) { } private static String sampleDataTextFor(DataType type) { - return switch (type) { - case KEYWORD, TEXT, DATETIME, DATE_NANOS, IP -> "\"" + sampleDataFor(type) + "\""; - default -> String.valueOf(sampleDataFor(type)); - }; + var value = sampleDataFor(type); + if (value instanceof String) { + return "\"" + value + "\""; + } + return String.valueOf(value); } private static Object sampleDataFor(DataType type) { @@ -312,8 +369,11 @@ private static Object sampleDataFor(DataType type) { case IP -> "127.0.0.1"; case KEYWORD, TEXT -> "key"; case BYTE, SHORT, INTEGER -> 1; - case LONG -> 1L; + case LONG, UNSIGNED_LONG -> 1L; case HALF_FLOAT, FLOAT, DOUBLE -> 1.0; + case VERSION -> "1.2.19"; + case GEO_POINT, CARTESIAN_POINT -> "POINT (1.0 2.0)"; + case GEO_SHAPE, CARTESIAN_SHAPE -> "POLYGON ((0.0 0.0, 1.0 0.0, 1.0 1.0, 0.0 1.0, 0.0 0.0))"; default -> throw new IllegalArgumentException("Unsupported type: " + type); }; } @@ -366,13 +426,18 @@ private void addFails(DataType mainType, DataType lookupType) { ); } - private void addFailsText(DataType mainType) { + private void addFailsUnsupported(DataType mainType, DataType lookupType) { String fieldName = "field_" + mainType.esType(); - String errorMessage = String.format(Locale.ROOT, "JOIN with right field [%s] of type [TEXT] is not supported", fieldName); + String errorMessage = String.format( + Locale.ROOT, + "JOIN with right field [%s] of type [%s] is not supported", + fieldName, + lookupType + ); add( new TestConfigFails<>( mainType, - DataType.TEXT, + lookupType, VerificationException.class, e -> assertThat(e.getMessage(), containsString(errorMessage)) ) @@ -398,7 +463,7 @@ default String fieldName() { } default String mainPropertySpec() { - return "\"" + fieldName() + "\": { \"type\" : \"" + mainType().esType() + "\" }"; + return "\"" + fieldName() + "\": { \"type\" : \"" + mainType().esType().replaceAll("cartesian_", "") + "\" }"; } /** Make sure the left index has the expected fields and types */ diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index 1c980fda4e1ba..d88a75e8da694 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; +import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.plan.logical.BinaryPlan; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; @@ -25,16 +26,61 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Objects; import static org.elasticsearch.xpack.esql.common.Failure.fail; +import static org.elasticsearch.xpack.esql.core.type.DataType.AGGREGATE_METRIC_DOUBLE; +import static org.elasticsearch.xpack.esql.core.type.DataType.CARTESIAN_POINT; +import static org.elasticsearch.xpack.esql.core.type.DataType.CARTESIAN_SHAPE; +import static org.elasticsearch.xpack.esql.core.type.DataType.COUNTER_DOUBLE; +import static org.elasticsearch.xpack.esql.core.type.DataType.COUNTER_INTEGER; +import static org.elasticsearch.xpack.esql.core.type.DataType.COUNTER_LONG; +import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_NANOS; +import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_PERIOD; +import static org.elasticsearch.xpack.esql.core.type.DataType.DOC_DATA_TYPE; +import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT; +import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_SHAPE; +import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; +import static org.elasticsearch.xpack.esql.core.type.DataType.OBJECT; +import static org.elasticsearch.xpack.esql.core.type.DataType.PARTIAL_AGG; +import static org.elasticsearch.xpack.esql.core.type.DataType.SCALED_FLOAT; +import static org.elasticsearch.xpack.esql.core.type.DataType.SOURCE; import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; +import static org.elasticsearch.xpack.esql.core.type.DataType.TIME_DURATION; +import static org.elasticsearch.xpack.esql.core.type.DataType.TSID_DATA_TYPE; +import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG; +import static org.elasticsearch.xpack.esql.core.type.DataType.UNSUPPORTED; +import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.LEFT; public class Join extends BinaryPlan implements PostAnalysisVerificationAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Join", Join::new); + public static final DataType[] UNSUPPORTED_TYPES = { + TEXT, + VERSION, + UNSIGNED_LONG, + GEO_POINT, + GEO_SHAPE, + CARTESIAN_POINT, + CARTESIAN_SHAPE, + UNSUPPORTED, + NULL, + COUNTER_LONG, + COUNTER_INTEGER, + COUNTER_DOUBLE, + SCALED_FLOAT, + DATE_NANOS, + OBJECT, + SOURCE, + DATE_PERIOD, + TIME_DURATION, + DOC_DATA_TYPE, + TSID_DATA_TYPE, + PARTIAL_AGG, + AGGREGATE_METRIC_DOUBLE }; private final JoinConfig config; private List lazyOutput; @@ -229,7 +275,8 @@ public void postAnalysisVerification(Failures failures) { ) ); } - if (rightField.dataType().equals(TEXT)) { + // TODO: Add support for VERSION by implementing QueryList.versionTermQueryList similar to ipTermQueryList + if (Arrays.stream(UNSUPPORTED_TYPES).anyMatch(t -> rightField.dataType().equals(t))) { failures.add( fail(leftField, "JOIN with right field [{}] of type [{}] is not supported", rightField.name(), rightField.dataType()) ); From 8d53897abe152bc9c84d41be9ba53cff81872787 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 7 May 2025 10:38:07 +0200 Subject: [PATCH 10/11] Add support for scaled_float and some code-review changes --- .../xpack/esql/action/LookupJoinTypesIT.java | 61 ++++++++++++------- .../xpack/esql/plan/logical/join/Join.java | 2 - 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index 383b037db61b4..680b8ffa3380c 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -49,6 +49,7 @@ import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD; import static org.elasticsearch.xpack.esql.core.type.DataType.LONG; import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; +import static org.elasticsearch.xpack.esql.core.type.DataType.SCALED_FLOAT; import static org.elasticsearch.xpack.esql.core.type.DataType.SHORT; import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; import static org.elasticsearch.xpack.esql.core.type.DataType.TSID_DATA_TYPE; @@ -151,7 +152,7 @@ protected Collection> nodePlugins() { } // Test float and double - var floatTypes = List.of(HALF_FLOAT, FLOAT, DOUBLE); + var floatTypes = List.of(HALF_FLOAT, FLOAT, DOUBLE, SCALED_FLOAT); { TestConfigs configs = testConfigurations.computeIfAbsent("floats", TestConfigs::new); for (DataType mainType : floatTypes) { @@ -172,6 +173,7 @@ protected Collection> nodePlugins() { } } } + // Tests for all unsupported types DataType[] unsupported = Join.UNSUPPORTED_TYPES; { @@ -197,17 +199,16 @@ protected Collection> nodePlugins() { } // Tests for all types where left and right are the same type - DataType[] supported = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, IP, KEYWORD }; + DataType[] supported = { BOOLEAN, LONG, INTEGER, DOUBLE, SHORT, BYTE, FLOAT, HALF_FLOAT, DATETIME, IP, KEYWORD, SCALED_FLOAT }; { Collection existing = testConfigurations.values(); TestConfigs configs = testConfigurations.computeIfAbsent("same", TestConfigs::new); for (DataType type : supported) { assertThat("Claiming supported for unsupported type: " + type, List.of(unsupported).contains(type), is(false)); - if (existingIndex(existing, type, type)) { - // Skip existing configurations - continue; + if (existingIndex(existing, type, type) == false) { + // Only add the configuration if it doesn't already exist + configs.addPasses(type, type); } - configs.addPasses(type, type); } } @@ -233,11 +234,10 @@ protected Collection> nodePlugins() { TestConfigs configs = testConfigurations.computeIfAbsent("others", TestConfigs::new); for (DataType mainType : supported) { for (DataType lookupType : supported) { - if (existingIndex(existing, mainType, lookupType)) { - // Skip existing configurations - continue; + if (existingIndex(existing, mainType, lookupType) == false) { + // Only add the configuration if it doesn't already exist + configs.addFails(mainType, lookupType); } - configs.addFails(mainType, lookupType); } } } @@ -305,10 +305,13 @@ private void testLookupJoinTypes(String group) { private void initIndexes(String group) { Collection configs = testConfigurations.get(group).configs.values(); + String propertyPrefix = "{\n \"properties\" : {\n"; + String propertySuffix = " }\n}\n"; // The main index will have many fields, one of each type to use in later type specific joins - String mainFields = "{\n \"properties\" : {\n" - + configs.stream().map(TestConfig::mainPropertySpec).distinct().collect(Collectors.joining(",\n ")) - + " }\n}\n"; + String mainFields = propertyPrefix + configs.stream() + .map(TestConfig::mainPropertySpec) + .distinct() + .collect(Collectors.joining(",\n ")) + propertySuffix; assertAcked(prepareCreate("index").setMapping(mainFields)); Settings.Builder settings = Settings.builder() @@ -319,7 +322,7 @@ private void initIndexes(String group) { // Each lookup index will get a document with a field to join on, and a results field to get back (c) -> assertAcked( prepareCreate(c.indexName()).setSettings(settings.build()) - .setMapping(c.fieldName(), "type=" + c.lookupType().esType().replace("cartesian_", ""), "other", "type=keyword") + .setMapping(propertyPrefix + c.lookupPropertySpec() + propertySuffix) ) ); } @@ -362,6 +365,8 @@ private static String sampleDataTextFor(DataType type) { return String.valueOf(value); } + private static final double SCALING_FACTOR = 1.0; + private static Object sampleDataFor(DataType type) { return switch (type) { case BOOLEAN -> true; @@ -371,6 +376,7 @@ private static Object sampleDataFor(DataType type) { case BYTE, SHORT, INTEGER -> 1; case LONG, UNSIGNED_LONG -> 1L; case HALF_FLOAT, FLOAT, DOUBLE -> 1.0; + case SCALED_FLOAT -> SCALING_FACTOR; case VERSION -> "1.2.19"; case GEO_POINT, CARTESIAN_POINT -> "POINT (1.0 2.0)"; case GEO_SHAPE, CARTESIAN_SHAPE -> "POLYGON ((0.0 0.0, 1.0 0.0, 1.0 1.0, 0.0 1.0, 0.0 0.0))"; @@ -402,10 +408,6 @@ private void addPasses(DataType mainType, DataType lookupType) { add(new TestConfigPasses(mainType, lookupType, true)); } - private void addEmptyResult(DataType mainType, DataType lookupType) { - add(new TestConfigPasses(mainType, lookupType, false)); - } - private void addFails(DataType mainType, DataType lookupType) { String fieldName = "field_" + mainType.esType(); String errorMessage = String.format( @@ -443,10 +445,6 @@ private void addFailsUnsupported(DataType mainType, DataType lookupType) { ) ); } - - private void addFails(DataType mainType, DataType lookupType, Class exception, Consumer assertion) { - add(new TestConfigFails<>(mainType, lookupType, exception, assertion)); - } } interface TestConfig { @@ -463,7 +461,11 @@ default String fieldName() { } default String mainPropertySpec() { - return "\"" + fieldName() + "\": { \"type\" : \"" + mainType().esType().replaceAll("cartesian_", "") + "\" }"; + return propertySpecFor(fieldName(), mainType(), ""); + } + + default String lookupPropertySpec() { + return propertySpecFor(fieldName(), lookupType(), ", \"other\": { \"type\" : \"keyword\" }"); } /** Make sure the left index has the expected fields and types */ @@ -479,6 +481,19 @@ default void validateLookupIndex() { void testQuery(String query); } + private static String propertySpecFor(String fieldName, DataType type, String extra) { + if (type == SCALED_FLOAT) { + return String.format( + Locale.ROOT, + "\"%s\": { \"type\" : \"%s\", \"scaling_factor\": %f }", + fieldName, + type.esType(), + SCALING_FACTOR + ) + extra; + } + return String.format(Locale.ROOT, "\"%s\": { \"type\" : \"%s\" }", fieldName, type.esType().replaceAll("cartesian_", "")) + extra; + } + private static void validateIndex(String indexName, String fieldName, Object expectedValue) { String query = String.format(Locale.ROOT, "FROM %s | KEEP %s", indexName, fieldName); try (var response = EsqlQueryRequestBuilder.newRequestBuilder(client()).query(query).get()) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index d88a75e8da694..8e887d1e92c25 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -45,7 +45,6 @@ import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; import static org.elasticsearch.xpack.esql.core.type.DataType.OBJECT; import static org.elasticsearch.xpack.esql.core.type.DataType.PARTIAL_AGG; -import static org.elasticsearch.xpack.esql.core.type.DataType.SCALED_FLOAT; import static org.elasticsearch.xpack.esql.core.type.DataType.SOURCE; import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; import static org.elasticsearch.xpack.esql.core.type.DataType.TIME_DURATION; @@ -71,7 +70,6 @@ public class Join extends BinaryPlan implements PostAnalysisVerificationAware, S COUNTER_LONG, COUNTER_INTEGER, COUNTER_DOUBLE, - SCALED_FLOAT, DATE_NANOS, OBJECT, SOURCE, From 820d890c92dd538cca143388114d652c62338c7d Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 7 May 2025 11:35:29 +0200 Subject: [PATCH 11/11] Fix mistake in scaled_float value and test non-unity scaling factor --- .../elasticsearch/xpack/esql/action/LookupJoinTypesIT.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java index 680b8ffa3380c..52c41e4056a8e 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java @@ -365,7 +365,7 @@ private static String sampleDataTextFor(DataType type) { return String.valueOf(value); } - private static final double SCALING_FACTOR = 1.0; + private static final double SCALING_FACTOR = 10.0; private static Object sampleDataFor(DataType type) { return switch (type) { @@ -375,8 +375,7 @@ private static Object sampleDataFor(DataType type) { case KEYWORD, TEXT -> "key"; case BYTE, SHORT, INTEGER -> 1; case LONG, UNSIGNED_LONG -> 1L; - case HALF_FLOAT, FLOAT, DOUBLE -> 1.0; - case SCALED_FLOAT -> SCALING_FACTOR; + case HALF_FLOAT, FLOAT, DOUBLE, SCALED_FLOAT -> 1.0; case VERSION -> "1.2.19"; case GEO_POINT, CARTESIAN_POINT -> "POINT (1.0 2.0)"; case GEO_SHAPE, CARTESIAN_SHAPE -> "POLYGON ((0.0 0.0, 1.0 0.0, 1.0 1.0, 0.0 1.0, 0.0 0.0))";