-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add YEARWEEK
Function To OpenSearch SQL
#236
Add YEARWEEK
Function To OpenSearch SQL
#236
Conversation
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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
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
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
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
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
impl(nullMissingHandling(DateTimeFunction::exprYearweekWithoutMode), INTEGER, TIMESTAMP), | ||
impl(nullMissingHandling(DateTimeFunction::exprYearweekWithoutMode), INTEGER, STRING), | ||
implWithProperties(nullMissingHandlingWithProperties((functionProperties, time, modeArg) | ||
-> DateTimeFunction.yearweekToday( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is DateTimeFunction.
needed ..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 271bf2d
* @param mode is an integer containing the initial mode arg | ||
* @return an integer containing the new mode | ||
*/ | ||
private int convertWeekModeFromMySqlToJava(LocalDate date, int mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this function under extractYearweek
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider naming your parameter modeMySql
to make it explicit. And returning modeJava
in the comments.
Then you could call the function convertToWeekModeJava
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved under extractYearWeek
c302e05
* @return is a long containing the formatted output for the yearweek function. | ||
*/ | ||
private int extractYearweek(LocalDate date, int mode) { | ||
mode = convertWeekModeFromMySqlToJava(date, mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a new var called modeJava. It's best not to update parameters.
Call the input parameter to modeMySql to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 271bf2d
* @param date ExprValue of Date/Datetime/Time/Timestamp/String type. | ||
* @param mode ExprValue of Integer type. | ||
*/ | ||
private ExprValue exprYearweek(ExprValue date, ExprValue mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ExprIntegerValue
and you can avoid creating new ExprIntegerValue
twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 271bf2d
|
||
if ((0 <= mode && mode <= 4) && (CalendarLookup.getWeekNumber(mode, date) == 0)) { | ||
mode = 2; | ||
} else if ((mode == 5) && (CalendarLookup.getWeekNumber(mode, date) == 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other options:
if (CalendarLookup.getWeekNumber() != 0) {
return mode;
}
if (mode <= 4) {
return 2;
}
return 7;
return CalendarLookup.getWeekNumber() != 0 ? mode :
mode <= 4 ? 2 :
7;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 271bf2d (Thanks @acarbonetto!)
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
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:
Issues Resolved
722
Check List
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.