From c9ae1928e0a1bbbdd86634dc2c055b924abe4019 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 29 Oct 2018 11:32:17 +0100 Subject: [PATCH] SQL: Fix function args verification and error msgs (#34926) Extract data type verification for function arguments to a single place so that NULL type can be treated as RESOLVED for all functions. Moreover this enables to have consistent verification error messages for all functions. Fixes: #34752 Fixes: #33469 --- .../xpack/sql/type/DataType.java | 2 +- .../xpack/sql/expression/Expression.java | 8 ++- .../xpack/sql/expression/Expressions.java | 63 ++++++++++++---- .../xpack/sql/expression/Literal.java | 3 +- .../expression/function/aggregate/Max.java | 3 +- .../expression/function/aggregate/Min.java | 3 +- .../function/aggregate/NumericAggregate.java | 3 +- .../function/aggregate/Percentile.java | 3 +- .../function/aggregate/PercentileRank.java | 8 +-- .../sql/expression/function/scalar/Cast.java | 4 +- .../scalar/datetime/BaseDateTimeFunction.java | 10 +-- .../scalar/math/BinaryNumericFunction.java | 19 ++--- .../function/scalar/math/MathFunction.java | 7 +- .../scalar/string/BinaryStringFunction.java | 16 +++-- .../string/BinaryStringNumericFunction.java | 6 +- .../string/BinaryStringStringFunction.java | 7 +- .../function/scalar/string/Concat.java | 9 +-- .../function/scalar/string/Insert.java | 17 ++--- .../function/scalar/string/Locate.java | 15 ++-- .../function/scalar/string/Replace.java | 23 +++--- .../scalar/string/StringFunctionUtils.java | 15 +--- .../function/scalar/string/Substring.java | 13 ++-- .../scalar/string/UnaryStringFunction.java | 8 +-- .../scalar/string/UnaryStringIntFunction.java | 8 +-- .../expression/predicate/BinaryOperator.java | 18 +++-- .../predicate/logical/BinaryLogic.java | 5 +- .../sql/expression/predicate/logical/Not.java | 6 +- .../arithmetic/ArithmeticOperation.java | 8 +-- .../predicate/operator/arithmetic/Neg.java | 5 +- .../operator/comparison/BinaryComparison.java | 2 +- .../xpack/sql/type/DataTypes.java | 6 +- .../analyzer/VerifierErrorMessagesTests.java | 71 +++++++++++++++++-- .../predicate/InProcessorTests.java | 2 +- .../sql/expression/predicate/InTests.java | 2 +- .../xpack/sql/optimizer/OptimizerTests.java | 16 ++++- 35 files changed, 260 insertions(+), 154 deletions(-) diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java index 3ad3b9090a5..e07351a877e 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java @@ -246,4 +246,4 @@ public enum DataType { (isString() && other.isString()) || (isNumeric() && other.isNumeric()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java index 27291a9253e..4a29358a7fa 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java @@ -35,7 +35,11 @@ public abstract class Expression extends Node implements Resolvable public static final TypeResolution TYPE_RESOLVED = new TypeResolution(false, StringUtils.EMPTY); - public TypeResolution(String message, Object... args) { + public TypeResolution(String message) { + this(true, message); + } + + TypeResolution(String message, Object... args) { this(true, format(Locale.ROOT, message, args)); } @@ -132,4 +136,4 @@ public abstract class Expression extends Node implements Resolvable public String toString() { return nodeName() + "[" + propertiesToString(false) + "]"; } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 5b25ac3df92..5a35e00ab9d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -10,10 +10,12 @@ import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.expression.Expression.TypeResolution; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.DataTypes; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Locale; import java.util.function.Predicate; import static java.util.Collections.emptyList; @@ -21,6 +23,14 @@ import static java.util.Collections.emptyMap; public final class Expressions { + public enum ParamOrdinal { + DEFAULT, + FIRST, + SECOND, + THIRD, + FOURTH + } + private Expressions() {} public static NamedExpression wrapAsNamed(Expression exp) { @@ -127,22 +137,51 @@ public final class Expressions { throw new SqlIllegalArgumentException("Cannot create pipe for {}", e); } - public static TypeResolution typeMustBe(Expression e, Predicate predicate, String message) { - return predicate.test(e) ? TypeResolution.TYPE_RESOLVED : new TypeResolution(message); + public static TypeResolution typeMustBeBoolean(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, dt -> dt == DataType.BOOLEAN, operationName, paramOrd, "boolean"); } - public static TypeResolution typeMustBeNumeric(Expression e) { - return e.dataType().isNumeric() ? TypeResolution.TYPE_RESOLVED : new TypeResolution(incorrectTypeErrorMessage(e, "numeric")); + public static TypeResolution typeMustBeInteger(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, dt -> dt.isInteger, operationName, paramOrd, "integer"); } - public static TypeResolution typeMustBeNumericOrDate(Expression e) { - return e.dataType().isNumeric() || e.dataType() == DataType.DATE ? + public static TypeResolution typeMustBeNumeric(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, DataType::isNumeric, operationName, paramOrd, "numeric"); + } + + public static TypeResolution typeMustBeString(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, DataType::isString, operationName, paramOrd, "string"); + } + + public static TypeResolution typeMustBeDate(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, dt -> dt == DataType.DATE, operationName, paramOrd, "date"); + } + + public static TypeResolution typeMustBeNumericOrDate(Expression e, String operationName, ParamOrdinal paramOrd) { + return typeMustBe(e, dt -> dt.isNumeric() || dt == DataType.DATE, operationName, paramOrd, "numeric", "date"); + } + + private static TypeResolution typeMustBe(Expression e, + Predicate predicate, + String operationName, + ParamOrdinal pOrd, + String... acceptedTypes) { + + return predicate.test(e.dataType()) || DataTypes.isNull(e.dataType())? TypeResolution.TYPE_RESOLVED : - new TypeResolution(incorrectTypeErrorMessage(e, "numeric", "date")); + new TypeResolution(incorrectTypeErrorMessage(e, operationName, pOrd, acceptedTypes)); + } - - private static String incorrectTypeErrorMessage(Expression e, String...acceptedTypes) { - return "Argument required to be " + Strings.arrayToDelimitedString(acceptedTypes, " or ") - + " ('" + Expressions.name(e) + "' type is '" + e.dataType().esType + "')"; + + private static String incorrectTypeErrorMessage(Expression e, + String operationName, + ParamOrdinal paramOrd, + String... acceptedTypes) { + return String.format(Locale.ROOT, "[%s]%s argument must be [%s], found value [%s] type [%s]", + operationName, + paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT), + Strings.arrayToDelimitedString(acceptedTypes, " or "), + Expressions.name(e), + e.dataType().esType); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java index 3c334c233f9..2e44240cc0c 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java @@ -26,6 +26,7 @@ public class Literal extends NamedExpression { public static final Literal TRUE = Literal.of(Location.EMPTY, Boolean.TRUE); public static final Literal FALSE = Literal.of(Location.EMPTY, Boolean.FALSE); + public static final Literal NULL = Literal.of(Location.EMPTY, null); private final Object value; private final DataType dataType; @@ -163,4 +164,4 @@ public class Literal extends NamedExpression { return new Literal(foldable.location(), name, fold, foldable.dataType()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Max.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Max.java index fde06f239cb..9c6b1374f07 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Max.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Max.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.aggregate; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; @@ -44,6 +45,6 @@ public class Max extends NumericAggregate implements EnclosedAgg { @Override protected TypeResolution resolveType() { - return Expressions.typeMustBeNumericOrDate(field()); + return Expressions.typeMustBeNumericOrDate(field(), functionName(), ParamOrdinal.DEFAULT); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Min.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Min.java index 42109aaf5d6..e0b68999d64 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Min.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Min.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.aggregate; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; @@ -47,6 +48,6 @@ public class Min extends NumericAggregate implements EnclosedAgg { @Override protected TypeResolution resolveType() { - return Expressions.typeMustBeNumericOrDate(field()); + return Expressions.typeMustBeNumericOrDate(field(), functionName(), ParamOrdinal.DEFAULT); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/NumericAggregate.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/NumericAggregate.java index a71dcfbbb9e..f384e157ec4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/NumericAggregate.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/NumericAggregate.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.aggregate; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.type.DataType; @@ -24,7 +25,7 @@ abstract class NumericAggregate extends AggregateFunction { @Override protected TypeResolution resolveType() { - return Expressions.typeMustBeNumeric(field()); + return Expressions.typeMustBeNumeric(field(), functionName(), ParamOrdinal.DEFAULT); } @Override diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java index a3293161e08..6e644fb4f75 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/Percentile.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.aggregate; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; @@ -43,7 +44,7 @@ public class Percentile extends NumericAggregate implements EnclosedAgg { TypeResolution resolution = super.resolveType(); if (TypeResolution.TYPE_RESOLVED.equals(resolution)) { - resolution = Expressions.typeMustBeNumeric(percent()); + resolution = Expressions.typeMustBeNumeric(percent(), functionName(), ParamOrdinal.DEFAULT); } return resolution; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java index dabe27a0cae..f01dad8800c 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/PercentileRank.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.aggregate; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.Foldables; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; @@ -41,12 +42,11 @@ public class PercentileRank extends AggregateFunction implements EnclosedAgg { @Override protected TypeResolution resolveType() { TypeResolution resolution = super.resolveType(); - - if (TypeResolution.TYPE_RESOLVED.equals(resolution)) { - resolution = Expressions.typeMustBeNumeric(value); + if (resolution.unresolved()) { + return resolution; } - return resolution; + return Expressions.typeMustBeNumeric(value, functionName(), ParamOrdinal.DEFAULT); } public Expression value() { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java index 29803964044..13b9d9822c4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java @@ -65,7 +65,7 @@ public class Cast extends UnaryScalarFunction { protected TypeResolution resolveType() { return DataTypeConversion.canConvert(from(), to()) ? TypeResolution.TYPE_RESOLVED : - new TypeResolution("Cannot cast %s to %s", from(), to()); + new TypeResolution("Cannot cast [" + from() + "] to [" + to()+ "]"); } @Override @@ -102,4 +102,4 @@ public class Cast extends UnaryScalarFunction { sb.insert(sb.length() - 1, " AS " + to().sqlName()); return sb.toString(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java index 952941342b5..130acd8eddc 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java @@ -8,10 +8,10 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.datetime; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; -import org.elasticsearch.xpack.sql.type.DataType; import org.joda.time.DateTime; import java.util.Objects; @@ -42,11 +42,7 @@ abstract class BaseDateTimeFunction extends UnaryScalarFunction { @Override protected TypeResolution resolveType() { - if (field().dataType() == DataType.DATE) { - return TypeResolution.TYPE_RESOLVED; - } - return new TypeResolution("Function [" + functionName() + "] cannot be applied on a non-date expression ([" - + Expressions.name(field()) + "] of type [" + field().dataType().esType + "])"); + return Expressions.typeMustBeDate(field(), functionName(), ParamOrdinal.DEFAULT); } public TimeZone timeZone() { @@ -90,4 +86,4 @@ abstract class BaseDateTimeFunction extends UnaryScalarFunction { public int hashCode() { return Objects.hash(field(), timeZone()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/BinaryNumericFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/BinaryNumericFunction.java index 1d26a88c012..6b067a9a875 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/BinaryNumericFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/BinaryNumericFunction.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.math; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.function.scalar.BinaryScalarFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.math.BinaryMathProcessor.BinaryMathOperation; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; @@ -19,7 +20,7 @@ public abstract class BinaryNumericFunction extends BinaryScalarFunction { private final BinaryMathOperation operation; - protected BinaryNumericFunction(Location location, Expression left, Expression right, BinaryMathOperation operation) { + BinaryNumericFunction(Location location, Expression left, Expression right, BinaryMathOperation operation) { super(location, left, right); this.operation = operation; } @@ -35,18 +36,12 @@ public abstract class BinaryNumericFunction extends BinaryScalarFunction { return new TypeResolution("Unresolved children"); } - TypeResolution resolution = resolveInputType(left().dataType()); + TypeResolution resolution = Expressions.typeMustBeNumeric(left(), functionName(), ParamOrdinal.FIRST); + if (resolution.unresolved()) { + return resolution; - if (resolution == TypeResolution.TYPE_RESOLVED) { - return resolveInputType(right().dataType()); } - return resolution; - } - - protected TypeResolution resolveInputType(DataType inputType) { - return inputType.isNumeric() ? - TypeResolution.TYPE_RESOLVED : - new TypeResolution("'%s' requires a numeric type, received %s", scriptMethodName(), inputType.esType); + return Expressions.typeMustBeNumeric(right(), functionName(), ParamOrdinal.SECOND); } @Override @@ -74,4 +69,4 @@ public abstract class BinaryNumericFunction extends BinaryScalarFunction { && Objects.equals(other.right(), right()) && Objects.equals(other.operation, operation); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathFunction.java index cd37e539bfc..ce6239c3cac 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathFunction.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.math; import org.elasticsearch.xpack.sql.expression.Expression; +import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.math.MathProcessor.MathOperation; import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; @@ -57,8 +59,7 @@ public abstract class MathFunction extends UnaryScalarFunction { return new TypeResolution("Unresolved children"); } - return field().dataType().isNumeric() ? TypeResolution.TYPE_RESOLVED - : new TypeResolution("'%s' requires a numeric type, received %s", operation(), field().dataType().esType); + return Expressions.typeMustBeNumeric(field(), operation().toString(), ParamOrdinal.DEFAULT); } @Override @@ -81,4 +82,4 @@ public abstract class MathFunction extends UnaryScalarFunction { public int hashCode() { return Objects.hash(field()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java index c1e74334442..b18ebe4f491 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringFunction.java @@ -10,12 +10,13 @@ import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.function.scalar.BinaryScalarFunction; import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.sql.tree.Location; -import org.elasticsearch.xpack.sql.type.DataType; import java.util.Locale; import java.util.Objects; import java.util.function.BiFunction; +import static org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; +import static org.elasticsearch.xpack.sql.expression.Expressions.typeMustBeString; import static org.elasticsearch.xpack.sql.expression.gen.script.ParamsBuilder.paramsBuilder; /** @@ -41,14 +42,15 @@ public abstract class BinaryStringFunction extends BinaryScalarFunction { return new TypeResolution("Unresolved children"); } - if (!left().dataType().isString()) { - return new TypeResolution("'%s' requires first parameter to be a string type, received %s", functionName(), left().dataType()); + TypeResolution resolution = typeMustBeString(left(), functionName(), ParamOrdinal.FIRST); + if (resolution.unresolved()) { + return resolution; } - - return resolveSecondParameterInputType(right().dataType()); + + return resolveSecondParameterInputType(right()); } - protected abstract TypeResolution resolveSecondParameterInputType(DataType inputType); + protected abstract TypeResolution resolveSecondParameterInputType(Expression e); @Override public Object fold() { @@ -83,4 +85,4 @@ public abstract class BinaryStringFunction extends BinaryScalarFunction { return Objects.equals(other.left(), left()) && Objects.equals(other.right(), right()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericFunction.java index eaddf4bc70f..8cc90e050e0 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericFunction.java @@ -25,10 +25,8 @@ public abstract class BinaryStringNumericFunction extends BinaryStringFunction 0); } - static TypeResolution resolveStringInputType(DataType inputType, String functionName) { - return inputType.isString() ? - TypeResolution.TYPE_RESOLVED : - new TypeResolution("'%s' requires a string type, received %s", functionName, inputType.esType); - } - - static TypeResolution resolveNumericInputType(DataType inputType, String functionName) { - return inputType.isNumeric() ? - TypeResolution.TYPE_RESOLVED : - new TypeResolution("'%s' requires a numeric type, received %s", functionName, inputType.esType); - } + } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Substring.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Substring.java index e1475665110..ea8378a224d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Substring.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Substring.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.string; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe; @@ -44,17 +45,17 @@ public class Substring extends ScalarFunction { return new TypeResolution("Unresolved children"); } - TypeResolution sourceResolution = StringFunctionUtils.resolveStringInputType(source.dataType(), functionName()); - if (sourceResolution != TypeResolution.TYPE_RESOLVED) { + TypeResolution sourceResolution = Expressions.typeMustBeString(source, functionName(), ParamOrdinal.FIRST); + if (sourceResolution.unresolved()) { return sourceResolution; } - TypeResolution startResolution = StringFunctionUtils.resolveNumericInputType(start.dataType(), functionName()); - if (startResolution != TypeResolution.TYPE_RESOLVED) { + TypeResolution startResolution = Expressions.typeMustBeNumeric(start, functionName(), ParamOrdinal.SECOND); + if (startResolution.unresolved()) { return startResolution; } - return StringFunctionUtils.resolveNumericInputType(length.dataType(), functionName()); + return Expressions.typeMustBeNumeric(length, functionName(), ParamOrdinal.THIRD); } @Override @@ -123,4 +124,4 @@ public class Substring extends ScalarFunction { return new Substring(location(), newChildren.get(0), newChildren.get(1), newChildren.get(2)); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringFunction.java index af9bd05fd15..8c64fefc36b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringFunction.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.string; import org.elasticsearch.xpack.sql.expression.Expression; +import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation; @@ -41,9 +43,7 @@ public abstract class UnaryStringFunction extends UnaryScalarFunction { if (!childrenResolved()) { return new TypeResolution("Unresolved children"); } - - return field().dataType().isString() ? TypeResolution.TYPE_RESOLVED : new TypeResolution( - "'%s' requires a string type, received %s", operation(), field().dataType().esType); + return Expressions.typeMustBeString(field(), operation().toString(), ParamOrdinal.DEFAULT); } @Override @@ -82,4 +82,4 @@ public abstract class UnaryStringFunction extends UnaryScalarFunction { public int hashCode() { return Objects.hash(field()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringIntFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringIntFunction.java index 0753af03f14..a14acef60e5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringIntFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/UnaryStringIntFunction.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.sql.expression.function.scalar.string; import org.elasticsearch.xpack.sql.expression.Expression; +import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.function.scalar.UnaryScalarFunction; import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation; @@ -43,9 +45,7 @@ public abstract class UnaryStringIntFunction extends UnaryScalarFunction { if (!childrenResolved()) { return new TypeResolution("Unresolved children"); } - - return field().dataType().isInteger ? TypeResolution.TYPE_RESOLVED : new TypeResolution( - "'%s' requires a integer type, received %s", operation(), field().dataType().esType); + return Expressions.typeMustBeInteger(field(), operation().toString(), ParamOrdinal.DEFAULT); } @Override @@ -83,4 +83,4 @@ public abstract class UnaryStringIntFunction extends UnaryScalarFunction { UnaryStringIntFunction other = (UnaryStringIntFunction) obj; return Objects.equals(other.field(), field()); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryOperator.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryOperator.java index 9684913dba8..e5e6e3563a8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryOperator.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryOperator.java @@ -6,8 +6,9 @@ package org.elasticsearch.xpack.sql.expression.predicate; import org.elasticsearch.xpack.sql.expression.Expression; +import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.tree.Location; -import org.elasticsearch.xpack.sql.type.DataType; /** * Operator is a specialized binary predicate where both sides have the compatible types @@ -23,7 +24,7 @@ public abstract class BinaryOperator swapLeftAndRight(); @@ -32,14 +33,11 @@ public abstract class BinaryOperator { @@ -22,7 +23,6 @@ public class InProcessorTests extends AbstractWireSerializingTestCase