Skip to content
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 TIME_FORMAT() Function To SQL Plugin #1301

Merged

Conversation

GabeFernandez310
Copy link
Contributor

@GabeFernandez310 GabeFernandez310 commented Jan 27, 2023

Signed-off-by: GabeFernandez310 [email protected]

Description

Adds the time_format function to the SQL plugin. The function takes a DATE/DATETIME/TIME/TIMESTAMP/STRING in a valid format, and a formatting string (e.g., "%h %p") and outputs a formatted string. If a DATE is passed in, it treats the argument as if it is taken at midnight (i.e., 00:00:00).

Example:

opensearchsql> SELECT time_format("1998-01-31 13:14:15.012345", "%f %H %h %I %i %p %r %S %s %T");
fetched rows / total rows = 1/1
+------------------------------------------------------------------------------+
| time_format("1998-01-31 13:14:15.012345", "%f %H %h %I %i %p %r %S %s %T")   |
|------------------------------------------------------------------------------|
| 012345 13 01 01 14 PM 01:14:15 PM 15 15 13:14:15                             |
+------------------------------------------------------------------------------+

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.

@GabeFernandez310 GabeFernandez310 requested a review from a team as a code owner January 27, 2023 20:52
@Yury-Fridlyand Yury-Fridlyand added enhancement New feature or request backport 2.x labels Jan 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Merging #1301 (efe4407) into main (72547f4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##               main    #1301   +/-   ##
=========================================
  Coverage     98.36%   98.37%           
- Complexity     3643     3680   +37     
=========================================
  Files           343      343           
  Lines          9010     9065   +55     
  Branches        585      585           
=========================================
+ Hits           8863     8918   +55     
  Misses          142      142           
  Partials          5        5           
Flag Coverage Δ
sql-engine 98.37% <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%> (ø)
...sql/expression/datetime/DateTimeFormatterUtil.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%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -636,7 +636,7 @@ public void testDateFormat() throws IOException {
String timestamp = "1998-01-31 13:14:15.012345";
String timestampFormat = "%a %b %c %D %d %e %f %H %h %I %i %j %k %l %M "
+ "%m %p %r %S %s %T %% %P";
String timestampFormatted = "Sat Jan 01 31st 31 31 12345 13 01 01 14 031 13 1 "
String timestampFormatted = "Sat Jan 01 31st 31 31 012345 13 01 01 14 031 13 1 "
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the change? 12345 -> 012345

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For DATE_FORMAT and TIME_FORMAT functions, microseconds should be padded with 0s on the left side according to both MySQL and our documentation (6 digits of precision should always be returned when using %f). This was fixed and the test was changed.

@@ -846,6 +847,12 @@ private DefaultFunctionResolver date_format() {
STRING, DATE, STRING),
impl(nullMissingHandling(DateTimeFormatterUtil::getFormattedDate),
STRING, DATETIME, STRING),
implWithProperties(
Copy link
Collaborator

Choose a reason for hiding this comment

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

update javadoc also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 1af5666

penghuo
penghuo previously approved these changes Jan 31, 2023
dai-chen
dai-chen previously approved these changes Feb 1, 2023
@GabeFernandez310 GabeFernandez310 dismissed stale reviews from dai-chen and penghuo via 31fdec6 February 2, 2023 16:40
@GabeFernandez310 GabeFernandez310 force-pushed the integ-add-time_format-function branch from 31fdec6 to 406ce81 Compare February 2, 2023 16:51
@GabeFernandez310 GabeFernandez310 requested review from penghuo and dai-chen and removed request for penghuo February 2, 2023 17:33
Yury-Fridlyand
Yury-Fridlyand previously approved these changes Feb 2, 2023
@GabeFernandez310 GabeFernandez310 marked this pull request as draft February 2, 2023 18:13
@@ -61,7 +66,7 @@ interface DateTimeFormatHandler {
.put("%D", (date) -> // %w - Day of month with English suffix
String.format("'%d%s'", date.getDayOfMonth(), getSuffix(date.getDayOfMonth())))
.put("%f", (date) -> // %f - Microseconds
String.format("'%d'", (date.getNano() / 1000)))
String.format(NANO_SEC_FORMAT, (date.getNano() / 1000)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love it - but I feel like we should use a constant for all of the surrounding format strings.

acarbonetto
acarbonetto previously approved these changes Feb 7, 2023
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

minor requests

DSL.literal("%h")),
Arguments.of(
DSL.literal("61:11:12"),
DSL.literal("%h"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • nanoseconds test would be good

Arguments.of("%V"),
Arguments.of("%v"),
Arguments.of("%X"),
Arguments.of("%x")
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could also test formats without % or with extra characters like %%h or %hh or wrong order or extra spaces like h% or %h

@GabeFernandez310 GabeFernandez310 marked this pull request as ready for review February 9, 2023 18:21
Copy link
Collaborator

@MaxKsyunz MaxKsyunz left a comment

Choose a reason for hiding this comment

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

@MaxKsyunz MaxKsyunz merged commit bae0f6b into opensearch-project:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants