Skip to content

Add Support For TIME Type in "*_OF_YEAR" Functions #199

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
Merged
GabeFernandez310 merged 20 commits into from
Jan 5, 2023
Merged

Add Support For TIME Type in "*_OF_YEAR" Functions #199

GabeFernandez310 merged 20 commits into from
Jan 5, 2023

Conversation

@GabeFernandez310
Copy link

@GabeFernandez310 GabeFernandez310 commented Dec 28, 2022 •
edited
Loading

Description

Adds support for the TIME type in the functions day_of_year, week_of_year, and month_of_year. Uses the current local date to calculate the output of the function.

Examples (Assuming the current date is 2022-12-28):

opensearchsql> SELECT day_of_year(time('12:23:34'));
fetched rows / total rows = 1/1
+---------------------------------+
| day_of_year(time('12:23:34'))   |
|---------------------------------|
| 362                             |
+---------------------------------+

opensearchsql> SELECT week_of_year(time('12:23:34'));
fetched rows / total rows = 1/1
+----------------------------------+
| week_of_year(time('12:23:34'))   |
|----------------------------------|
| 52                               |
+----------------------------------+

opensearchsql> SELECT month_of_year(time('12:23:34'));
fetched rows / total rows = 1/1
+-----------------------------------+
| month_of_year(time('12:23:34'))   |
|-----------------------------------|
| 12                                |
+-----------------------------------+

Issues Resolved

[#722](https://github.com/opensearch-project/sql/issues/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.

@codecov
Copy link

codecov bot commented Dec 28, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (integ-add-support-for-time-type@91ef34d). Click here to learn what that means.
The diff coverage is n/a.

@@                        Coverage Diff                         @@
##             integ-add-support-for-time-type     #199   +/-   ##
==================================================================
  Coverage                                   ?   95.84%           
  Complexity                                 ?     3563           
==================================================================
  Files                                      ?      354           
  Lines                                      ?     9438           
  Branches                                   ?      674           
==================================================================
  Hits                                       ?     9046           
  Misses                                     ?      334           
  Partials                                   ?       58
Flag Coverage Δ
query-workbench 62.76% <0.00%> (?)
sql-engine 98.32% <0.00%> (?)

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

@GabeFernandez310
Fixed Checkstyle Issue
e787dff 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310 GabeFernandez310 marked this pull request as draft December 28, 2022 17:43
@GabeFernandez310 GabeFernandez310 marked this pull request as ready for review December 29, 2022 19:44
@GabeFernandez310 GabeFernandez310 requested a review from a team December 29, 2022 19:44
@GabeFernandez310
Fixed Checkstyle
08608d5 
Signed-off-by: GabeFernandez310 <[email protected]>
acarbonetto
acarbonetto approved these changes Dec 29, 2022
View reviewed changes
@GabeFernandez310
Fixed Naming Of Args And Functions
85c93e6 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310
Added Base Test Class
2926623 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310
Changed Helper Function Names
8eea630 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310
Changed Documentation
eac4b4e 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310
Fixed Checkstyle
4d4afa5 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310 GabeFernandez310 requested a review from a team January 3, 2023 15:34
Yury-Fridlyand
Yury-Fridlyand reviewed Jan 3, 2023
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated
Comment on lines 374 to 375
implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfYearToday(
functionProperties.getQueryStartClock()), INTEGER, TIME),
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
implWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfYearToday(
functionProperties.getQueryStartClock()), INTEGER, TIME),
implWithProperties(nullMissingHandlingWithProperties((functionProperties, arg) -> DateTimeFunction.dayOfYearToday(
functionProperties.getQueryStartClock())), INTEGER, TIME),

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 6465ec8

Yury-Fridlyand
Yury-Fridlyand suggested changes Jan 3, 2023
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated
Comment on lines 449 to 450
implWithProperties((functionProperties, arg) -> DateTimeFunction.monthOfYearToday(
functionProperties.getQueryStartClock()), INTEGER, TIME),
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
implWithProperties((functionProperties, arg) -> DateTimeFunction.monthOfYearToday(
functionProperties.getQueryStartClock()), INTEGER, TIME),
implWithProperties(nullMissingHandlingWithProperties((functionProperties, arg) -> DateTimeFunction.monthOfYearToday(
functionProperties.getQueryStartClock())), INTEGER, TIME),

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 6465ec8

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated
Comment on lines 612 to 614
implWithProperties((functionProperties, arg) -> DateTimeFunction.weekOfYearToday(
DEFAULT_WEEK_OF_YEAR_MODE,
functionProperties.getQueryStartClock()), INTEGER, TIME),
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
implWithProperties((functionProperties, arg) -> DateTimeFunction.weekOfYearToday(
DEFAULT_WEEK_OF_YEAR_MODE,
functionProperties.getQueryStartClock()), INTEGER, TIME),
implWithProperties(nullMissingHandlingWithProperties((functionProperties, arg) -> DateTimeFunction.weekOfYearToday(
DEFAULT_WEEK_OF_YEAR_MODE,
functionProperties.getQueryStartClock())), INTEGER, TIME),

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 6465ec8

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated
Comment on lines 619 to 621
implWithProperties((functionProperties, time, modeArg) -> DateTimeFunction.weekOfYearToday(
modeArg,
functionProperties.getQueryStartClock()), INTEGER, TIME, INTEGER),
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
implWithProperties((functionProperties, time, modeArg) -> DateTimeFunction.weekOfYearToday(
modeArg,
functionProperties.getQueryStartClock()), INTEGER, TIME, INTEGER),
implWithProperties(nullMissingHandlingWithProperties((functionProperties, time, modeArg) -> DateTimeFunction.weekOfYearToday(
modeArg,
functionProperties.getQueryStartClock())), INTEGER, TIME, INTEGER),

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 2517b31

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

private ExprValue dayOfYearToday(Clock clock) {
return new ExprIntegerValue((formatNow(clock).getDayOfYear()));
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

You can avoid extra actions made in formatNow

Suggested change
return new ExprIntegerValue((formatNow(clock).getDayOfYear()));
return new ExprIntegerValue(LocalDateTime.now(clock).getDayOfYear());

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 2517b31

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

private ExprValue weekOfYearToday(ExprValue mode, Clock clock) {
return new ExprIntegerValue(
CalendarLookup.getWeekNumber(mode.integerValue(), formatNow(clock).toLocalDate()));
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
CalendarLookup.getWeekNumber(mode.integerValue(), formatNow(clock).toLocalDate()));
CalendarLookup.getWeekNumber(mode.integerValue(), LocalDateTime.now(clock).toLocalDate()));

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 2517b31

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

private ExprValue monthOfYearToday(Clock clock) {
return new ExprIntegerValue((formatNow(clock).getMonthValue()));
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Suggested change
return new ExprIntegerValue((formatNow(clock).getMonthValue()));
return new ExprIntegerValue(LocalDateTime.now(clock).getMonthValue());

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Fixed in 2517b31

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated
assertEquals(integerValue(0), eval(expression));

expression = DSL.week(DSL.literal("2019-01-05 00:01:00"));
expression = DSL.week(functionProperties, DSL.literal("2019-01-05 00:01:00"));
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Is it possible to split tests or use assertAll?

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Refactored and added tests in 3c843ba, c664af7, and dcbe2e2

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated
testWeek("2000-01-01", 0, 0);
testWeek("2000-01-01", 2, 52);
testWeek("1999-12-31", 0, 52);
weekQuery("2019-01-05", 0, 0);
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

Is it possible to use parametrized test?

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Refactored and added tests in 3c843ba, c664af7, and dcbe2e2

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated
testWeekOfYear("2000-01-01", 0, 0);
testWeekOfYear("2000-01-01", 2, 52);
testWeekOfYear("1999-12-31", 0, 52);
weekOfYearQuery("2019-01-05", 0, 0);
Copy link

@Yury-Fridlyand Yury-Fridlyand Jan 3, 2023

Choose a reason for hiding this comment

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

The same as above - is it possible to split test cases and use parameterized test?

Copy link
Author

@GabeFernandez310 GabeFernandez310 Jan 4, 2023

Choose a reason for hiding this comment

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

Refactored and added tests in 3c843ba, c664af7, and dcbe2e2

@GabeFernandez310
Fixed Checkstyle
2d8049f 
Signed-off-by: GabeFernandez310 <[email protected]>
@GabeFernandez310 GabeFernandez310 merged commit c586115 into integ-add-support-for-time-type Jan 5, 2023
GabeFernandez310 added a commit that referenced this pull request Jan 9, 2023
@GabeFernandez310
Add Support For TIME Type in "*_OF_YEAR" Functions (#199) (opensear…
6e72f18 
…ch-project#1223)

Added Support And Tests For Time Type in day_of_year, week_of_year, month_of_year Functions
Signed-off-by: GabeFernandez310 <[email protected]>
matthewryanwells pushed a commit that referenced this pull request Feb 1, 2023
@opensearch-trigger-bot @GabeFernandez310
Add Support For TIME Type in "*_OF_YEAR" Functions (#199) (opensear…
820c833 
…ch-project#1223) (opensearch-project#1258)

Added Support And Tests For Time Type in day_of_year, week_of_year, month_of_year Functions
Signed-off-by: GabeFernandez310 <[email protected]>
(cherry picked from commit 6e72f18)

Co-authored-by: GabeFernandez310 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acarbonetto acarbonetto acarbonetto approved these changes

+2 more reviewers

@GumpacG GumpacG GumpacG approved these changes

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@GabeFernandez310 @GumpacG @Yury-Fridlyand @acarbonetto