SQL: Test and fix the NULL handling of the String functions #68379
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,21 @@ | ||
| /* | ||
| * 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.sql.qa.single_node; | ||
|
|
||
| import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; | ||
| import org.elasticsearch.common.CheckedConsumer; | ||
| import org.elasticsearch.common.UUIDs; | ||
| import org.elasticsearch.common.collect.Tuple; | ||
| import org.elasticsearch.xpack.sql.qa.jdbc.JdbcIntegrationTestCase; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.sql.ResultSet; | ||
| import java.util.ArrayList; | ||
| import java.util.LinkedHashMap; | ||
|
|
@@ -23,7 +25,6 @@ | |
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
||
| import static java.lang.String.join; | ||
| import static java.util.Arrays.asList; | ||
|
|
@@ -33,16 +34,16 @@ | |
| import static org.hamcrest.collection.IsEmptyCollection.empty; | ||
|
|
||
| /** | ||
| * <p>This test was introduced because of the inconsistencies regarding NULL argument handling. NULL literal vs NULL field | ||
| * value as function arguments in some case result in different function return values.</p> | ||
| * | ||
| * <p>Functions should return with the same value no matter if the argument(s) came from a field or from a literal.</p> | ||
| * | ||
| * <p>The test class based on the example function calls (and argument specifications) generates all the | ||
| * permutations of the function arguments (4 options per argument: value/NULL as literal/field) and tests that the | ||
| * function calls with the same argument values provide the same result regardless of the source (literal, field) | ||
| * of the arguments.</p> | ||
| * | ||
| * <p>To ignore any of the tests, add an .ignore() method call after the Fn ctors in the FUNCTION_CALLS_TO_TEST list below, like: | ||
| * <code> new Fn("ASCII", "foobar").ignore()</code></p> | ||
| */ | ||
|
|
@@ -73,51 +74,51 @@ public class ConsistentFunctionArgHandlingIT extends JdbcIntegrationTestCase { | |
| new Fn("UCASE", "foobar") | ||
| ); | ||
|
|
||
| private static final List<String> NON_TESTED_FUNCTIONS; | ||
| static { | ||
| try { | ||
| Class<?> c = ConsistentFunctionArgHandlingIT.class; | ||
| NON_TESTED_FUNCTIONS = Files.readAllLines(Path.of(c.getResource(c.getSimpleName() + "-non-tested-functions.txt").toURI())); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
|
|
||
| private enum Source { | ||
| FIELD, | ||
| LITERAL | ||
| } | ||
|
|
||
| private static class Fn { | ||
| private final String name; | ||
| private final List<Argument> arguments; | ||
| private List<String> aliases = new ArrayList<>(); | ||
| private boolean ignored = false; | ||
|
|
||
| private Fn(String name, Object... arguments) { | ||
| this.name = name; | ||
| this.arguments = new ArrayList<>(); | ||
| for (Object a : arguments) { | ||
| this.arguments.add(Argument.class.isAssignableFrom(a.getClass()) ? (Argument) a : new Argument(a)); | ||
|
There was a problem hiding this comment. When can there already be an Argument sent as a parameter to this constructor? I am only seeing non-Argument instances being used.... There was a problem hiding this comment. Previously I used for one of the cases ( There was a problem hiding this comment. @palesz I would suggest removing it and use it when needed. Someone looking at the code gets confused about a functionality that's not actually there. |
||
| } | ||
| } | ||
|
|
||
| public Fn aliases(String... aliases) { | ||
| this.aliases = asList(aliases); | ||
| return this; | ||
| } | ||
|
|
||
| public Fn ignore() { | ||
| this.ignored = true; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return name + "(" + arguments.stream().map(a -> String.valueOf(a.exampleValue)).collect(joining(", ")) + ")"; | ||
| } | ||
| } | ||
|
|
||
| private static class Argument { | ||
| private final Object exampleValue; | ||
| private final Source[] acceptedSources; | ||
|
|
@@ -127,27 +128,27 @@ private Argument(Object exampleValue, Source... acceptedSources) { | |
| this.acceptedSources = acceptedSources.length == 0 ? Source.values() : acceptedSources; | ||
| } | ||
| } | ||
|
|
||
| @ParametersFactory | ||
| public static Iterable<Object[]> testFactory() { | ||
| List<Object[]> tests = new ArrayList<>(); | ||
| tests.add(new Object[] { null }); | ||
| FUNCTION_CALLS_TO_TEST.forEach(f -> tests.add(new Object[] { f })); | ||
| return tests; | ||
| } | ||
|
|
||
| private final Fn fn; | ||
|
|
||
| public ConsistentFunctionArgHandlingIT(Fn fn) { | ||
| this.fn = fn; | ||
| } | ||
|
|
||
| public void test() throws Exception { | ||
| if (fn == null) { | ||
| checkScalarFunctionCoverage(); | ||
| return; | ||
| } | ||
|
|
||
| assumeFalse("Ignored", fn.ignored); | ||
|
|
||
| // create a record for the function, where all the example (non-null) argument values are stored in fields | ||
|
|
@@ -157,20 +158,20 @@ public void test() throws Exception { | |
| final String argPrefix = "arg_" + functionName + "_"; | ||
| final String nullArgPrefix = "arg_null_" + functionName + "_"; | ||
| final String testDocId = functionName + "_" + UUIDs.base64UUID(); | ||
|
|
||
| indexTestDocForFunction(functionName, indexName, argPrefix, nullArgPrefix, testDocId); | ||
|
|
||
| List<List<Object>> possibleValuesPerArguments = fn.arguments.stream().map(a -> asList(a.exampleValue, null)).collect(toList()); | ||
| List<List<Source>> acceptedSourcesPerArguments = fn.arguments.stream().map(a -> asList(a.acceptedSources)).collect(toList()); | ||
|
|
||
| iterateAllPermutations(possibleValuesPerArguments, argValues -> { | ||
| // we only want to check the function calls that have at least a single NULL argument | ||
| if (argValues.stream().noneMatch(Objects::isNull)) { | ||
| return; | ||
| } | ||
|
|
||
| List<Tuple<String, Object>> results = new ArrayList<>(); | ||
|
|
||
| iterateAllPermutations(acceptedSourcesPerArguments, argSources -> { | ||
| List<String> functionCallArgs = new ArrayList<>(); | ||
| List<String> functionCallArgsForAssert = new ArrayList<>(); | ||
|
|
@@ -193,18 +194,17 @@ public void test() throws Exception { | |
| final String functionCall = functionName + "(" + join(", ", functionCallArgs) + ")"; | ||
| final String query = "SELECT " + functionCall + " FROM " + indexName + " WHERE docId = '" + testDocId + "'"; | ||
| ResultSet retVal = esJdbc().createStatement().executeQuery(query); | ||
|
|
||
| assertTrue(retVal.next()); | ||
| results.add(tuple(functionName + "(" + join(", ", functionCallArgsForAssert) + ")", retVal.getObject(1))); | ||
| // only a single row should be returned | ||
| assertFalse(retVal.next()); | ||
|
|
||
|
There was a problem hiding this comment. Why did you remove this and now go over the stream components twice? There was a problem hiding this comment. Sadly streams can only be iterated once, during the second iteration it gave There was a problem hiding this comment. Why did the tests pass then? There was a problem hiding this comment. Because the second iteration only happens if the test finds an issue (to create the detailed message for error). |
||
| if (results.stream().map(Tuple::v2).distinct().count() > 1) { | ||
| int maxResultWidth = results.stream().map(Tuple::v2).mapToInt(o -> asLiteralInQuery(o).length()).max().orElse(20); | ||
| String resultsAsString = results.stream() | ||
| .map(r -> String.format(Locale.ROOT, "%2$-" + maxResultWidth + "s // %1$s", r.v1(), asLiteralInQuery(r.v2()))) | ||
| .collect(joining("\n")); | ||
| fail("The result of the last call differs from the other calls:\n" + resultsAsString); | ||
| } | ||
| }); | ||
|
|
@@ -246,9 +246,8 @@ private void checkScalarFunctionCoverage() throws Exception { | |
| functions.removeAll(fn.aliases); | ||
| } | ||
| functions.removeAll(NON_TESTED_FUNCTIONS); | ||
|
|
||
| assertThat("Some functions are not covered by this test", functions, empty()); | ||
| } | ||
|
|
||
| private static String asLiteralInQuery(Object argValue) { | ||
|
|
@@ -264,9 +263,9 @@ private static String asLiteralInQuery(Object argValue) { | |
| return argInQuery; | ||
| } | ||
|
|
||
| private static <T> void iterateAllPermutations(List<List<T>> possibleValuesPerItem, CheckedConsumer<List<T>, Exception> consumer) | ||
| throws Exception { | ||
|
|
||
| if (possibleValuesPerItem.isEmpty()) { | ||
| consumer.accept(new ArrayList<>()); | ||
| return; | ||
|
|
@@ -280,6 +279,5 @@ private static <T> void iterateAllPermutations(List<List<T>> possibleValuesPerIt | |
| } | ||
| }); | ||
| } | ||
|
|
||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| CURDATE | ||
| CURRENT_DATE | ||
| CURRENT_TIME | ||
| CURRENT_TIMESTAMP | ||
| CURTIME | ||
| DATEADD | ||
| DATEDIFF | ||
| DATEPART | ||
| DATETIME_FORMAT | ||
| DATETIME_PARSE | ||
| DATETRUNC | ||
| DATE_ADD | ||
| DATE_DIFF | ||
| DATE_PARSE | ||
| DATE_PART | ||
| DATE_TRUNC | ||
| DAY | ||
| DAYNAME | ||
| DAYOFMONTH | ||
| DAYOFWEEK | ||
| DAYOFYEAR | ||
| DAY_NAME | ||
| DAY_OF_MONTH | ||
| DAY_OF_WEEK | ||
| DAY_OF_YEAR | ||
| DOM | ||
| DOW | ||
| DOY | ||
| FORMAT | ||
| HOUR | ||
| HOUR_OF_DAY | ||
| IDOW | ||
| ISODAYOFWEEK | ||
| ISODOW | ||
| ISOWEEK | ||
| ISOWEEKOFYEAR | ||
| ISO_DAY_OF_WEEK | ||
| ISO_WEEK_OF_YEAR | ||
| IW | ||
| IWOY | ||
| MINUTE | ||
| MINUTE_OF_DAY | ||
| MINUTE_OF_HOUR | ||
| MONTH | ||
| MONTHNAME | ||
| MONTH_NAME | ||
| MONTH_OF_YEAR | ||
| NOW | ||
| QUARTER | ||
| SECOND | ||
| SECOND_OF_MINUTE | ||
| TIMESTAMPADD | ||
| TIMESTAMPDIFF | ||
| TIMESTAMP_ADD | ||
| TIMESTAMP_DIFF | ||
| TIME_PARSE | ||
| TODAY | ||
| TO_CHAR | ||
| WEEK | ||
| WEEK_OF_YEAR | ||
| YEAR | ||
| ABS | ||
| ACOS | ||
| ASIN | ||
| ATAN | ||
| ATAN2 | ||
| CBRT | ||
| CEIL | ||
| CEILING | ||
| COS | ||
| COSH | ||
| COT | ||
| DEGREES | ||
| E | ||
| EXP | ||
| EXPM1 | ||
| FLOOR | ||
| LOG | ||
| LOG10 | ||
| MOD | ||
| PI | ||
| POWER | ||
| RADIANS | ||
| RAND | ||
| RANDOM | ||
| ROUND | ||
| SIGN | ||
| SIGNUM | ||
| SIN | ||
| SINH | ||
| SQRT | ||
| TAN | ||
| TRUNC | ||
| TRUNCATE | ||
| CAST | ||
| CONVERT | ||
| DATABASE | ||
| USER | ||
| ST_ASTEXT | ||
| ST_ASWKT | ||
| ST_DISTANCE | ||
| ST_GEOMETRYTYPE | ||
| ST_GEOMFROMTEXT | ||
| ST_WKTTOSQL | ||
| ST_X | ||
| ST_Y | ||
| ST_Z |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if it's possible get all the functions from FunctionRegistry:
Something like: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java#L2199
Can also be done at a later point and not as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlFunctionRegistryis not available in integration tests (can add it as a dependency, but I don't think we really need it). I get the list of the functions fromSHOW FUNCTIONSsee here, that achieves pretty much the same thatSqlFunctionRegistrywould (for more see the last paragraph here). The purpose of theNON_TESTED_FUNCTIONSlist is to identify functions added after writing this test class which likely we should think about adding to the tested function list / non tested function list (one-liner). Newly added =SHOW FUNCTIONS \ FUNCTIONS_CALLS \ NON_TESTED_FUNCTIONSshould be an empty set. TheNON_TESTED_FUNCTIONSlist should shrink later once the datetime, ... functions are also added to the tested function list (separate PR).