Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Support date and time functions: date_add, date_sub, day, dayname, dayofweek, dayofyear, from_days, hour, microsecond, minute, month, monthname, quarter, second, subdate, time_to_sec, to_days #746

Merged

Conversation

rupal-bq
Copy link
Contributor

@rupal-bq rupal-bq commented Sep 18, 2020

Issue #709

Description of changes:
Added implementation, unit test, manual IT, and doc updates for following date and time functions:

  • date_add
  • date_sub
  • day
  • dayname
  • dayofweek
  • dayofyear
  • from_days
  • hour
  • microsecond
  • minute
  • month
  • monthname
  • quarter
  • second
  • subdate
  • time_to_sec
  • to_days
  • year

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 338 to 340
(DATE, DATE INTERVAL) -> DATE
(DATE, TIME INTERVAL) -> DATETIME
(DATETIME/TIMESTAMP, INTERVAL) -> DATETIME
Copy link
Contributor

Choose a reason for hiding this comment

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

The format is the doc is not correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you add example for each case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed format & added example for each case.

Description
-----------

Usage: date_add(date, INTERVAL expr unit) adds the time interval expr to date
Copy link
Contributor

Choose a reason for hiding this comment

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

please add other signature, e.g. data_add(date, expression) 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.

done

(DATE, TIME INTERVAL) -> DATETIME
(DATETIME/TIMESTAMP, INTERVAL) -> DATETIME

Synonyms: ADDDATE
Copy link
Contributor

Choose a reason for hiding this comment

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

do we support ADDDATE? if not, remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we support ADDDATE

Comment on lines 112 to 113
impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATE, DATE, INTERVAL),
impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATETIME, DATE, INTERVAL),
Copy link
Contributor

Choose a reason for hiding this comment

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

why the function has same input signature but has different output signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some use cases required different return type. E.g select date_add(date('2020-09-16'), interval 1 hour) should return 2020-09-16 01:00:00 but select date_add(date('2020-09-16'), interval 1 day) should return 2020-09-17. But this was always returning datetime in schema anyway so removed it.

/**
* Specify a start date and add a temporal amount to the date.
* The return type depends on the date type and the interval unit. Detailed supported signatures:
* (DATE, DATETIME/TIMESTAMP, INTERVAL) -> DATETIME
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean (DATE/DATETIME/TIMESTAMP, INTERVAL) -> DATETIME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fixed it.

*
* @param date ExprValue of Date/Datetime/Timestamp type.
* @param expr ExprValue of Interval type, the temporal amount to add.
* @return Date/Datetime resulted from expr added to date.
Copy link
Contributor

Choose a reason for hiding this comment

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

The ExprDatetimeValue only has type DATETIME.

*/
private ExprValue exprDayOfWeek(ExprValue date) {
if (date instanceof ExprStringValue) {
date = new ExprDateValue(date.stringValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

The input value should be final

@penghuo
Copy link
Contributor

penghuo commented Sep 21, 2020

Two questions about String data type.

  1. When the String should be used? is in date_add function, it doesn't accept String as paramater, but in month function it accept String and Date.
  2. Whe the String can be only used to represent DATE?, I see String can be only used to represent DATE, why not applied to DATETIME/TIMESTAMP/TIME?

I think we should have consistent interface for user. There are two options in my mind.

  1. allow user to use String in function which accept DATE/DATETIME/TIMESTAMP/TIME.
    2.only allow use to use String for function like date(), time(), timestamp().

/**
* DAY(DATE). return the day of the month (1-31).
*/
private FunctionResolver day() {
Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. day(date_add("2020-09-21", 1))
this will throw exception because day function can't accept datetime as paramater?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added datetime & timestamp for all functions

@penghuo
Copy link
Contributor

penghuo commented Sep 21, 2020

Please also add test cases for PPL

Rupal Mahajan added 6 commits September 22, 2020 17:03
…ime-functions

# Conflicts:
#	core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/DSL.java
#	core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/datetime/DateTimeFunction.java
#	core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/function/BuiltinFunctionName.java
#	core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/datetime/DateTimeFunctionTest.java
#	docs/user/dql/functions.rst
#	integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/DateTimeFunctionIT.java
#	ppl/src/main/antlr/OpenDistroPPLLexer.g4
#	ppl/src/main/antlr/OpenDistroPPLParser.g4
#	sql/src/main/antlr/OpenDistroSQLParser.g4
@rupal-bq
Copy link
Contributor Author

Please also add test cases for PPL

Added integration tests for PPL

@rupal-bq
Copy link
Contributor Author

Two questions about String data type.

  1. When the String should be used? is in date_add function, it doesn't accept String as paramater, but in month function it accept String and Date.
  2. Whe the String can be only used to represent DATE?, I see String can be only used to represent DATE, why not applied to DATETIME/TIMESTAMP/TIME?

I think we should have consistent interface for user. There are two options in my mind.

  1. allow user to use String in function which accept DATE/DATETIME/TIMESTAMP/TIME.
    2.only allow use to use String for function like date(), time(), timestamp().

added string for all date-time functions.

* @return Datetime resulted from expr added to date.
*/
private ExprValue exprAddDateInterval(ExprValue date, ExprValue expr) {
ExprValue exprValue = new ExprDatetimeValue(date.datetimeValue().plus(expr.intervalValue()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adddate, date_add, date_sub & subdate need to return date/datetime according to calculated result. Here, I have returned date if time is zero for these functions to get correct result. But the return type will be still be datetime in schema because multiple return type can't be added in define function for the same input parameters.

@@ -43,6 +48,40 @@ public String stringValue() {
return value;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add the test cases for these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added test cases for date-time functions in ExprStringValue

Copy link
Contributor

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

@dai-chen dai-chen merged commit 9fa2b78 into opendistro-for-elasticsearch:develop Sep 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants