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

Jump to bottom

Add `YEARWEEK` Function To OpenSearch SQL #236

Merged

GabeFernandez310 merged 13 commits into integ-add-yearweek-function from dev-add-yearweek-function

Mar 9, 2023

GabeFernandez310 commented Feb 27, 2023 •

edited

Loading

Description

The yearweek(date [, mode]) function accepts a first argument representing a date, and an optional second argument containing a mode. It extract the year and week of the year from the input argument, and formats it as an integer which is returned. If an argument of type TIME is provided, the current date is used. The mode argument defines a set of rules to decide how to parse the year and week of the year and are based on MySQL.

Example:

opensearchsql> SELECT YEARWEEK('2020-08-26');
fetched rows / total rows = 1/1
+--------------------------+
| YEARWEEK('2020-08-26')   |
|--------------------------|
| 202034                   |
+--------------------------+

Issues Resolved

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.

GabeFernandez310 added 5 commits

February 24, 2023 15:19


          Added Tests

f637b02

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Added Implementation

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Fixed Implementation

5bbb28b

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Added Documentation

e6e324a

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Fixed Tests

9e22a53

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

codecov bot commented Feb 27, 2023 •

edited

Loading

Codecov Report

Merging #236 (c302e05) into integ-add-yearweek-function (259b001) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                      Coverage Diff                       @@
##             integ-add-yearweek-function     #236   +/-   ##
==============================================================
  Coverage                          98.38%   98.39%           
- Complexity                          3693     3703   +10     
==============================================================
  Files                                343      343           
  Lines                               9107     9134   +27     
  Branches                             585      586    +1     
==============================================================
+ Hits                                8960     8987   +27     
  Misses                               142      142           
  Partials                               5        5

Flag	Coverage Δ
sql-engine	`98.39% <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%> (ø)`

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

Yury-Fridlyand reviewed

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved

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

integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeFunctionIT.java Outdated Show resolved Hide resolved

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

GumpacG approved these changes

View reviewed changes


          Addressed PR Comments

b533e4b

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

Yury-Fridlyand reviewed

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/datetime/YearweekTest.java Outdated Show resolved Hide resolved

core/src/test/java/org/opensearch/sql/expression/datetime/YearweekTest.java Outdated Show resolved Hide resolved

core/src/test/java/org/opensearch/sql/expression/datetime/YearweekTest.java Show resolved Hide resolved

GabeFernandez310 added 2 commits

March 1, 2023 08:34


          Cleaned YearweekTest File

a853b6a

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Added Tests and Fixed Docs

7eac2c6

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

Yury-Fridlyand approved these changes

View reviewed changes

GabeFernandez310 marked this pull request as ready for review

March 1, 2023 22:48

GabeFernandez310 added 2 commits

March 3, 2023 15:42


          Merge branch 'opensearch-project:main' into dev-add-yearweek-function

5ee1687


          Added And Modified Unit Tests

f5c480c

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

matthewryanwells approved these changes

View reviewed changes


          Fixed Jacoco In Core

84dec18

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

acarbonetto reviewed

View reviewed changes

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

+                      impl(nullMissingHandling(DateTimeFunction::exprYearweekWithoutMode), INTEGER, TIMESTAMP),
+                      impl(nullMissingHandling(DateTimeFunction::exprYearweekWithoutMode), INTEGER, STRING),
+                      implWithProperties(nullMissingHandlingWithProperties((functionProperties, time, modeArg)
+                          -> DateTimeFunction.yearweekToday(

acarbonetto Mar 6, 2023

is DateTimeFunction. needed ..?

Author

GabeFernandez310 Mar 8, 2023

Fixed in 271bf2d

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

+                 * @param mode is an integer containing the initial mode arg
+                 * @return an integer containing the new mode
+                 */
+                private int convertWeekModeFromMySqlToJava(LocalDate date, int mode) {

acarbonetto Mar 6, 2023

can you move this function under extractYearweek?

acarbonetto Mar 6, 2023

consider naming your parameter modeMySql to make it explicit. And returning modeJava in the comments.
Then you could call the function convertToWeekModeJava

Author

GabeFernandez310 Mar 8, 2023

Moved under extractYearWeek c302e05

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

+                 * @return is a long containing the formatted output for the yearweek function.
+                 */
+                private int extractYearweek(LocalDate date, int mode) {
+                  mode = convertWeekModeFromMySqlToJava(date, mode);

acarbonetto Mar 6, 2023

Create a new var called modeJava. It's best not to update parameters.
Call the input parameter to modeMySql to be explicit.

Author

GabeFernandez310 Mar 8, 2023

Fixed in 271bf2d

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

+                 * @param date ExprValue of Date/Datetime/Time/Timestamp/String type.
+                 * @param mode ExprValue of Integer type.
+                 */
+                private ExprValue exprYearweek(ExprValue date, ExprValue mode) {

acarbonetto Mar 6, 2023

return ExprIntegerValue and you can avoid creating new ExprIntegerValue twice

Author

GabeFernandez310 Mar 8, 2023

Fixed in 271bf2d

acarbonetto reviewed

View reviewed changes

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

+                  if ((0 <= mode && mode <= 4) && (CalendarLookup.getWeekNumber(mode, date) == 0)) {
+                    mode = 2;
+                  } else if ((mode == 5) && (CalendarLookup.getWeekNumber(mode, date) == 0)) {

acarbonetto Mar 8, 2023

you can combine the CalendarLookup.getWeekNumber calls:

if (CalendarLookup.getWeekNumber() == 0) {
  switch (mode) {
    case 0:
    case 1:
    case 2:
    case 3:
    case 4: 
      return 2;
    case 5:
      return 7;
      break;
    default:  //no-op
  }
  return mode;
}

Author

GabeFernandez310 Mar 8, 2023 •

edited

Loading

Cannot do it this way due to code coverage. Since the minimum number CalendarLookup.getWeekNumber() returns is 1 for case 6 and 7 (and also 3 which is a bug in my original code) it will never enter the if-statement and the default case is never checked.

Author

GabeFernandez310 Mar 8, 2023

switch (mode) {
    case 0:
    case 1:
    case 2:
    case 3:
    case 4:
      if (CalendarLookup.getWeekNumber() == 0) {
        return 2;
      }
    case 5:
      if (CalendarLookup.getWeekNumber() == 0) {
        return 7;
      }
    break;
    default:  //no-op
  }
  return mode;

Something similar to this would pass though.

acarbonetto Mar 8, 2023

Other options:

      if (CalendarLookup.getWeekNumber() != 0) {
        return mode;
      }
      if (mode <= 4) {
        return 2;
      }
      return 7;

return CalendarLookup.getWeekNumber() != 0 ? mode : 
           mode <= 4 ? 2 : 
           7;

Author

GabeFernandez310 Mar 8, 2023

Fixed in 271bf2d (Thanks @acarbonetto!)

acarbonetto approved these changes

View reviewed changes

GabeFernandez310 added 2 commits

March 8, 2023 11:46


          Reworked convertWeekModeFromMySqlToJava Function

271bf2d

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>


          Refactored To Remove Extra Function

c302e05

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

GabeFernandez310 merged commit 84884c8 into integ-add-yearweek-function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet