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.

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

SQL: Introduce INTERVAL support #35521

Merged
merged 9 commits into from Nov 21, 2018
Merged

SQL: Introduce INTERVAL support #35521

merged 9 commits into from Nov 21, 2018

Conversation

costin
Copy link
Member

@costin costin commented Nov 13, 2018 •
edited

Sorry for the big PR. It had rippling effects through the code base.

Introduce INTERVAL as a DataType
Add INTERVAL to the grammar which supports the standard SQL declaration
(without precision):

INTERVAL '1 23:45:01.123456789' DAY TO SECOND

but also number for single unit intervals:

INTERVAL 1 YEAR

as well as the plurals of the units:

INTERVAL 2 YEARS

Interval are internally supported as just another Literal being backed
by java.time.Period and java.time.Duration
Move JDBC away from JDBCType enum to SQLType interface
Refactor DataType by:

  • move it into server core
  • add dedicated (and much simpler) JDBC type
    Improve internal JDBC conversion by normalizing on the DataType
    Rename JDBC columnInfo to JdbcColumnInfo to differentiate between it and
    the SQL ColumnInfo
    Extended arithmetic operations to support operations between intervals and intervals and dates
    (underlying folding operation not implemented yet)

Left to do:

  1. add basic operation +/- between intervals
  2. add basic operation +/- between timestamps and intervals
  3. introduce now/current date/current time/current timestamp methods (likely another PR)

Fix #29990

@costin
SQL: Introduce INTERVAL support
ba436a7 
Introduce INTERVAL as a DataType
Add INTERVAL to the grammar which supports the standard SQL declaration
 (without precision):
 > INTERVAL '1 23:45:01.123456789' DAY TO SECOND
 but also number for single unit intervals:
 > INTERVAL 1 YEAR
 as well as the plurals of the units:
 > INTERVAL 2 YEARS
Interval are internally supported as just another Literal being backed
 by java.time.Period and java.time.Duration
Move JDBC away from JDBCType enum to SQLType interface
Refactor DataType by:
 - move it into server core
 - add dedicated (and much simpler) JDBC type
Improve internal JDBC conversion by normalizing on the DataType
Rename JDBC columnInfo to JdbcColumnInfo to differentiate between it and
 the SQL ColumnInfo

Left to do:

4. add basic operation +/- between intervals
5. add basic operation +/- between timestamps and intervals
6. add conversion of intervals and other datatypes (see what functions
make sense)
7. introduce now/current date/current time/current timestamp methods

Fix elastic#29990
@costin
Loosen type compatibility
beae069
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

astefan
astefan reviewed Nov 15, 2018
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

Impressive. Left some minor/picky comments.

...lugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/JdbcHttpClient.java Outdated Show resolved Hide resolved
...lugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java Outdated Show resolved Hide resolved
...in/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ColumnInfoTests.java Outdated Show resolved Hide resolved
...in/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ColumnInfoTests.java Outdated Show resolved Hide resolved
...in/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ColumnInfoTests.java Outdated Show resolved Hide resolved
...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtils.java Outdated Show resolved Hide resolved
...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtils.java Outdated Show resolved Hide resolved
...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtils.java Outdated Show resolved Hide resolved
...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtils.java Outdated Show resolved Hide resolved
...gin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtilsTests.java Outdated Show resolved Hide resolved
@costin costin mentioned this pull request Nov 16, 2018
Merged
matriv
matriv reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Left some comments, but still reviewing.

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcResultSet.java Show resolved Hide resolved
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java Show resolved Hide resolved
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java Outdated Show resolved Hide resolved
matriv
matriv reviewed Nov 19, 2018
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Finished review, left some more comments/questions. Really nice work!!

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java Outdated Show resolved Hide resolved
...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/type/ExtendedJDBCType.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java Show resolved Hide resolved
...gin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtilsTests.java Outdated Show resolved Hide resolved
@costin
Add integration tests plus scripting
3d62531 
Fix datetime formatting
@costin costin removed the WIP label Nov 20, 2018
matriv
matriv reviewed Nov 21, 2018
View reviewed changes
...org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
@@ -146,27 +152,27 @@ public static Boolean regex(String value, String pattern) {
//
// Math
//
public static Number add(Number left, Number right) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you switch to object? We should have already validated they are Numbers or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ignore, I rushed, it's because of numeric operations between intervals.

matriv
matriv reviewed Nov 21, 2018
View reviewed changes
...sticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java Outdated
return Arithmetics.add((ZonedDateTime) r, ((IntervalDayTime) l).interval());
}

throw new SqlIllegalArgumentException("Cannot compute [+] between [{}] [{}]", l.getClass(), r.getClass());
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 a test for this case.

matriv
matriv reviewed Nov 21, 2018
View reviewed changes
...sticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java Outdated
throw new SqlIllegalArgumentException("Cannot substract a date from an interval; do you mean the reverse?");
}

throw new SqlIllegalArgumentException("Cannot compute [-] between [{}] [{}]", l.getClass(), r.getClass());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

matriv
matriv approved these changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM. Nice! (Added two pointers to add some test for Exception throwing)

@costin
Copy link
Member Author

costin commented Nov 21, 2018

test this please

@costin costin merged commit f0a3d32 into elastic:master Nov 21, 2018
@costin costin deleted the introduce-sql-interval branch November 21, 2018 13:46
costin added a commit that referenced this pull request Nov 21, 2018
@costin
SQL: Introduce INTERVAL support (#35521)
9301b46 
Introduce INTERVAL as a DataType
Add INTERVAL to the grammar which supports the standard SQL declaration
 (without precision):
 > INTERVAL '1 23:45:01.123456789' DAY TO SECOND
 but also number for single unit intervals:
 > INTERVAL 1 YEAR
 as well as the plurals of the units:
 > INTERVAL 2 YEARS
Interval are internally supported as just another Literal being backed
 by java.time.Period and java.time.Duration
Move JDBC away from JDBCType enum to SQLType interface
Refactor DataType by moving it into server core and adding dedicated (and
 much simpler) JDBC driver type
Improve internal JDBC conversion by normalizing on the DataType
Rename JDBC columnInfo to JdbcColumnInfo to differentiate between it and
 the SQL ColumnInfo

Fix #29990

(cherry picked from commit f0a3d32)
@imotov imotov mentioned this pull request Nov 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan left review comments

@matriv matriv matriv approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >feature v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@costin @elasticmachine @astefan @matriv @colings86