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

Add temporal interval data type and support interval clause as function #687

Merged
merged 12 commits into from
Aug 20, 2020

Conversation

chloe-zh
Copy link
Member

@chloe-zh chloe-zh commented Aug 15, 2020

Issue #, if available:

Description of changes:

Doc: https://github.com/chloe-zh/sql/blob/interval/docs/user/general/datatypes.rst

  • Added INTERVAL data type in the SQL language structure. Syntax: INTERVAL expr unit. The corresponding expression value is ExprIntervalValue that can encapsulate the temporal amount as the property value.

  • Quantity expression expr in interval clause can be any value expression, including the basic numeric values, field expression, functions etc. which has been revealed in the parser.

  • Interval units to support for phase 1: microsecond, second, minute, hour, day, week, month, quarter, year.

  • Added docs for date and time data types.

  • [TODO] Current PR does not contain examples of date and time types in the reference manual yet. These examples will be added in the coming PR for datetime functions where functions date, time, datetime, timestamp and interval are implemented.

Note:

Interval data type maps to TemporalAmount in Java runtime, it has two types of amount Duration subclass that can be mapped with microseconds to days, where all amount would be converted to a value with second as the unit; and another amount Period can be mapped with days to years, where the values would be converted to some value with day as the the unit. This rule is similar to H2: https://www.h2database.com/html/datatypes.html#interval_type

In this PR, these two types are sharing the same syntax, but will differ in the storage method, and is only comparable with the same type of interval. For example, the comparison between INTERVAL 1 week with INTERVAL 1 month will result in 1 week is shorter than 1 month, but an attempt to compare INTERVAL 1 week with INTERVAL 1 minute will fail and throw an exception. Which is also similar to H2 behavior.

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

@chloe-zh chloe-zh self-assigned this Aug 15, 2020
@chloe-zh chloe-zh requested review from penghuo and dai-chen August 17, 2020 22:12
@chloe-zh chloe-zh marked this pull request as ready for review August 17, 2020 22:14
@@ -172,4 +171,8 @@ public static Boolean getBooleanValue(ExprValue exprValue) {
public static ZonedDateTime getDateValue(ExprValue exprValue) {
return exprValue.dateValue();
}

public static TemporalAmount getIntervalValue(ExprValue exprValue) {
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 need this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, i've also added getTimeValue, getDatetimeValue, getTimestampValue in the other PR #694 to stay consistent. We can also remove all these methods including the getDateValue actually

@dai-chen
Copy link
Member

So in this PR the changes regarding parser only covers PPL language?

@chloe-zh
Copy link
Member Author

So in this PR the changes regarding parser only covers PPL language?

Added parser in SQL in the latest update, thanks!

@dai-chen
Copy link
Member

So in this PR the changes regarding parser only covers PPL language?

Added parser in SQL in the latest update, thanks!

Thanks for adding! In this case please add comparison test case to make sure it can work with JDBC driver together.

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.

LGTM

Copy link
Member

@dai-chen dai-chen 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 changes!

# Conflicts:
#	sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilder.java
#	sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilderTest.java
@chloe-zh chloe-zh merged commit e52db25 into opendistro-for-elasticsearch:develop Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants