Browse Source

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
Andras Palinkas 4 years ago
parent
commit
a3dbdae2ef
13 changed files with 428 additions and 46 deletions
  1. 2 2
      docs/reference/sql/functions/string.asciidoc
  2. 283 0
      x-pack/plugin/sql/qa/server/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java
  3. 107 0
      x-pack/plugin/sql/qa/server/single-node/src/test/resources/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT-non-tested-functions.txt
  4. 1 7
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java
  5. 9 3
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Locate.java
  6. 5 8
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/LocateFunctionProcessor.java
  7. 2 5
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/ReplaceFunctionProcessor.java
  8. 1 1
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/SubstringFunctionProcessor.java
  9. 1 1
      x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
  10. 3 5
      x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertProcessorTests.java
  11. 10 6
      x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/LocateProcessorTests.java
  12. 2 4
      x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/ReplaceProcessorTests.java
  13. 2 4
      x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/SubstringProcessorTests.java

+ 2 - 2
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]
 --------------------------------------------------

+ 283 - 0
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);
+            }
+        });
+    }
+
+}

+ 107 - 0
x-pack/plugin/sql/qa/server/single-node/src/test/resources/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT-non-tested-functions.txt

@@ -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

+ 1 - 7
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java

@@ -48,21 +48,15 @@ public class InsertFunctionProcessor implements Processor {
     }
 
     public static Object doProcess(Object input, Object start, Object length, Object replacement) {
-        if (input == null) {
+        if (input == null || start == null || length == null || replacement == null) {
             return null;
         }
         if ((input instanceof String || input instanceof Character) == false) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", input);
         }
-        if (replacement == null) {
-            return input;
-        }
         if ((replacement instanceof String || replacement instanceof Character) == false) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", replacement);
         }
-        if (start == null || length == null) {
-            return input;
-        }
 
         Check.isFixedNumberAndInRange(start, "start", (long) Integer.MIN_VALUE + 1, (long) Integer.MAX_VALUE);
         Check.isFixedNumberAndInRange(length, "length", 0L, (long) Integer.MAX_VALUE);

+ 9 - 3
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Locate.java

@@ -10,6 +10,7 @@ import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.Expressions;
 import org.elasticsearch.xpack.ql.expression.Expressions.ParamOrdinal;
 import org.elasticsearch.xpack.ql.expression.FieldAttribute;
+import org.elasticsearch.xpack.ql.expression.Nullability;
 import org.elasticsearch.xpack.ql.expression.function.OptionalArgument;
 import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction;
 import org.elasticsearch.xpack.ql.expression.gen.pipeline.Pipe;
@@ -20,11 +21,11 @@ import org.elasticsearch.xpack.ql.tree.Source;
 import org.elasticsearch.xpack.ql.type.DataType;
 import org.elasticsearch.xpack.ql.type.DataTypes;
 
-import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
 
 import static java.lang.String.format;
+import static java.util.Arrays.asList;
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isNumeric;
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact;
 import static org.elasticsearch.xpack.ql.expression.gen.script.ParamsBuilder.paramsBuilder;
@@ -42,7 +43,7 @@ public class Locate extends ScalarFunction implements OptionalArgument {
     private final Expression pattern, input, start;
 
     public Locate(Source source, Expression pattern, Expression input, Expression start) {
-        super(source, start != null ? Arrays.asList(pattern, input, start) : Arrays.asList(pattern, input));
+        super(source, start != null ? asList(pattern, input, start) : asList(pattern, input));
         this.pattern = pattern;
         this.input = input;
         this.start = start;
@@ -80,6 +81,11 @@ public class Locate extends ScalarFunction implements OptionalArgument {
         return NodeInfo.create(this, Locate::new, pattern, input, start);
     }
 
+    @Override
+    public Nullability nullable() {
+        return (Expressions.isNull(pattern) || Expressions.isNull(input)) ? Nullability.TRUE : Nullability.UNKNOWN;
+    }
+
     @Override
     public boolean foldable() {
         return pattern.foldable()
@@ -89,7 +95,7 @@ public class Locate extends ScalarFunction implements OptionalArgument {
 
     @Override
     public Object fold() {
-        return doProcess(pattern.fold(), input.fold(), (start == null ? null : start.fold()));
+        return doProcess(pattern.fold(), input.fold(), start == null ? null : start.fold());
     }
 
     @Override

+ 5 - 8
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/LocateFunctionProcessor.java

@@ -43,17 +43,14 @@ public class LocateFunctionProcessor implements Processor {
     public Object process(Object input) {
         return doProcess(pattern().process(input), input().process(input), start() == null ? null : start().process(input));
     }
-
+    
     public static Integer doProcess(Object pattern, Object input, Object start) {
-        if (input == null) {
+        if (pattern == null || input == null) {
             return null;
         }
         if ((input instanceof String || input instanceof Character) == false) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", input);
         }
-        if (pattern == null) {
-            return 0;
-        }
 
         if ((pattern instanceof String || pattern instanceof Character) == false) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", pattern);
@@ -66,9 +63,9 @@ public class LocateFunctionProcessor implements Processor {
         String stringInput = input instanceof Character ? input.toString() : (String) input;
         String stringPattern = pattern instanceof Character ? pattern.toString() : (String) pattern;
 
-        return Integer.valueOf(1 + (start != null ?
-                stringInput.indexOf(stringPattern, ((Number) start).intValue() - 1)
-                : stringInput.indexOf(stringPattern)));
+        
+        int startIndex = start == null ? 0 : ((Number) start).intValue() - 1;
+        return 1 + stringInput.indexOf(stringPattern, startIndex);
     }
 
     @Override

+ 2 - 5
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/ReplaceFunctionProcessor.java

@@ -45,16 +45,13 @@ public class ReplaceFunctionProcessor implements Processor {
     }
 
     public static Object doProcess(Object input, Object pattern, Object replacement) {
-        if (input == null) {
+        if (input == null || pattern == null || replacement == null) {
             return null;
         }
         if ((input instanceof String || input instanceof Character) == false) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", input);
         }
-        if (pattern == null || replacement == null) {
-            return input;
-        }
-        if ((pattern instanceof String || pattern instanceof Character) == false) {
+        if (!(pattern instanceof String || pattern instanceof Character)) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", pattern);
         }
         if ((replacement instanceof String || replacement instanceof Character) == false) {

+ 1 - 1
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/SubstringFunctionProcessor.java

@@ -53,7 +53,7 @@ public class SubstringFunctionProcessor implements Processor {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", input);
         }
         if (start == null || length == null) {
-            return input;
+            return null;
         }
 
         Check.isFixedNumberAndInRange(start, "start", (long) Integer.MIN_VALUE + 1, (long) Integer.MAX_VALUE);

+ 1 - 1
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java

@@ -16,10 +16,10 @@ import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiffP
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DatePartProcessor;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFormatProcessor.Formatter;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
+import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTruncProcessor;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NamedDateTimeProcessor.NameExtractor;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NonIsoDateTimeProcessor.NonIsoDateTimeExtractor;
-import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.QuarterProcessor;
 import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.TimeFunction;
 import org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoProcessor;

+ 3 - 5
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertProcessorTests.java

@@ -45,11 +45,9 @@ public class InsertProcessorTests extends AbstractWireSerializingTestCase<Insert
 
     public void testInsertWithEdgeCases() {
         assertNull(new Insert(EMPTY, l(null), l(4), l(3), l("baz")).makePipe().asProcessor().process(null));
-        assertEquals("foobar", new Insert(EMPTY, l("foobar"), l(4), l(3), l(null)).makePipe().asProcessor().process(null));
-        assertEquals("foobar",
-                new Insert(EMPTY, l("foobar"), l(null), l(3), l("baz")).makePipe().asProcessor().process(null));
-        assertEquals("foobar",
-                new Insert(EMPTY, l("foobar"), l(4), l(null), l("baz")).makePipe().asProcessor().process(null));
+        assertNull(new Insert(EMPTY, l("foobar"), l(null), l(3), l("baz")).makePipe().asProcessor().process(null));
+        assertNull(new Insert(EMPTY, l("foobar"), l(4), l(null), l("baz")).makePipe().asProcessor().process(null));
+        assertNull(new Insert(EMPTY, l("foobar"), l(4), l(3), l(null)).makePipe().asProcessor().process(null));
         assertEquals("bazbar", new Insert(EMPTY, l("foobar"), l(-1), l(3), l("baz")).makePipe().asProcessor().process(null));
         assertEquals("foobaz", new Insert(EMPTY, l("foobar"), l(4), l(30), l("baz")).makePipe().asProcessor().process(null));
         assertEquals("foobaz", new Insert(EMPTY, l("foobar"), l(6), l(1), l('z')).makePipe().asProcessor().process(null));

+ 10 - 6
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/LocateProcessorTests.java

@@ -42,20 +42,24 @@ public class LocateProcessorTests extends AbstractWireSerializingTestCase<Locate
     }
 
     public void testLocateFunctionWithValidInput() {
-        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(1)).makePipe().asProcessor().process(null));
         assertEquals(7, new Locate(EMPTY, l("bar"), l("foobarbar"), l(5)).makePipe().asProcessor().process(null));
     }
 
     public void testLocateFunctionWithEdgeCasesInputs() {
-        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
         assertNull(new Locate(EMPTY, l("bar"), l(null), l(3)).makePipe().asProcessor().process(null));
-        assertEquals(0, new Locate(EMPTY, l(null), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
-        assertEquals(0, new Locate(EMPTY, l(null), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
+        assertNull(new Locate(EMPTY, l(null), l("foobarbar"), l(3)).makePipe().asProcessor().process(null));
+        assertNull(new Locate(EMPTY, l(null), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
 
-        assertEquals(1, new Locate(EMPTY, l("foo"), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), null).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(null)).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(1)).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(0)).makePipe().asProcessor().process(null));
+        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(-3)).makePipe().asProcessor().process(null));
+        assertEquals(0, new Locate(EMPTY, l("bar"), l("foobarbar"), l(100)).makePipe().asProcessor().process(null));
+        assertEquals(1, new Locate(EMPTY, l('o'), l('o'), l(1)).makePipe().asProcessor().process(null));
         assertEquals(1, new Locate(EMPTY, l('o'), l('o'), l(null)).makePipe().asProcessor().process(null));
         assertEquals(9, new Locate(EMPTY, l('r'), l("foobarbar"), l(9)).makePipe().asProcessor().process(null));
-        assertEquals(4, new Locate(EMPTY, l("bar"), l("foobarbar"), l(-3)).makePipe().asProcessor().process(null));
     }
 
     public void testLocateFunctionValidatingInputs() {

+ 2 - 4
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/ReplaceProcessorTests.java

@@ -45,10 +45,8 @@ public class ReplaceProcessorTests extends AbstractWireSerializingTestCase<Repla
     }
 
     public void testReplaceFunctionWithEdgeCases() {
-        assertEquals("foobarbar",
-                new Replace(EMPTY, l("foobarbar"), l("bar"), l(null)).makePipe().asProcessor().process(null));
-        assertEquals("foobarbar",
-                new Replace(EMPTY, l("foobarbar"), l(null), l("baz")).makePipe().asProcessor().process(null));
+        assertNull(new Replace(EMPTY, l("foobarbar"), l("bar"), l(null)).makePipe().asProcessor().process(null));
+        assertNull(new Replace(EMPTY, l("foobarbar"), l(null), l("baz")).makePipe().asProcessor().process(null));
         assertNull(new Replace(EMPTY, l(null), l("bar"), l("baz")).makePipe().asProcessor().process(null));
         assertNull(new Replace(EMPTY, l(null), l(null), l(null)).makePipe().asProcessor().process(null));
     }

+ 2 - 4
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/SubstringProcessorTests.java

@@ -45,10 +45,8 @@ public class SubstringProcessorTests extends AbstractWireSerializingTestCase<Sub
     }
 
     public void testSubstringFunctionWithEdgeCases() {
-        assertEquals("foobarbar",
-                new Substring(EMPTY, l("foobarbar"), l(1), l(null)).makePipe().asProcessor().process(null));
-        assertEquals("foobarbar",
-                new Substring(EMPTY, l("foobarbar"), l(null), l(3)).makePipe().asProcessor().process(null));
+        assertNull(new Substring(EMPTY, l("foobarbar"), l(1), l(null)).makePipe().asProcessor().process(null));
+        assertNull(new Substring(EMPTY, l("foobarbar"), l(null), l(3)).makePipe().asProcessor().process(null));
         assertNull(new Substring(EMPTY, l(null), l(1), l(3)).makePipe().asProcessor().process(null));
         assertNull(new Substring(EMPTY, l(null), l(null), l(null)).makePipe().asProcessor().process(null));