SQL: Introduce INTERVAL support by costin · Pull Request #35521 · elastic/elasticsearch

costin · 2018-11-13T22:16:22Z

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:

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

Fix #29990

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

elasticmachine · 2018-11-13T22:16:24Z

Pinging @elastic/es-search-aggs

astefan

Impressive. Left some minor/picky comments.

...lugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/net/client/JdbcHttpClient.java

...lugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java

...in/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/net/protocol/ColumnInfoTests.java

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtils.java

...gin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtilsTests.java

…-sql-interval

matriv

Left some comments, but still reviewing.

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcResultSet.java

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java

matriv

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

...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/type/ExtendedJDBCType.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

...gin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/literal/IntervalUtilsTests.java

Fix datetime formatting

…-sql-interval

matriv · 2018-11-21T00:29:35Z

...org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java

    //
    // Math
    //
-    public static Number add(Number left, Number right) {


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

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

matriv · 2018-11-21T00:37:11Z

...sticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java

+                return Arithmetics.add((ZonedDateTime) r, ((IntervalDayTime) l).interval());
+            }
+
+            throw new SqlIllegalArgumentException("Cannot compute [+] between [{}] [{}]", l.getClass(), r.getClass());


Please add a test for this case.

matriv · 2018-11-21T00:37:24Z

...sticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java

+                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());


matriv

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

costin · 2018-11-21T12:10:40Z

test this please

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)

costin added 2 commits November 14, 2018 00:12

Loosen type compatibility

beae069

costin added >feature WIP v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Nov 13, 2018

costin requested review from astefan and matriv November 13, 2018 22:16

astefan reviewed Nov 15, 2018

View reviewed changes

Polishing

d1d99ee

costin mentioned this pull request Nov 16, 2018

SQL: Move internals from Joda to java.time #35649

Merged

costin added 2 commits November 18, 2018 18:30

Merge remote-tracking branch 'remotes/upstream/master' into introduce…

85b5fe7

…-sql-interval

Merge branch 'master' into introduce-sql-interval

fd74694

matriv reviewed Nov 19, 2018

View reviewed changes

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java Show resolved Hide resolved

matriv reviewed Nov 19, 2018

View reviewed changes

Add integration tests plus scripting

3d62531

Fix datetime formatting

costin force-pushed the introduce-sql-interval branch from 9fc8f29 to 3d62531 Compare November 20, 2018 17:33

costin removed the WIP label Nov 20, 2018

costin added 2 commits November 20, 2018 23:51

Polishing

ec43c41

Merge remote-tracking branch 'remotes/upstream/master' into introduce…

01fd56d

…-sql-interval

matriv reviewed Nov 21, 2018

View reviewed changes

matriv approved these changes Nov 21, 2018

View reviewed changes

Address feedback

0e3b182

costin merged commit f0a3d32 into elastic:master Nov 21, 2018

costin deleted the introduce-sql-interval branch November 21, 2018 13:46

imotov mentioned this pull request Nov 21, 2018

SQL: Add Misc Geometry Property functions #35698

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

costin commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 13, 2018

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

matriv Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

matriv Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

matriv Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

costin commented Nov 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

costin commented Nov 13, 2018 •

edited

Loading