Skip to content

Commit

Permalink
QL: Remove implicit conversion inside Literal (elastic#50962)
Browse files Browse the repository at this point in the history 
Literal constructor makes an implicit conversion for each value given
which turns out has some subtle side-effects.
Improve MathProcessors to preserve numeric type where possible
Fix bug on issue compatibility between date and intervals
Preserve the source when folding inside the Optimizer
  • Loading branch information
@costin @SivagurunathanV
costin authored and SivagurunathanV committed Jan 21, 2020
1 parent 78a49a7 commit b0586ad
Show file tree
Hide file tree
Showing 14 changed files with 193 additions and 109 deletions.
9 changes: 4 additions & 5 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Literal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypeConversion;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.util.Objects;
Expand All @@ -19,17 +18,17 @@
*/
public class Literal extends LeafExpression {

public static final Literal TRUE = Literal.of(Source.EMPTY, Boolean.TRUE);
public static final Literal FALSE = Literal.of(Source.EMPTY, Boolean.FALSE);
public static final Literal NULL = Literal.of(Source.EMPTY, null);
public static final Literal TRUE = new Literal(Source.EMPTY, Boolean.TRUE, DataType.BOOLEAN);
public static final Literal FALSE = new Literal(Source.EMPTY, Boolean.FALSE, DataType.BOOLEAN);
public static final Literal NULL = new Literal(Source.EMPTY, null, DataType.NULL);

private final Object value;
private final DataType dataType;

public Literal(Source source, Object value, DataType dataType) {
super(source);
this.dataType = dataType;
this.value = DataTypeConversion.convert(value, dataType);
this.value = value;
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...n/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/UnresolvedFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public UnresolvedFunction preprocessStar(UnresolvedFunction uf) {
// dedicated count optimization
if (uf.name.toUpperCase(Locale.ROOT).equals("COUNT")) {
return new UnresolvedFunction(uf.source(), uf.name(), uf.resolutionType,
singletonList(Literal.of(uf.arguments().get(0).source(), Integer.valueOf(1))));
singletonList(new Literal(uf.arguments().get(0).source(), Integer.valueOf(1), DataType.INTEGER)));
}
return uf;
}
Expand Down
5 changes: 2 additions & 3 deletions ...src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/In.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,13 @@ public Nullability nullable() {
@Override
public boolean foldable() {
return Expressions.foldable(children()) ||
(Expressions.foldable(list) && list().stream().allMatch(e -> e.dataType() == DataType.NULL));
(Expressions.foldable(list) && list().stream().allMatch(Expressions::isNull));
}

@Override
public Boolean fold() {
// Optimization for early return and Query folding to LocalExec
if (value.dataType() == DataType.NULL ||
list.size() == 1 && list.get(0).dataType() == DataType.NULL) {
if (Expressions.isNull(value) || list.size() == 1 && Expressions.isNull(list.get(0))) {
return null;
}
return InProcessor.apply(value.fold(), Foldables.valuesOf(list, value.dataType()));
Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataTypeConversion.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,18 @@ public static DataType commonType(DataType left, DataType right) {

// interval and dates
if (left == DATE) {
if (right.isInterval()) {
if (right.isYearMonthInterval()) {
return left;
}
// promote
return DATETIME;
}
if (right == DATE) {
if (left.isInterval()) {
if (left.isYearMonthInterval()) {
return right;
}
// promote
return DATETIME;
}
if (left == TIME) {
if (right == DATE) {
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/LiteralTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ static class ValueAndCompatibleTypes {

public static Literal randomLiteral() {
ValueAndCompatibleTypes gen = randomFrom(GENERATORS);
return new Literal(SourceTests.randomSource(), gen.valueSupplier.get(), randomFrom(gen.validDataTypes));
DataType dataType = randomFrom(gen.validDataTypes);
return new Literal(SourceTests.randomSource(), DataTypeConversion.convert(gen.valueSupplier.get(), dataType), dataType);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/type/DataTypeConversionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,8 @@ public void testCommonType() {
assertEquals(DATETIME, commonType(randomInterval(), DATETIME));
assertEquals(DATETIME, commonType(DATE, TIME));
assertEquals(DATETIME, commonType(TIME, DATE));
assertEquals(DATE, commonType(DATE, randomInterval()));
assertEquals(DATE, commonType(randomInterval(), DATE));
assertEquals(DATE, commonType(DATE, INTERVAL_YEAR));
assertEquals(DATETIME, commonType(DATE, INTERVAL_HOUR_TO_MINUTE));
assertEquals(TIME, commonType(TIME, randomInterval()));
assertEquals(TIME, commonType(randomInterval(), TIME));

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ SELECT ISNULL(birth_date, INTERVAL '23:45' HOUR TO MINUTES + {d '2019-09-17'}) A
c:ts | salary:i | birth_date:ts | hire_date:ts
------------------------+-----------------+------------------------+------------------------
1956-12-13T00:00:00.000Z|74999 |1956-12-13T00:00:00.000Z|1985-11-20T00:00:00.000Z
2019-09-17T00:00:00.000Z|74970 |null |1989-09-02T00:00:00.000Z
2019-09-17T23:45:00.000Z|74970 |null |1989-09-02T00:00:00.000Z
1957-05-23T00:00:00.000Z|74572 |1957-05-23T00:00:00.000Z|1989-02-10T00:00:00.000Z
1962-07-10T00:00:00.000Z|73851 |1962-07-10T00:00:00.000Z|1989-07-07T00:00:00.000Z
1953-01-23T00:00:00.000Z|73717 |1953-01-23T00:00:00.000Z|1999-04-30T00:00:00.000Z
Expand Down
6 changes: 2 additions & 4 deletions ...plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.sql.expression.function.scalar;

import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.Expressions;
import org.elasticsearch.xpack.ql.expression.Nullability;
import org.elasticsearch.xpack.ql.expression.function.scalar.UnaryScalarFunction;
import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
Expand Down Expand Up @@ -64,10 +65,7 @@ public Object fold() {

@Override
public Nullability nullable() {
if (from().isNull()) {
return Nullability.TRUE;
}
return Nullability.UNKNOWN;
return Expressions.isNull(field()) ? Nullability.TRUE : Nullability.UNKNOWN;
}

@Override
Expand Down
67 changes: 56 additions & 11 deletions .../main/java/org/elasticsearch/xpack/sql/expression/function/scalar/math/MathProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import org.elasticsearch.common.Randomness;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.xpack.ql.QlIllegalArgumentException;
import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.ql.type.DataTypeConversion;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;

import java.io.IOException;
Expand All @@ -21,17 +23,38 @@ public class MathProcessor implements Processor {

public enum MathOperation {
ABS((Object l) -> {
if (l instanceof Float) {
return Double.valueOf(Math.abs(((Float) l).floatValue()));
}
if (l instanceof Double) {
return Math.abs(((Double) l).doubleValue());
}
if (l instanceof Float) {
return Math.abs(((Float) l).floatValue());
}

// fallback to integer
long lo = ((Number) l).longValue();
//handles the corner-case of Long.MIN_VALUE
return lo >= 0 ? lo : lo == Long.MIN_VALUE ? Double.valueOf(Long.MAX_VALUE) : -lo;
}),

if (lo == Long.MIN_VALUE) {
throw new QlIllegalArgumentException("[" + lo + "] cannot be negated since the result is outside the range");
}

lo = lo < 0 ? -lo : lo;

if (l instanceof Integer) {
if ((int) lo == Integer.MIN_VALUE) {
throw new QlIllegalArgumentException("[" + lo + "] cannot be negated since the result is outside the range");
}
return DataTypeConversion.safeToInt(lo);
}

if (l instanceof Short) {
return DataTypeConversion.safeToShort(lo);
}
if (l instanceof Byte) {
return DataTypeConversion.safeToByte(lo);
}

return lo;
}),
ACOS(Math::acos),
ASIN(Math::asin),
ATAN(Math::atan),
Expand All @@ -52,15 +75,37 @@ public enum MathOperation {
RANDOM((Object l) -> l != null ?
new Random(((Number) l).longValue()).nextDouble() :
Randomness.get().nextDouble(), true),
SIGN((DoubleFunction<Double>) Math::signum),
SIGN((Object l) -> {
if (l instanceof Double) {
return Math.signum((Double) l);
}
if (l instanceof Float) {
return Math.signum((Float) l);
}

long lo = Long.signum(((Number) l).longValue());

if (l instanceof Integer) {
return DataTypeConversion.safeToInt(lo);
}
if (l instanceof Short) {
return DataTypeConversion.safeToShort(lo);
}
if (l instanceof Byte) {
return DataTypeConversion.safeToByte(lo);
}

//fallback to generic double
return lo;
}),
SIN(Math::sin),
SINH(Math::sinh),
SQRT(Math::sqrt),
TAN(Math::tan);

private final Function<Object, Double> apply;
private final Function<Object, Number> apply;

MathOperation(Function<Object, Double> apply) {
MathOperation(Function<Object, Number> apply) {
this(apply, false);
}

Expand All @@ -69,7 +114,7 @@ public enum MathOperation {
* If true, nulls are passed through, otherwise the function is short-circuited
* and null returned.
*/
MathOperation(Function<Object, Double> apply, boolean nullAware) {
MathOperation(Function<Object, Number> apply, boolean nullAware) {
if (nullAware) {
this.apply = apply;
} else {
Expand All @@ -85,7 +130,7 @@ public enum MathOperation {
this.apply = l -> supplier.get();
}

public final Double apply(Object l) {
public final Number apply(Object l) {
return apply.apply(l);
}
}
Expand Down
48 changes: 24 additions & 24 deletions .../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,107 +130,107 @@ public static Number truncate(Number v, Number s) {
return BinaryOptionalMathOperation.TRUNCATE.apply(v, s);
}

public static Double abs(Number value) {
public static Number abs(Number value) {
return MathOperation.ABS.apply(value);
}

public static Double acos(Number value) {
public static Number acos(Number value) {
return MathOperation.ACOS.apply(value);
}

public static Double asin(Number value) {
public static Number asin(Number value) {
return MathOperation.ASIN.apply(value);
}

public static Double atan(Number value) {
public static Number atan(Number value) {
return MathOperation.ATAN.apply(value);
}

public static Number atan2(Number left, Number right) {
return BinaryMathOperation.ATAN2.apply(left, right);
}

public static Double cbrt(Number value) {
public static Number cbrt(Number value) {
return MathOperation.CBRT.apply(value);
}

public static Double ceil(Number value) {
public static Number ceil(Number value) {
return MathOperation.CEIL.apply(value);
}

public static Double cos(Number value) {
public static Number cos(Number value) {
return MathOperation.COS.apply(value);
}

public static Double cosh(Number value) {
public static Number cosh(Number value) {
return MathOperation.COSH.apply(value);
}

public static Double cot(Number value) {
public static Number cot(Number value) {
return MathOperation.COT.apply(value);
}

public static Double degrees(Number value) {
public static Number degrees(Number value) {
return MathOperation.DEGREES.apply(value);
}

public static Double e(Number value) {
public static Number e(Number value) {
return MathOperation.E.apply(value);
}

public static Double exp(Number value) {
public static Number exp(Number value) {
return MathOperation.EXP.apply(value);
}

public static Double expm1(Number value) {
public static Number expm1(Number value) {
return MathOperation.EXPM1.apply(value);
}

public static Double floor(Number value) {
public static Number floor(Number value) {
return MathOperation.FLOOR.apply(value);
}

public static Double log(Number value) {
public static Number log(Number value) {
return MathOperation.LOG.apply(value);
}

public static Double log10(Number value) {
public static Number log10(Number value) {
return MathOperation.LOG10.apply(value);
}

public static Double pi(Number value) {
public static Number pi(Number value) {
return MathOperation.PI.apply(value);
}

public static Number power(Number left, Number right) {
return BinaryMathOperation.POWER.apply(left, right);
}

public static Double radians(Number value) {
public static Number radians(Number value) {
return MathOperation.RADIANS.apply(value);
}

public static Double random(Number value) {
public static Number random(Number value) {
return MathOperation.RANDOM.apply(value);
}

public static Double sign(Number value) {
public static Number sign(Number value) {
return MathOperation.SIGN.apply(value);
}

public static Double sin(Number value) {
public static Number sin(Number value) {
return MathOperation.SIN.apply(value);
}

public static Double sinh(Number value) {
public static Number sinh(Number value) {
return MathOperation.SINH.apply(value);
}

public static Double sqrt(Number value) {
public static Number sqrt(Number value) {
return MathOperation.SQRT.apply(value);
}

public static Double tan(Number value) {
public static Number tan(Number value) {
return MathOperation.TAN.apply(value);
}

Expand Down
Loading

0 comments on commit b0586ad

Please sign in to comment.