Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend comparison methods to accept different datetime types. #129

Merged
merged 19 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ public abstract class AbstractExprValue implements ExprValue {
public int compareTo(ExprValue other) {
if (this.isNull() || this.isMissing() || other.isNull() || other.isMissing()) {
throw new IllegalStateException(
String.format("[BUG] Unreachable, Comparing with NULL or MISSING is undefined"));
"[BUG] Unreachable, Comparing with NULL or MISSING is undefined");
}
if ((this.isNumber() && other.isNumber()) || this.type() == other.type()) {
if ((this.isNumber() && other.isNumber())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to include a isComparable(AbstractExprValue) function here? Overridden by child classes that return true for valid cases or are the same type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a new type, we have to update that function override in all/some other types to reflect the change. It is not scalable solution.

|| (this.isDateTime() && other.isDateTime())
|| this.type() == other.type()) {
return compare(other);
} else {
throw new ExpressionEvaluationException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
Expand Down Expand Up @@ -69,7 +68,12 @@ public LocalDateTime datetimeValue() {

@Override
public Instant timestampValue() {
return ZonedDateTime.of(date, timeValue(), ZoneId.systemDefault()).toInstant();
return ZonedDateTime.of(date, timeValue(), ExprTimestampValue.ZONE).toInstant();
}

@Override
public boolean isDateTime() {
return true;
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
GabeFernandez310 marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
Expand Down Expand Up @@ -71,7 +70,12 @@ public LocalTime timeValue() {

@Override
public Instant timestampValue() {
return ZonedDateTime.of(datetime, ZoneId.of("UTC")).toInstant();
return ZonedDateTime.of(datetime, ExprTimestampValue.ZONE).toInstant();
}

@Override
public boolean isDateTime() {
return true;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.format.DateTimeParseException;
import java.util.Objects;
import lombok.RequiredArgsConstructor;
import org.opensearch.sql.data.type.ExprCoreType;
import org.opensearch.sql.data.type.ExprType;
import org.opensearch.sql.exception.SemanticCheckException;
import org.opensearch.sql.expression.function.FunctionProperties;

/**
* Expression Time Value.
Expand Down Expand Up @@ -57,6 +57,24 @@ public LocalTime timeValue() {
return time;
}

public LocalDate dateValue(FunctionProperties functionProperties) {
return LocalDate.now(functionProperties.getQueryStartClock());
}

public LocalDateTime datetimeValue(FunctionProperties functionProperties) {
return LocalDateTime.of(dateValue(functionProperties), timeValue());
}

public Instant timestampValue(FunctionProperties functionProperties) {
return ZonedDateTime.of(dateValue(functionProperties), timeValue(), ExprTimestampValue.ZONE)
.toInstant();
}

@Override
public boolean isDateTime() {
return true;
}

@Override
public String toString() {
return String.format("TIME '%s'", value());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public class ExprTimestampValue extends AbstractExprValue {
/**
* todo. only support UTC now.
*/
private static final ZoneId ZONE = ZoneId.of("UTC");
public static final ZoneId ZONE = ZoneId.of("UTC");

private final Instant timestamp;

Expand Down Expand Up @@ -81,6 +81,11 @@ public LocalDateTime datetimeValue() {
return timestamp.atZone(ZONE).toLocalDateTime();
}

@Override
public boolean isDateTime() {
return true;
}

@Override
public String toString() {
return String.format("TIMESTAMP '%s'", value());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ default boolean isNumber() {
return false;
}

/**
* Is Number value.
*
* @return true: is a datetime value, otherwise false
*/
default boolean isDateTime() {
return false;
}

/**
* Get the {@link BindingTuple}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

package org.opensearch.sql.data.model;

import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.temporal.TemporalAmount;
import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -61,6 +65,22 @@ public static ExprValue intervalValue(TemporalAmount value) {
return new ExprIntervalValue(value);
}

public static ExprValue dateValue(LocalDate value) {
return new ExprDateValue(value);
}

public static ExprValue datetimeValue(LocalDateTime value) {
return new ExprDatetimeValue(value);
}

public static ExprValue timeValue(LocalTime value) {
return new ExprTimeValue(value);
}

public static ExprValue timestampValue(Instant value) {
return new ExprTimestampValue(value);
}

/**
* {@link ExprTupleValue} constructor.
*/
Expand Down Expand Up @@ -115,6 +135,14 @@ public static ExprValue fromObjectValue(Object o) {
return stringValue((String) o);
} else if (o instanceof Float) {
return floatValue((Float) o);
} else if (o instanceof LocalDate) {
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
return dateValue((LocalDate) o);
} else if (o instanceof LocalDateTime) {
return datetimeValue((LocalDateTime) o);
} else if (o instanceof LocalTime) {
return timeValue((LocalTime) o);
} else if (o instanceof Instant) {
return timestampValue((Instant) o);
} else {
throw new ExpressionEvaluationException("unsupported object " + o.getClass());
}
Expand Down
36 changes: 30 additions & 6 deletions core/src/main/java/org/opensearch/sql/expression/DSL.java
Original file line number Diff line number Diff line change
Expand Up @@ -502,28 +502,52 @@ public static FunctionExpression not(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.NOT, expressions);
}

public static FunctionExpression equal(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.EQUAL, expressions);
}

public static FunctionExpression equal(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.EQUAL, expressions);
return equal(FunctionProperties.None, expressions);
}

public static FunctionExpression notequal(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.NOTEQUAL, expressions);
}

public static FunctionExpression notequal(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.NOTEQUAL, expressions);
return notequal(FunctionProperties.None, expressions);
}

public static FunctionExpression less(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.LESS, expressions);
}

public static FunctionExpression less(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.LESS, expressions);
return less(FunctionProperties.None, expressions);
}

public static FunctionExpression lte(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.LTE, expressions);
}

public static FunctionExpression lte(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.LTE, expressions);
return lte(FunctionProperties.None, expressions);
}

public static FunctionExpression greater(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.GREATER, expressions);
}

public static FunctionExpression greater(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.GREATER, expressions);
return greater(FunctionProperties.None, expressions);
}

public static FunctionExpression gte(FunctionProperties fp, Expression... expressions) {
return compile(fp, BuiltinFunctionName.GTE, expressions);
}

public static FunctionExpression gte(Expression... expressions) {
return compile(FunctionProperties.None, BuiltinFunctionName.GTE, expressions);
return gte(FunctionProperties.None, expressions);
}

public static FunctionExpression like(Expression... expressions) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,52 @@ public String toString() {
};
}

/**
* Implementation of a function that takes two arguments, returns a value, and
* requires FunctionProperties to complete.
*
* @param function {@link ExprValue} based unary function.
* @param returnType return type.
* @param args1Type first argument type.
* @param args2Type second argument type.
* @return Unary Function Implementation.
*/
public static SerializableFunction<FunctionName, Pair<FunctionSignature, FunctionBuilder>>
implWithProperties(
SerializableTriFunction<FunctionProperties, ExprValue, ExprValue, ExprValue> function,
ExprType returnType,
ExprType args1Type,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use a List<ExprType> argTypes instead of having to create a separate signature for each permutation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a separate feature. We have to change all impl/implWithProperties and hundreds of their usages in that case.
You are welcome to open a feature request!

ExprType args2Type) {

return functionName -> {
FunctionSignature functionSignature =
new FunctionSignature(functionName, Arrays.asList(args1Type, args2Type));
FunctionBuilder functionBuilder =
(functionProperties, arguments) -> new FunctionExpression(functionName, arguments) {
@Override
public ExprValue valueOf(Environment<Expression, ExprValue> valueEnv) {
ExprValue arg1 = arguments.get(0).valueOf(valueEnv);
ExprValue arg2 = arguments.get(1).valueOf(valueEnv);
return function.apply(functionProperties, arg1, arg2);
}

@Override
public ExprType type() {
return returnType;
}

@Override
public String toString() {
return String.format("%s(%s)", functionName,
arguments.stream()
.map(Object::toString)
.collect(Collectors.joining(", ")));
}
};
return Pair.of(functionSignature, functionBuilder);
};
}

/**
* No Arg Function Implementation.
*
Expand Down Expand Up @@ -317,4 +363,22 @@ public SerializableTriFunction<ExprValue, ExprValue, ExprValue, ExprValue> nullM
}
};
}

/**
* Wrapper for the ExprValue function that takes 2 arguments and is aware of FunctionProperties,
* with default NULL and MISSING handling.
*/
public static SerializableTriFunction<FunctionProperties, ExprValue, ExprValue, ExprValue>
nullMissingHandlingWithProperties(
SerializableTriFunction<FunctionProperties, ExprValue, ExprValue,ExprValue> implementation) {
return (functionProperties, v1, v2) -> {
if (v1.isMissing() || v2.isMissing()) {
return ExprValueUtils.missingValue();
} else if (v1.isNull() || v2.isNull()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else unnecessary here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just copy-pasted from

public static SerializableBiFunction<ExprValue, ExprValue, ExprValue> nullMissingHandling(
SerializableBiFunction<ExprValue, ExprValue, ExprValue> function) {
return (v1, v2) -> {
if (v1.isMissing() || v2.isMissing()) {
return ExprValueUtils.missingValue();
} else if (v1.isNull() || v2.isNull()) {
return ExprValueUtils.nullValue();
} else {
return function.apply(v1, v2);
}
};
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can groom all of them in scope of another task I think.

return ExprValueUtils.nullValue();
} else {
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
return implementation.apply(functionProperties, v1, v2);
}
};
}
}
Loading