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.

Sign up for GitHub

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

Add maketime and makedate #102

Merged
merged 10 commits into from
Aug 12, 2022
Merged

Add maketime and makedate #102

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1153097
Implementation.
Yury-Fridlyand Aug 4, 2022
e9baac8
Fix `maketime` and add tests.
Yury-Fridlyand Aug 8, 2022
b58e49e
Add integration tests.
Yury-Fridlyand Aug 9, 2022
7af8a4d
Add docs.
Yury-Fridlyand Aug 9, 2022
4e83762
Rework on tests according to PR feedback.
Yury-Fridlyand Aug 11, 2022
335cbf8
Update docs.
Yury-Fridlyand Aug 11, 2022
4dae1ce
Add javadoc.
Yury-Fridlyand Aug 11, 2022
797aa03
Minor test improvements.
Yury-Fridlyand Aug 11, 2022
42c3b2b
Replace randomly generated test data with hardcoded.
Yury-Fridlyand Aug 11, 2022
1083a44
Merge remote-tracking branch 'upstream/main' into dev-make-time-n-date
Yury-Fridlyand Aug 12, 2022
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
39 changes: 39 additions & 0 deletions core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import static org.opensearch.sql.data.type.ExprCoreType.DATE;
import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
import static org.opensearch.sql.data.type.ExprCoreType.DOUBLE;
import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
import static org.opensearch.sql.data.type.ExprCoreType.INTERVAL;
import static org.opensearch.sql.data.type.ExprCoreType.LONG;
Expand All @@ -19,6 +20,9 @@
import static org.opensearch.sql.expression.function.FunctionDSL.nullMissingHandling;

import java.time.LocalDate;
import java.time.LocalTime;
import java.time.Year;
import java.time.format.DateTimeFormatter;
import java.time.format.TextStyle;
import java.util.Locale;
import java.util.concurrent.TimeUnit;
Expand All @@ -27,6 +31,7 @@
import org.opensearch.sql.data.model.ExprDatetimeValue;
import org.opensearch.sql.data.model.ExprIntegerValue;
import org.opensearch.sql.data.model.ExprLongValue;
import org.opensearch.sql.data.model.ExprNullValue;
import org.opensearch.sql.data.model.ExprStringValue;
import org.opensearch.sql.data.model.ExprTimeValue;
import org.opensearch.sql.data.model.ExprTimestampValue;
Expand Down Expand Up @@ -64,6 +69,8 @@ public void register(BuiltinFunctionRepository repository) {
repository.register(dayOfYear());
repository.register(from_days());
repository.register(hour());
repository.register(makedate());
repository.register(maketime());
repository.register(microsecond());
repository.register(minute());
repository.register(month());
Expand Down Expand Up @@ -236,6 +243,16 @@ private FunctionResolver hour() {
);
}

private FunctionResolver makedate() {
return define(BuiltinFunctionName.MAKEDATE.getName(),
impl(nullMissingHandling(DateTimeFunction::exprMakeDate), DATE, DOUBLE, DOUBLE));
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
}

private FunctionResolver maketime() {
return define(BuiltinFunctionName.MAKETIME.getName(),
impl(nullMissingHandling(DateTimeFunction::exprMakeTime), TIME, DOUBLE, DOUBLE, DOUBLE));
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* MICROSECOND(STRING/TIME/DATETIME/TIMESTAMP). return the microsecond value for time.
*/
Expand Down Expand Up @@ -512,6 +529,28 @@ private ExprValue exprHour(ExprValue time) {
return new ExprIntegerValue(time.timeValue().getHour());
}

private ExprValue exprMakeDate(ExprValue yearExpr, ExprValue dayOfYearExp) {
Copy link

Choose a reason for hiding this comment

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

A javadoc with some explanation of the mysql compliance would be nice.

var year = Math.round(yearExpr.doubleValue());
var dayOfYear = Math.round(dayOfYearExp.doubleValue());
// We need to do this to comply with MySQL
if (0 >= dayOfYear || 0 > year) {
Copy link

Choose a reason for hiding this comment

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

nit: Can we make this check prior to rounding? Then if the value is negative, we save those operations.

Copy link
Author

Choose a reason for hiding this comment

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

To optimize performance we shouldn't use LocalDate::parse nor LocalDate::plusDays. See my note about faster and easier implementation.

return ExprNullValue.of();
}
if (0 == year) {
year = 2000;
}
return new ExprDateValue(LocalDate.ofYearDay((int)year, 1).plusDays(dayOfYear - 1));
}

private ExprValue exprMakeTime(ExprValue hour, ExprValue minute, ExprValue second) {
if (0 > hour.doubleValue() || 0 > minute.doubleValue() || 0 > second.doubleValue()) {
Copy link

Choose a reason for hiding this comment

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

Can the minutes and seconds not be valid at 0?

Copy link
Author

Choose a reason for hiding this comment

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

I check here for negative values, zero is OK for maketime:

opensearchsql> select maketime(0, 0, 0);
fetched rows / total rows = 1/1
+---------------------+
| maketime(0, 0, 0)   |
|---------------------|
| 00:00:00            |
+---------------------+

reference:

mysql> select maketime(0, 0, 0);
+-------------------+
| maketime(0, 0, 0) |
+-------------------+
| 00:00:00          |
+-------------------+
1 row in set (0.00 sec)

return ExprNullValue.of();
}
return new ExprTimeValue(LocalTime.parse(String.format("%02d:%02d:%012.9f",
Math.round(hour.doubleValue()), Math.round(minute.doubleValue()), second.doubleValue()),
DateTimeFormatter.ISO_TIME));
}

/**
* Microsecond implementation for ExprValue.
*
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public enum BuiltinFunctionName {
DAYOFYEAR(FunctionName.of("dayofyear")),
FROM_DAYS(FunctionName.of("from_days")),
HOUR(FunctionName.of("hour")),
MAKEDATE(FunctionName.of("makedate")),
MAKETIME(FunctionName.of("maketime")),
MICROSECOND(FunctionName.of("microsecond")),
MINUTE(FunctionName.of("minute")),
MONTH(FunctionName.of("month")),
Expand Down
145 changes: 145 additions & 0 deletions core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.opensearch.sql.data.model.ExprValueUtils.stringValue;
import static org.opensearch.sql.data.type.ExprCoreType.DATE;
import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
import static org.opensearch.sql.data.type.ExprCoreType.DOUBLE;
import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
import static org.opensearch.sql.data.type.ExprCoreType.INTERVAL;
import static org.opensearch.sql.data.type.ExprCoreType.LONG;
Expand All @@ -24,7 +25,15 @@
import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;

import com.google.common.collect.ImmutableList;
import java.time.Duration;
import java.time.LocalDate;
import java.time.LocalTime;
import java.time.Year;
import java.util.HashSet;
import java.util.List;
import java.util.Random;
import java.util.Set;
import java.util.stream.IntStream;
import lombok.AllArgsConstructor;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -43,7 +52,10 @@
import org.opensearch.sql.expression.Expression;
import org.opensearch.sql.expression.ExpressionTestBase;
import org.opensearch.sql.expression.FunctionExpression;
import org.opensearch.sql.expression.config.ExpressionConfig;
import org.opensearch.sql.expression.env.Environment;
import org.opensearch.sql.expression.function.FunctionName;
import org.opensearch.sql.expression.function.FunctionSignature;

@ExtendWith(MockitoExtension.class)
class DateTimeFunctionTest extends ExpressionTestBase {
Expand Down Expand Up @@ -994,6 +1006,139 @@ public void date_format() {
assertEquals(missingValue(), eval(dsl.date_format(DSL.literal(""), missingRef)));
}

private FunctionExpression maketime(Expression hour, Expression minute, Expression second) {
Copy link

Choose a reason for hiding this comment

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

I realize these tests follow the convention in this file, but the tests would be much easier to understand if each function had it's own test suite, and each test in a suite tested a particular scenario -- all valid arguments, null as an argument, negative values, rounding behaviour, etc.

I'll leave it up to you whether you want to break with the convention and introduce a MakeTimeTests.

Copy link

Choose a reason for hiding this comment

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

This is a good suggestion. I would recommend setting the convention for the test file to include the original name, such as: DateTimeFunctionMakedateTest and DateTimeFunctionMaketimeTest. It would be easier to find for future developers.

var repo = new ExpressionConfig().functionRepository();
var func = repo.resolve(new FunctionSignature(new FunctionName("maketime"),
List.of(DOUBLE, DOUBLE, DOUBLE)));
return (FunctionExpression)func.apply(List.of(hour, minute, second));
}

private LocalTime maketime(Double hour, Double minute, Double second) {
return maketime(DSL.literal(hour), DSL.literal(minute), DSL.literal(second))
.valueOf(null).timeValue();
}

@Test
public void maketime() {
var r = new Random();
assertEquals(maketime(20., 30., 40.), LocalTime.of(20, 30, 40));

for (var ignored : IntStream.range(0, 25).toArray()) {
Copy link

Choose a reason for hiding this comment

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

Why is this for loop necessary? What does it test that isn't covered by the test on line 1024 above?

As an aside, tests must be reproducible so if a test uses RNG, the user must be able to seed it and any test failures must include the seed used. Otherwise, if it does fail, there is no way to investigate the failure.

Copy link

Choose a reason for hiding this comment

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

Instead of testing RNG numbers, test boundary cases. For example: 0, and 23:59:59.99 would be decent test cases for time.
Feb 29 on leap years, and older years would be good boundary cases for dates.
Lots of timezones (and over the the dateline) are also excellent test cases when timezones are included.

Copy link
Author

Choose a reason for hiding this comment

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

I dump values used on case of failure, but if it not enough I can hardcode values gotten from RNG.

var hour = r.nextDouble() * 23;
var minute = r.nextDouble() * 59;
var second = r.nextDouble() * 59;
// results could have 1 nanosec diff because of rounding FP
var expected = LocalTime.of((int)Math.round(hour), (int)Math.round(minute),
// pick fraction second part as nanos
(int)Math.floor(second)).withNano((int)((second % 1) * 1E9));
var delta = Duration.between(expected, maketime(hour, minute, second)).getNano();
assertEquals(0, delta, 1,
String.format("hour = %f, minute = %f, second = %f", hour, minute, second));
}

assertEquals(nullValue(), eval(maketime(DSL.literal(-1.), DSL.literal(42.), DSL.literal(42.))),
"Negative hour doesn't produce NULL");
assertEquals(nullValue(), eval(maketime(DSL.literal(42.), DSL.literal(-1.), DSL.literal(42.))),
"Negative minute doesn't produce NULL");
assertEquals(nullValue(), eval(maketime(DSL.literal(42.), DSL.literal(42.), DSL.literal(-1.))),
"Negative second doesn't produce NULL");

// The function has 3 arguments, testing NULL and MISSING in all position(s)
var combinations = permuteArgumentsWith(3, nullRef, DSL.literal(42.));
for (var args : combinations) {
assertEquals(nullValue(), eval(maketime(args[0], args[1], args[2])));
}
combinations = permuteArgumentsWith(3, missingRef, DSL.literal(42.));
for (var args : combinations) {
assertEquals(missingValue(), eval(maketime(args[0], args[1], args[2])));
}
}

private FunctionExpression makedate(Expression year, Expression dayOfYear) {
var repo = new ExpressionConfig().functionRepository();
var func = repo.resolve(new FunctionSignature(new FunctionName("makedate"),
List.of(DOUBLE, DOUBLE)));
return (FunctionExpression)func.apply(List.of(year, dayOfYear));
}

private LocalDate makedate(Double year, Double dayOfYear) {
return makedate(DSL.literal(year), DSL.literal(dayOfYear)).valueOf(null).dateValue();
}

@Test
public void makedate() {
var r = new Random();
for (var ignored : IntStream.range(0, 1025).toArray()) {
Copy link

Choose a reason for hiding this comment

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

What cases are tested in this for loop? Why is RNG necessary?

Copy link
Author

Choose a reason for hiding this comment

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

RNG helps to find unconsidered cases, it helped me to find few bugs. I dump values which cause a failure, so any other developer would be able to reproduce it.

Copy link

Choose a reason for hiding this comment

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

A good discussion on using RNG in unit/integration tests: https://softwareengineering.stackexchange.com/questions/429601/is-the-usage-of-random-values-in-unit-testing-a-good-practice

// Avoid zero values
var year = r.nextDouble() * 5000 + 1;
var dayOfYear = r.nextDouble() * 1000 + 500;

LocalDate actual = makedate(year, dayOfYear);

// Using another algorithm to get reference value
// We should go to the next year until remaining @dayOfYear is bigger than 365/366
var yearL = (int)Math.round(year);
var dayL = (int)Math.round(dayOfYear);
while (true) {
int daysInYear = Year.isLeap(yearL) ? 366 : 365;
if (dayL > daysInYear) {
dayL -= daysInYear;
yearL++;
} else {
break;
}
}
LocalDate expected = LocalDate.ofYearDay(yearL, dayL);

assertEquals(expected, actual,
String.format("year = %f, dayOfYear = %f", year, dayOfYear));
}

assertEquals(LocalDate.ofYearDay(2000, 42), makedate(0., 42.),
"0 year is not interpreted as 2000 as in MySQL");
assertEquals(nullValue(), eval(makedate(DSL.literal(-1.), DSL.literal(42.))),
"Negative year doesn't produce NULL");
assertEquals(nullValue(), eval(makedate(DSL.literal(42.), DSL.literal(-1.))),
"Negative dayOfYear doesn't produce NULL");
assertEquals(nullValue(), eval(makedate(DSL.literal(42.), DSL.literal(0.))),
Copy link

Choose a reason for hiding this comment

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

What about day 366 of a non-leap year?

"Zero dayOfYear doesn't produce NULL");
Copy link

Choose a reason for hiding this comment

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

what about tests for invalid hour/minute/second or year/month/day?
(is there an invalid year, or is year 0 and 9999 valid?)

Copy link
Author

Choose a reason for hiding this comment

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

MySQL is limited on output value: from 0001-01-01 to 9999-12-31. Java has no limits.


// The function has 2 arguments, testing NULL and MISSING in all position(s)
var combinations = permuteArgumentsWith(2, nullRef, DSL.literal(42.));
for (var args : combinations) {
assertEquals(nullValue(), eval(makedate(args[0], args[1])));
}
combinations = permuteArgumentsWith(2, missingRef, DSL.literal(42.));
for (var args : combinations) {
assertEquals(missingValue(), eval(makedate(args[0], args[1])));
}
}

/**
* Combines permutations of @size args with @exprToPermute placed in all possible position(s),
* the rest are filled with @exprToFill.
* @param size Size of the array to permute
* @param exprToPermute Expression to permute, usually invalid one to test
* @param exprToFill Expression to fill the rest place, usually valid one
* @return A collection with all possible permutations
*/
private Set<Expression[]> permuteArgumentsWith(int size,
Copy link

Choose a reason for hiding this comment

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

The tests would be easier to understand if the permutations tested were listed.

Since this function is only used with size of 2 and 3, I'd replace it with a function that just uses List.of(...) and lists all the permutations.

Expression exprToPermute, Expression exprToFill) {
Set<Expression[]> combinations = new HashSet<>();
for (int i = 1; i < 1 << size; i++) {
var args = new Expression[size];
for (int j = 0; j < size; j++) {
if ((i & (1 << j)) > 0) {
args[j] = exprToPermute;
} else {
args[j] = exprToFill;
}
}
combinations.add(args);
}
return combinations;
}

void testDateFormat(DateFormatTester dft) {
FunctionExpression expr = dft.getDateFormatExpression();
assertEquals(STRING, expr.type());
Expand Down
47 changes: 46 additions & 1 deletion docs/user/dql/functions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,15 +1219,60 @@ Example::
+---------------------------+


MAKEDATE
--------

Description
>>>>>>>>>>>

Returns a date, given `year` and `day-of-year` values. `dayofyear` must be greater than 0 or the result is `NULL`. The result is also `NULL` if either argument is `NULL`.
Arguments are rounded to an integer.
Copy link

Choose a reason for hiding this comment

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

Was there a test case for this rounding behaviour in DateTimeFunctionTest?

Copy link

Choose a reason for hiding this comment

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

What about invalid positive numbers? such as dayofyear 999?

Copy link
Author

Choose a reason for hiding this comment

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

Was there a test case for this rounding behaviour in DateTimeFunctionTest?

Thanks, I will add.

What about invalid positive numbers? such as dayofyear 999?

There is no limit! Freedom!


Specifications:

1. MAKEDATE(DOUBLE, DOUBLE) -> DATE

Argument type: DOUBLE

Return type: DATE

Example::

os> select MAKEDATE(1945, 5.9) as f1, MAKEDATE(1984, 1984) as f2
fetched rows / total rows = 1/1
+------------+------------+
| f1 | f2 |
|------------+------------|
| 1945-01-06 | 1989-06-06 |
Copy link

Choose a reason for hiding this comment

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

If sample code is going to show that MAKEDATE(1984, 1984) is 1989-... the description should describe this behaviour as well.

This is also a good test case to have in the unit tests -- that MAKEDATE($year, $days_in_year + 1) == MAKEDATE($year + 1, 1).

+------------+------------+


MAKETIME
--------

Description
>>>>>>>>>>>

Returns a time value calculated from the hour, minute, and second arguments. Returns `NULL` if any of its arguments are `NULL`.
The second argument can have a fractional part, rest arguments are rounded to an integer.
Copy link

Choose a reason for hiding this comment

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

Is the fractional part rounded to a particular number of decimal places?

Copy link

Choose a reason for hiding this comment

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

What about hour > 24? What about minute/second > 60?
Make sure to indicate 24 hour time is used, not 12-hour with am/pm.


Specifications:

1. MAKETIME(INTEGER, INTEGER, INTEGER) -> DATE
1. MAKETIME(DOUBLE, DOUBLE, DOUBLE) -> TIME

Argument type: DOUBLE

Return type: TIME

Example::

os> select MAKETIME(20, 30, 40) as f1, MAKETIME(20.2, 49.5, 42.100502) as f2
fetched rows / total rows = 1/1
+----------+-----------------+
| f1 | f2 |
|----------+-----------------|
| 20:30:40 | 20:50:42.100502 |
+----------+-----------------+


MICROSECOND
Expand Down
47 changes: 46 additions & 1 deletion docs/user/ppl/functions/datetime.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,15 +388,60 @@ Example::
+--------------------------+


MAKEDATE
Copy link

Choose a reason for hiding this comment

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

Same comments as in SQL apply here.

--------

Description
>>>>>>>>>>>

Returns a date, given `year` and `day-of-year` values. `dayofyear` must be greater than 0 or the result is `NULL`. The result is also `NULL` if either argument is `NULL`.
Arguments are rounded to an integer.

Specifications:

1. MAKEDATE(DOUBLE, DOUBLE) -> DATE

Argument type: DOUBLE

Return type: DATE

Example::

os> source=people | eval f1 = MAKEDATE(1945, 5.9), f2 = MAKEDATE(1984, 1984) | fields f1, f2
fetched rows / total rows = 1/1
+------------+------------+
| f1 | f2 |
|------------+------------|
| 1945-01-06 | 1989-06-06 |
+------------+------------+


MAKETIME
--------

Description
>>>>>>>>>>>

Returns a time value calculated from the hour, minute, and second arguments. Returns `NULL` if any of its arguments are `NULL`.
The second argument can have a fractional part, rest arguments are rounded to an integer.

Specifications:

1. MAKETIME(INTEGER, INTEGER, INTEGER) -> DATE
1. MAKETIME(DOUBLE, DOUBLE, DOUBLE) -> TIME

Argument type: DOUBLE

Return type: TIME

Example::

os> source=people | eval f1 = MAKETIME(20, 30, 40), f2 = MAKETIME(20.2, 49.5, 42.100502) | fields f1, f2
fetched rows / total rows = 1/1
+----------+-----------------+
| f1 | f2 |
|----------+-----------------|
| 20:30:40 | 20:50:42.100502 |
+----------+-----------------+


MICROSECOND
Expand Down
Loading