SQL: Test and fix the NULL handling of the String functions (#68379)

Fixed the inconsistencies regarding NULL argument handling.
NULL literal vs NULL field value as function arguments in some case
resulted in different function return values.

Functions should return with the same value no matter if the argument(s)
came from a field or from a literal.

The introduced integration test tests if function calls with same
argument values (regardless of literal/field) will return with the
same output (also checks if newly added functions are added to the
testcases).

Fixed the following functions:
* Insert: NULL start, length and replacement arguments (as fields) also
result in NULL return value instead of returning the input.
* Locate: NULL pattern results in NULL return value, NULL optional start
argument handled the same as missing start argument
* Replace: NULL pattern and replacement results in NULL instead of
returning the input
* Substring: NULL start or length results in NULL instead of returning
the input

Fixes #58907

Author: Andras Palinkas

docs/reference/sql/functions/string.asciidoc

@@ -107,7 +107,7 @@ CONCAT(
 
 *Output*: string
 
-*Description*: Returns a character string that is the result of concatenating `string_exp1` to `string_exp2`. If one of the string is `NULL`, the other string will be returned.
+*Description*: Returns a character string that is the result of concatenating `string_exp1` to `string_exp2`. `NULL` input strings are treated as empty strings.
 
 [source, sql]
 --------------------------------------------------
@@ -228,7 +228,7 @@ LOCATE(
 
 *Output*: integer
 
-*Description*: Returns the starting position of the first occurrence of `pattern` within `source`. The search for the first occurrence of `pattern` begins with the first character position in `source` unless the optional argument, `start`, is specified. If `start` is specified, the search begins with the character position indicated by the value of `start`. The first character position in `source` is indicated by the value 1. If `pattern` is not found within `source`, the value 0 is returned.
+*Description*: Returns the starting position of the first occurrence of `pattern` within `source`. The optional `start` specifies the character position to start the search with. The first character position in `source` is indicated by the value 1. Not specifying `start` or specifying it as `NULL`, any negative value, 0 or 1, starts the search at the first character position. If `pattern` is not found within `source`, the value 0 is returned.
 
 [source, sql]
 --------------------------------------------------

x-pack/plugin/sql/qa/server/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

@@ -0,0 +1,283 @@
+/*
+ * 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;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import static java.lang.String.join;
+import static java.util.Arrays.asList;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static org.elasticsearch.common.collect.Tuple.tuple;
+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>
+ */
+public class ConsistentFunctionArgHandlingIT extends JdbcIntegrationTestCase {
+
+    private static final List<Fn> FUNCTION_CALLS_TO_TEST = asList(
+        new Fn("ASCII", "foobar"),
+        new Fn("BIT_LENGTH", "foobar"),
+        new Fn("CHAR", 66),
+        new Fn("CHAR_LENGTH", "foobar").aliases("CHARACTER_LENGTH"),
+        new Fn("CONCAT", "foo", "bar"),
+        new Fn("INSERT", "foobar", 2, 3, "replacement"),
+        new Fn("LCASE", "STRING"),
+        new Fn("LEFT", "foobar", 3),
+        new Fn("LENGTH", "foobar"),
+        new Fn("LOCATE", "ob", "foobar", 1),
+        new Fn("LTRIM", "   foobar"),
+        new Fn("OCTET_LENGTH", "foobar"),
+        new Fn("POSITION", "foobar", "ob"),
+        new Fn("REPEAT", "foobar", 10),
+        new Fn("REPLACE", "foo", "o", "bar"),
+        new Fn("RIGHT", "foobar", 3),
+        new Fn("RTRIM", "foobar   "),
+        new Fn("SPACE", 5),
+        new Fn("STARTS_WITH", "foobar", "foo"),
+        new Fn("SUBSTRING", "foobar", 1, 2),
+        new Fn("TRIM", "  foobar   "),
+        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(new Argument(a));
+            }
+        }
+
+        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;
+
+        private Argument(Object exampleValue, Source... acceptedSources) {
+            this.exampleValue = exampleValue;
+            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
+        // so the field mapping is automatically set up
+        final String functionName = fn.name;
+        final String indexName = "test";
+        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<>();
+                for (int argIndex = 0; argIndex < argValues.size(); argIndex++) {
+                    final Object argValue = argValues.get(argIndex);
+                    final Source argSource = argSources.get(argIndex);
+                    final String valueAsLiteral = asLiteralInQuery(argValue);
+                    switch (argSource) {
+                        case LITERAL:
+                            functionCallArgs.add(valueAsLiteral);
+                            break;
+                        case FIELD:
+                            final String argFieldName = (argValue == null ? nullArgPrefix : argPrefix) + (argIndex + 1);
+                            functionCallArgs.add(argFieldName);
+                            break;
+                    }
+                    functionCallArgsForAssert.add(valueAsLiteral + "{" + argSource.name().charAt(0) + "}");
+                }
+
+                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());
+
+                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);
+                }
+            });
+        });
+    }
+
+    private void indexTestDocForFunction(String functionName, String indexName, String argPrefix, String nullArgPrefix, String testDocId)
+        throws IOException {
+        Map<String, Object> testDoc = new LinkedHashMap<>();
+        testDoc.put("docId", testDocId);
+        int idx = 0;
+        for (Argument arg : fn.arguments) {
+            idx += 1;
+            testDoc.put(argPrefix + idx, arg.exampleValue);
+            // first set the same value, so the mapping is populated for the null columns
+            testDoc.put(nullArgPrefix + idx, arg.exampleValue);
+        }
+        index(indexName, functionName, body -> body.mapContents(testDoc));
+
+        // zero out the fields to be used as nulls
+        for (idx = 1; idx <= fn.arguments.size(); idx++) {
+            testDoc.put(nullArgPrefix + idx, null);
+        }
+        index(indexName, functionName, body -> body.mapContents(testDoc));
+    }
+
+    private void checkScalarFunctionCoverage() throws Exception {
+        ResultSet resultSet = esJdbc().createStatement().executeQuery("SHOW FUNCTIONS");
+        Set<String> functions = new LinkedHashSet<>();
+        while (resultSet.next()) {
+            String name = resultSet.getString(1);
+            String fnType = resultSet.getString(2);
+            if ("SCALAR".equals(fnType)) {
+                functions.add(name);
+            }
+        }
+        for (Fn fn : FUNCTION_CALLS_TO_TEST) {
+            functions.remove(fn.name);
+            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) {
+        String argInQuery;
+        if (argValue == null) {
+            argInQuery = "NULL";
+        } else {
+            argInQuery = String.valueOf(argValue);
+            if (argValue instanceof String) {
+                argInQuery = "'" + argInQuery + "'";
+            }
+        }
+        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;
+        }
+        iterateAllPermutations(possibleValuesPerItem.subList(1, possibleValuesPerItem.size()), onePermutationOfTail -> {
+            for (T option : possibleValuesPerItem.get(0)) {
+                ArrayList<T> onePermutation = new ArrayList<>();
+                onePermutation.add(option);
+                onePermutation.addAll(onePermutationOfTail);
+                consumer.accept(onePermutation);
+            }
+        });
+    }
+
+}