Add Day_Of_Week Function As An Alias Of DayOfWeek #190

GabeFernandez310 · 2022-12-15T23:11:38Z

Description

Adds support for the day_of_week function as an alias for the dayofweek function which currently exists in opensearch

Issues Resolved

#722

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: GabeFernandez310 <[email protected]>

codecov · 2022-12-15T23:19:12Z

Codecov Report

Merging #190 (20150cf) into integ-add-day_of_week-function (c923e80) will decrease coverage by 2.49%.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##             integ-add-day_of_week-function     #190      +/-   ##
====================================================================
- Coverage                             98.31%   95.81%   -2.50%     
- Complexity                             3521     3524       +3     
====================================================================
  Files                                   342      352      +10     
  Lines                                  8700     9365     +665     
  Branches                                554      673     +119     
====================================================================
+ Hits                                   8553     8973     +420     
- Misses                                  142      334     +192     
- Partials                                  5       58      +53

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

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

Impacted Files	Coverage Δ
...c/main/java/org/opensearch/sql/expression/DSL.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%> (ø)`
workbench/public/application.tsx	`0.00% <0.00%> (ø)`
workbench/public/components/Header/Header.tsx	`100.00% <0.00%> (ø)`
workbench/public/components/app.tsx	`0.00% <0.00%> (ø)`
...h/public/components/QueryLanguageSwitch/Switch.tsx	`85.71% <0.00%> (ø)`
...ublic/components/QueryResults/QueryResultsBody.tsx	`68.32% <0.00%> (ø)`
workbench/public/utils/PanelWrapper.tsx	`100.00% <0.00%> (ø)`
...ch/public/components/QueryResults/QueryResults.tsx	`61.60% <0.00%> (ø)`
... and 3 more

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

Signed-off-by: GabeFernandez310 <[email protected]>

docs/user/dql/functions.rst

Signed-off-by: GabeFernandez310 <[email protected]>

MitchellGale · 2022-12-19T19:52:59Z

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

+    testDayOfWeekWithUnderscoress(DSL.day_of_week(DSL.literal("2021-02-28")), 1);
+
+    //Feb. 29 of a non-leap year
+    assertThrows(SemanticCheckException.class, () ->  testInvalidDayOfWeek("2021-02-29"));


MitchellGale · 2022-12-19T19:56:24Z

docs/user/dql/functions.rst

+    | 4                                              |
+    +------------------------------------------------+
+
+    os> SELECT DAY_OF_WEEK(TIMESTAMP('2020-08-26 00:00:00'))


consider having both dayofweek and day_of_week on the same line to make comparisons easier to show equivalence.

eg. SELECT DAYOFWEEK(DATETIME('2020-08-26 00:00:00')), DAY_OF_WEEK(DATETIME('2020-08-26 00:00:00'));

also consider removing a lot of these from the documentation. It's a little cumbersome and the variants are already tested in DateTimeFunctionIT. I'd just include one example for DAYOFWEEK and one example of DAY_OF_WEEK or combine them like Mitchell suggested.

Fixed in e571bf0

acarbonetto · 2022-12-19T21:07:48Z

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

@@ -458,6 +458,78 @@ public void dayOfWeek() {
    assertEquals(integerValue(1), eval(expression));
  }

+  public void testDayOfWeekWithUnderscoress(FunctionExpression dateExpression, int dayOfWeek) {


Fixed in f044830

acarbonetto · 2022-12-19T21:07:56Z

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

+    );
+  }
+
+  public void testInvalidDayOfWeek(String date) {


Fixed in f044830

acarbonetto · 2022-12-19T21:12:52Z

docs/user/dql/functions.rst

+    | 4                                              |
+    +------------------------------------------------+
+
+    os> SELECT DAY_OF_WEEK(TIMESTAMP('2020-08-26 00:00:00'))


also consider removing a lot of these from the documentation. It's a little cumbersome and the variants are already tested in DateTimeFunctionIT. I'd just include one example for DAYOFWEEK and one example of DAY_OF_WEEK or combine them like Mitchell suggested.

acarbonetto · 2022-12-19T21:14:36Z

sql/src/main/antlr/OpenSearchSQLParser.g4

@@ -244,6 +244,7 @@ datetimeConstantLiteral
    : CURRENT_DATE
    | CURRENT_TIME
    | CURRENT_TIMESTAMP
+    | DAY_OF_WEEK


I don't understand why we don't also need DAYOFWEEK defined here.

Same as #195 (comment)

Fixed in e571bf0

Yury-Fridlyand

mysql> select dayofweek(curtime());
+----------------------+
| dayofweek(curtime()) |
+----------------------+
|                    2 |
+----------------------+
1 row in set (0.00 sec)

Function could accept TIME arg too and use FunctionProperties to resolve date from it.

Yury-Fridlyand · 2022-12-19T23:00:06Z

sql/src/main/antlr/OpenSearchSQLParser.g4

@@ -244,6 +244,7 @@ datetimeConstantLiteral
    : CURRENT_DATE
    | CURRENT_TIME
    | CURRENT_TIMESTAMP
+    | DAY_OF_WEEK


Same as #195 (comment)

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 · 2022-12-21T22:54:46Z

mysql> select dayofweek(curtime());
+----------------------+
| dayofweek(curtime()) |
+----------------------+
|                    2 |
+----------------------+
1 row in set (0.00 sec)
Function could accept TIME arg too and use FunctionProperties to resolve date from it.

This has been implemented in 14279ed, although I am not sure if it completely follows the behaviour you were looking for. Your query of dayofweek(curtime()) or dayofweek(time('12:23:34')) now works, but a query such as dayofweek('12:23:34') does not work since the argument is of type STRING instead of the type TIME.

Signed-off-by: GabeFernandez310 <[email protected]>

acarbonetto · 2022-12-29T21:13:27Z

docs/user/dql/functions.rst

@@ -1434,20 +1434,21 @@ Description

 Usage: dayofweek(date) returns the weekday index for date (1 = Sunday, 2 = Monday, …, 7 = Saturday).

+The `day_of_week` function is also provided as an alias.


do we need an underscore to use this as an alias?

Yes you do need the underscores. Both dayofweek and day_of_week work, but the version with the underscores is an alias to dayofweek which already exists and is implemented. However, day_ofweek and dayof_week do not work if that is what you are asking.

Yury-Fridlyand · 2023-01-04T00:03:51Z

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

+        implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(
+            functionProperties.getQueryStartClock()), INTEGER, TIME),


Suggested change

implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(

functionProperties.getQueryStartClock()), INTEGER, TIME),

implWithProperties(nullMissingHandlingWithProperties(functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(

functionProperties.getQueryStartClock())), INTEGER, TIME),

And all related changes

Works as is without needing to checkout files from other branches. Fixed in 8daa4ba

Yury-Fridlyand · 2023-01-04T00:04:37Z

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

@@ -771,7 +778,7 @@ private ExprValue exprDayOfMonth(ExprValue date) {
  /**
   * Day of Week implementation for ExprValue.
   *
-   * @param date ExprValue of Date/String type.
+   * @param date ExprValue of Date/Datetime/String type.


and time and timestamp

Added Timestamp. Added javadoc comment to dayOfWeekToday since this is used instead for the TIME type. Done in 8daa4ba.

Yury-Fridlyand · 2023-01-04T00:36:43Z

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

-    FunctionExpression expression = DSL.dayofweek(DSL.literal(new ExprDateValue("2020-08-07")));
+    FunctionExpression expression = DSL.dayofweek(
+        functionProperties,
+        DSL.literal(new ExprDateValue("2020-08-07")));


Try @ParametrizedTest and/or split into different tests and/or use assertAll

Fixed in 20150cf

Signed-off-by: GabeFernandez310 <[email protected]>

…project#1228) Added Implementation And Testing For Day_Of_Week Function Signed-off-by: GabeFernandez310 <[email protected]> Signed-off-by: GabeFernandez310 <[email protected]>

…project#1228) (opensearch-project#1239) Added Implementation And Testing For Day_Of_Week Function Signed-off-by: GabeFernandez310 <[email protected]> Signed-off-by: GabeFernandez310 <[email protected]> (cherry picked from commit bac9c37) Co-authored-by: GabeFernandez310 <[email protected]>

GabeFernandez310 added 3 commits December 15, 2022 09:05

Added Tests

7832301

Signed-off-by: GabeFernandez310 <[email protected]>

Added Implementation

4274273

Signed-off-by: GabeFernandez310 <[email protected]>

Added Documentation

4a6f083

Signed-off-by: GabeFernandez310 <[email protected]>

Fixed Checkstyle Issues

4853304

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 marked this pull request as ready for review December 16, 2022 15:40

GabeFernandez310 requested a review from a team December 16, 2022 16:10

MitchellGale reviewed Dec 16, 2022

View reviewed changes

docs/user/dql/functions.rst Outdated Show resolved Hide resolved

Added Doctests

9a061ea

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 requested review from MitchellGale and a team December 19, 2022 19:46

MitchellGale reviewed Dec 19, 2022

View reviewed changes

MitchellGale approved these changes Dec 19, 2022

View reviewed changes

MitchellGale requested review from acarbonetto, forestmvey, GumpacG, margarit-h, MaxKsyunz and Yury-Fridlyand December 19, 2022 19:57

acarbonetto approved these changes Dec 19, 2022

View reviewed changes

Yury-Fridlyand reviewed Dec 19, 2022

View reviewed changes

Yury-Fridlyand requested changes Dec 19, 2022

View reviewed changes

GumpacG approved these changes Dec 20, 2022

View reviewed changes

GabeFernandez310 added 2 commits December 21, 2022 08:14

Fixed Function Names And Public Functions

f044830

Signed-off-by: GabeFernandez310 <[email protected]>

Fixed Parser and Documentation

e571bf0

Signed-off-by: GabeFernandez310 <[email protected]>

Yury-Fridlyand approved these changes Dec 21, 2022

View reviewed changes

Added Support For Time Type

14279ed

Signed-off-by: GabeFernandez310 <[email protected]>

Added Basic Test For Time Type

0f08311

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 added 2 commits December 21, 2022 16:37

Fixed Unit Test

0db6299

Signed-off-by: GabeFernandez310 <[email protected]>

Fixed Checkstyle

2364e68

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 requested a review from acarbonetto December 22, 2022 16:21

Altered Implementation And Test For Time Type

996d300

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 requested a review from Yury-Fridlyand December 27, 2022 18:35

acarbonetto approved these changes Dec 29, 2022

View reviewed changes

Yury-Fridlyand reviewed Jan 4, 2023

View reviewed changes

GabeFernandez310 added 2 commits January 4, 2023 15:42

Added Support For Null And Missing Args, Fixed Comments

8daa4ba

Signed-off-by: GabeFernandez310 <[email protected]>

Refactored Tests

20150cf

Signed-off-by: GabeFernandez310 <[email protected]>

GabeFernandez310 merged commit cc7d920 into integ-add-day_of_week-function Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Day_Of_Week Function As An Alias Of DayOfWeek #190

Add Day_Of_Week Function As An Alias Of DayOfWeek #190

GabeFernandez310 commented Dec 15, 2022

codecov bot commented Dec 15, 2022 •

edited

Loading

MitchellGale Dec 19, 2022

MitchellGale Dec 19, 2022

acarbonetto Dec 19, 2022

GabeFernandez310 Dec 21, 2022

acarbonetto Dec 19, 2022

GabeFernandez310 Dec 21, 2022

acarbonetto Dec 19, 2022

GabeFernandez310 Dec 21, 2022

acarbonetto Dec 19, 2022

acarbonetto Dec 19, 2022

Yury-Fridlyand Dec 19, 2022

GabeFernandez310 Dec 21, 2022

Yury-Fridlyand left a comment

Yury-Fridlyand Dec 19, 2022

GabeFernandez310 commented Dec 21, 2022

acarbonetto Dec 29, 2022

GabeFernandez310 Dec 29, 2022 •

edited

Loading

Yury-Fridlyand Jan 4, 2023

Yury-Fridlyand Jan 4, 2023

GabeFernandez310 Jan 4, 2023

Yury-Fridlyand Jan 4, 2023

GabeFernandez310 Jan 4, 2023

Yury-Fridlyand Jan 4, 2023

GabeFernandez310 Jan 5, 2023

		@@ -1434,20 +1434,21 @@ Description

		Usage: dayofweek(date) returns the weekday index for date (1 = Sunday, 2 = Monday, …, 7 = Saturday).

		The `day_of_week` function is also provided as an alias.

		implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfWeekToday(
		functionProperties.getQueryStartClock()), INTEGER, TIME),

Add Day_Of_Week Function As An Alias Of DayOfWeek #190

Add Day_Of_Week Function As An Alias Of DayOfWeek #190

Conversation

GabeFernandez310 commented Dec 15, 2022

Description

Issues Resolved

Check List

codecov bot commented Dec 15, 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

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabeFernandez310 commented Dec 21, 2022

Choose a reason for hiding this comment

GabeFernandez310 Dec 29, 2022 • edited Loading

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

codecov bot commented Dec 15, 2022 •

edited

Loading

GabeFernandez310 Dec 29, 2022 •

edited

Loading