Add `maketime` and `makedate` #102

Yury-Fridlyand · 2022-08-09T00:34:31Z

Description

maketime link link
- Supports any values in range 00:00:00.0 - 23:59:59.(9);
- All arguments are double, but hour and minute are rounded (as MySQL works);
- Fraction second part is kept;
- NULL values causes NULL result;
- Unlike MySQL, negative values, and hour > 23 are not supported. Implementation of that requires big changes.
makedate link link
- All arguments are double, but rounded (as MySQL works);
- Zero year interpreted as 2000 (as MySQL works);
- NULL values causes NULL result;
- day < 1 causes NULL result;
- day > 365/366 causes switching to the next year.

Functions have 2 implementations:

easier
faster

I used easier one, but faster one used in unit tests. Could be swapped.

According to MySQL both functions accept double values. Arguments which are not supposed to have fraction parts (e.g. hour, year) are rounded.

TODO

Implementation
Unit tests
Integration tests
Doctests

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

codecov · 2022-08-09T00:52:40Z

Codecov Report

Merging #102 (1083a44) into integ-make-time-n-date (d26cde3) will decrease coverage by 2.89%.
The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##             integ-make-time-n-date     #102      +/-   ##
============================================================
- Coverage                     97.74%   94.84%   -2.90%     
- Complexity                     2858     2908      +50     
============================================================
  Files                           273      287      +14     
  Lines                          7020     7825     +805     
  Branches                        442      571     +129     
============================================================
+ Hits                           6862     7422     +560     
- Misses                          157      349     +192     
- Partials                          1       54      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`97.79% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ensearch/sql/planner/physical/PhysicalPlanDSL.java	`100.00% <ø> (ø)`
.../sql/planner/physical/PhysicalPlanNodeVisitor.java	`100.00% <ø> (ø)`
...ain/java/org/opensearch/sql/analysis/Analyzer.java	`100.00% <100.00%> (ø)`
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <100.00%> (ø)`
...org/opensearch/sql/analysis/HighlightAnalyzer.java	`100.00% <100.00%> (ø)`
...ensearch/sql/expression/ExpressionNodeVisitor.java	`100.00% <100.00%> (ø)`
...opensearch/sql/expression/HighlightExpression.java	`100.00% <100.00%> (ø)`
...arch/sql/expression/datetime/DateTimeFunction.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

MitchellGale · 2022-08-09T16:32:52Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

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


Can the minutes and seconds not be valid at 0?

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)

MaxKsyunz · 2022-08-10T08:06:39Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+    var r = new Random();
+    assertEquals(maketime(20., 30., 40.), LocalTime.of(20, 30, 40));
+
+    for (var ignored : IntStream.range(0, 25).toArray()) {


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.

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.

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

MaxKsyunz · 2022-08-10T08:13:32Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+   * @param exprToFill Expression to fill the rest place, usually valid one
+   * @return A collection with all possible permutations
+   */
+  private Set<Expression[]> permuteArgumentsWith(int size,


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.

MaxKsyunz · 2022-08-10T08:24:48Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+        "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.))),


What about day 366 of a non-leap year?

MaxKsyunz · 2022-08-10T08:26:29Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

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


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

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.

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

MaxKsyunz · 2022-08-10T08:29:13Z

docs/user/dql/functions.rst

+>>>>>>>>>>>
+
+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.


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

What about invalid positive numbers? such as dayofyear 999?

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!

MaxKsyunz · 2022-08-10T08:32:11Z

docs/user/dql/functions.rst

+    +------------+------------+
+    | f1         | f2         |
+    |------------+------------|
+    | 1945-01-06 | 1989-06-06 |


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).

MaxKsyunz · 2022-08-10T08:33:46Z

docs/user/dql/functions.rst

 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.


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

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

MaxKsyunz · 2022-08-10T08:35:36Z

docs/user/ppl/functions/datetime.rst

@@ -388,15 +388,60 @@ Example::
    +--------------------------+


+MAKEDATE


Same comments as in SQL apply here.

MaxKsyunz · 2022-08-10T08:38:07Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

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


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.

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.

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

acarbonetto · 2022-08-10T17:43:38Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

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


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.

acarbonetto · 2022-08-10T17:45:55Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+    var r = new Random();
+    assertEquals(maketime(20., 30., 40.), LocalTime.of(20, 30, 40));
+
+    for (var ignored : IntStream.range(0, 25).toArray()) {


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.

acarbonetto · 2022-08-10T17:47:32Z

docs/user/dql/functions.rst

+>>>>>>>>>>>
+
+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.


What about invalid positive numbers? such as dayofyear 999?

acarbonetto · 2022-08-10T17:48:38Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

+    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.))),
+        "Zero dayOfYear doesn't produce NULL");


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

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

acarbonetto · 2022-08-10T17:50:23Z

docs/user/dql/functions.rst

 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.


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

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

forestmvey · 2022-08-11T16:06:21Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -512,6 +528,28 @@ private ExprValue exprHour(ExprValue time) {
    return new ExprIntegerValue(time.timeValue().getHour());
  }

+  private ExprValue exprMakeDate(ExprValue yearExpr, ExprValue dayOfYearExp) {


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

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

acarbonetto · 2022-08-11T20:02:34Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+    var hour = Math.round(hourExpr.doubleValue());
+    var minute = Math.round(minuteExpr.doubleValue());
+    var second = secondExpr.doubleValue();
+    if (0 > hour || 0 > minute || 0 > second) {


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

acarbonetto · 2022-08-11T20:02:41Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+    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) {


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

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

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

…attern_CI Include feature branch in workflow to trigger CI

Yury-Fridlyand added 2 commits August 4, 2022 14:15

Implementation.

1153097

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Fix maketime and add tests.

e9baac8

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand added 2 commits August 8, 2022 19:10

Add integration tests.

b58e49e

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Add docs.

7af8a4d

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand changed the title ~~[WIP] maketime and makedate~~ Add maketime and makedate Aug 9, 2022

Yury-Fridlyand marked this pull request as ready for review August 9, 2022 02:17

Yury-Fridlyand requested a review from a team August 9, 2022 02:17

MitchellGale reviewed Aug 9, 2022

View reviewed changes

MitchellGale approved these changes Aug 9, 2022

View reviewed changes

MaxKsyunz requested changes Aug 10, 2022

View reviewed changes

acarbonetto requested changes Aug 10, 2022

View reviewed changes

Yury-Fridlyand added 2 commits August 10, 2022 19:31

Rework on tests according to PR feedback.

4e83762

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Update docs.

335cbf8

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

forestmvey reviewed Aug 11, 2022

View reviewed changes

forestmvey approved these changes Aug 11, 2022

View reviewed changes

Yury-Fridlyand added 3 commits August 11, 2022 11:30

Add javadoc.

4dae1ce

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Minor test improvements.

797aa03

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Replace randomly generated test data with hardcoded.

42c3b2b

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

acarbonetto reviewed Aug 11, 2022

View reviewed changes

acarbonetto approved these changes Aug 11, 2022

View reviewed changes

MaxKsyunz approved these changes Aug 12, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into dev-make-time-n-date

1083a44

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand merged commit 1b6877d into integ-make-time-n-date Aug 12, 2022

Yury-Fridlyand deleted the dev-make-time-n-date branch August 12, 2022 18:51

Yury-Fridlyand added a commit that referenced this pull request Aug 12, 2022

Add maketime and makedate (#102)

00a2dca

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand added a commit that referenced this pull request Aug 17, 2022

Add maketime and makedate (#102) (opensearch-project#755)

9f602c3

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Merge pull request #102 from naveentatikonda/include_feature_branch_p…

3fa3259

…attern_CI Include feature branch in workflow to trigger CI

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `maketime` and `makedate` #102

Add `maketime` and `makedate` #102

Yury-Fridlyand commented Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

MitchellGale Aug 9, 2022

Yury-Fridlyand Aug 9, 2022

MaxKsyunz Aug 10, 2022

acarbonetto Aug 10, 2022

Yury-Fridlyand Aug 10, 2022

MaxKsyunz Aug 10, 2022

MaxKsyunz Aug 10, 2022

MaxKsyunz Aug 10, 2022

Yury-Fridlyand Aug 11, 2022

acarbonetto Aug 11, 2022

MaxKsyunz Aug 10, 2022

acarbonetto Aug 10, 2022

Yury-Fridlyand Aug 11, 2022

MaxKsyunz Aug 10, 2022

MaxKsyunz Aug 10, 2022

acarbonetto Aug 10, 2022

MaxKsyunz Aug 10, 2022

MaxKsyunz Aug 10, 2022

acarbonetto Aug 10, 2022

acarbonetto Aug 10, 2022

acarbonetto Aug 10, 2022

acarbonetto Aug 10, 2022

acarbonetto Aug 10, 2022

Yury-Fridlyand Aug 10, 2022

acarbonetto Aug 10, 2022

forestmvey Aug 11, 2022

acarbonetto Aug 11, 2022

acarbonetto Aug 11, 2022

Yury-Fridlyand Aug 11, 2022

		@@ -388,15 +388,60 @@ Example::
		+--------------------------+


		MAKEDATE

Add maketime and makedate #102

Add maketime and makedate #102

Conversation

Yury-Fridlyand commented Aug 9, 2022 • edited Loading

Description

TODO

Check List

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `maketime` and `makedate` #102

Add `maketime` and `makedate` #102

Yury-Fridlyand commented Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading