Integration tests for LOOKUP JOIN over wider range of data types #126150
Conversation
craigtaverner
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
…icsearch into lookup_join_types_it
There was a problem hiding this comment.
Nice! This is already looking great!
Before we merge, I wonder if we could simplify the test setup and make the test class a tad easier to follow. Although this is already a great value-add and is safe to merge.
I agree that as a next step, we should expand the number of different types covered.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java
Show resolved
Hide resolved
...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java
Show resolved
Hide resolved
| * <dt>index_double_float</dt> | ||
| * <dd>Index containing a single document with a field of type 'float' like: <pre> | ||
| * { | ||
| * "field_double": 1.0, // this is mapped as type 'float' (a float with the name of the main index field) |
There was a problem hiding this comment.
I think we may cut down on the number of created indices (maybe speeding up the test a little) and make this simpler to debug if we always name the fields by the type. We can just perform a RENAME in the query. Prefixing the field names with main_field or lookup_field would also guarantee that we don't accidentally shadow while renaming, and it will make the javadoc more self explanatory.
In fact, the test index creation could be simplified to 2 indices: one main index with every type we currently consider, and one lookup index with every type we currently consider. This means index creation can be done entirely during the setup of the test suite, rather than in each test.
This concern may be more important later, though, when we generalize the join condition so that multiple fields (a composite key) can be used. Then we probably also want to expand this test to use different combinations of types.
There was a problem hiding this comment.
I love the idea, but think we can re-visit this later.
| * For example, if we are testing the pairs (double, double), (double, float), (float, double) and (float, float), | ||
| * we will create the following indexes: | ||
| * <dl> | ||
| * <dt>index_double_double</dt> |
There was a problem hiding this comment.
nit: it's unclear whether this is the main or the lookup index when reading this javadoc.
| { | ||
| TestConfigs configs = testConfigurations.computeIfAbsent("strings", TestConfigs::new); | ||
| configs.addPasses(KEYWORD, KEYWORD); | ||
| configs.addPasses(TEXT, KEYWORD); |
There was a problem hiding this comment.
Is it possible to have a TEXT field without a .keyword subfield? I think we fail in this situation. Could you maybe double check and add a test if that's possible?
There was a problem hiding this comment.
We always pass with TEXT on the left because we will read from source for the main index if no KEYWORD subfield exists. Likewise we always fail with TEXT on the right, even if a KEYWORD subfield exists because we have not coded a special case for that (yet).
| assertAcked(prepareCreate("index").setMapping(mainFields)); | ||
|
|
||
| Settings.Builder settings = Settings.builder() | ||
| .put("index.number_of_shards", 1) |
There was a problem hiding this comment.
This is theoretically redundant; we used to have to set this manually, but by now I think just setting the index mode to lookup should suffice. I think @idegtiarenko double checked this the other day.
| private void addEmptyResult(DataType mainType, DataType lookupType) { | ||
| add(new TestConfigPasses(mainType, lookupType, false)); | ||
| } |
There was a problem hiding this comment.
This appears unused?
Similarly, the boolean arg in the TestConfigPasses could always be true, then, no?
| private <E extends Exception> void addFails(DataType mainType, DataType lookupType, Class<E> exception, Consumer<E> assertion) { | ||
| add(new TestConfigFails<>(mainType, lookupType, exception, assertion)); | ||
| } |
There was a problem hiding this comment.
I think this was meant to be used in the other addFails methods above? Currently, I think it is unused.
| } | ||
| } | ||
|
|
||
| // TODO: Add tests for more types, eg. unsigned_long, version, spatial types, date/temporal 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 |
There was a problem hiding this comment.
++, the behavior should be exactly as if we evaluated an ==, I think.
There was a problem hiding this comment.
LGTM.
The setup is thorough, but I'd maybe echo Alex's note on complexity: setting up indices does take time and while this test itself takes under 10s (w/o gradle setup), it could probably be sped-up with just two indices and renames (which would be skipped on same type in both indices, so the RENAME itself should not introduce a blind corner).
This complexity surfaces a bit also in the need to test the test (validateIndex()).
| Collection<TestConfigs> existing = testConfigurations.values(); | ||
| TestConfigs configs = testConfigurations.computeIfAbsent("same", TestConfigs::new); | ||
| for (DataType type : all) { | ||
| if (existingIndex(existing, type, type)) { |
There was a problem hiding this comment.
Supernit: test against false and skip the continue for slightly better legibility.
Here and below.
|
Let's get this PR merged in since it's been sitting here for a while - and move the nanos and potentially speed-up (if it can't be addressed shortly) in a follow-up. |
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.
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 JOINcommand 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:
{ "field_double": 1.0, // this is mapped as type 'double' "other": "value" }{ "field_double": 1.0, // this is mapped as type 'float' (a float with the name of the main index field) "other": "value" }{ "field_float": 1.0, // this is mapped as type 'double' (a double with the name of the main index field) "other": "value" }{ "field_float": 1.0, // this is mapped as type 'float' "other": "value" }{ "field_double": 1.0, // this is mapped as type 'double' "field_float": 1.0 // this is mapped as type 'float' }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:
And assert that the result exists and is equal to "value".
Checklist:
alllist by depending on more modules (eg. unsigned_long and spatial types)unsigned_long(fromx-pack:plugin:mapper-unsigned-long)version(not sure where to get this, as it is defined in server, but ES through missing mapper exception)cartesian_prefix and module access for shapesdatetime(just to add tests)date_nanos(currently tests fail as no results are found even with identical fields)One thing to consider changing here is allowing float and integer types to be used together. Right now the only thing blocking this is the validation code. The join actually succeeds if we remove the validation.