Add Interval Literal #190

dey4ss · 2021-09-10T11:36:37Z

This PR adds SQL date intervals, i.e., supports statements such the following:

SELECT * FROM t 
   WHERE t.date BETWEEN '2021-01-01' AND DATE '2021-01-01' + 30 DAYS

This is achieved by a new type of literal/Expr, with type kExpressionLiteralInterval. It is returned as second operand of the binary plus (or minus, respectivley) expression, where the start date ist the first one. Only checks if duration is a positive integer and the time unit is any of second(s), minute(s), hour(s), day(s), month(s), year(s). Any further sanity checks are in response of the consuming system.

closes #184
closes #178

dey4ss · 2021-09-10T21:29:04Z

Regarding the syntax, cast(... as date) + INTERVAL n days is kind of a random: I've seen cast(... as date) + n days [1] (which is implemented as well), cast(... as date) + interval 'n' days [2], and Postgres supposes cast(.. as date) + interval 'n days'[3]. However, I thougt this to be natural. If there are opinions preferring any of the variations, I'm open to it.

src/parser/flex_lexer.l

dey4ss · 2021-09-13T21:48:47Z

After revisiting the SQL standard and validation with Postgres, both INTERVAL 'n' days and INTERVAL 'n days' seems to be correct (but not INTERVAL n days. I will change the current behavior to apply this.

mweisgut · 2021-09-14T06:50:12Z

I will do the review on Thursday at the latest.

klauck · 2021-09-14T16:39:41Z

src/parser/bison_parser.y

 	|	extract_expr
 	|	cast_expr
 	|	array_expr
+	|	interval_expression


I would suggest to introduce an 'inverval_literal' in ll. 1029 ('literal: [...]') instead. (interval_expression is used in the SQL standard to enforce type compatibility (see http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt p.168 ), which we do not do in the hyrise parser)

I'm just about to change that. I guess Expr::isLiteral() should return true for interval literals, as well? And is there a reason not to include date literals here?

For now, I added isType(kExprLiteralDate) || isType(kExprLiteralInterval) to Expr::isLiteral().

klauck · 2021-09-14T17:09:37Z

After revisiting the SQL standard and validation with Postgres, both INTERVAL 'n' days and INTERVAL 'n days' seems to be correct (but not INTERVAL n days. I will change the current behavior to apply this.

Arrg, the interval syntax is complex and differs depending on the specific database system: Further, note, it is 'n' day, '1 day', and '5 days' (with and without 's').

dey4ss · 2021-09-15T11:08:35Z

Further, note, it is 'n' day, '1 day', and '5 days' (with and without 's').

This ambiguation works now.

mweisgut

In addition to the above comments, it would be helpful if the description of this PR summarizes the different ways to define the intervals, i.e., 'n' day, '1 day', and '5 days'.

src/parser/bison_parser.y

src/parser/flex_lexer.l

mweisgut · 2021-09-16T12:52:54Z

test/queries/queries-bad.sql

+!SELECT * FROM t WHERE a = DATE '2000-01-01' + INTERVAL '30' DAYS;
+!SELECT * FROM t WHERE a = DATE '2000-01-01' + x DAYS;
+!SELECT * FROM t WHERE a = DATE '2000-01-01' + INTERVAL 'x' DAY;
+!SELECT * FROM t WHERE a = DATE '2000-01-01' + INTERVAL '3.3 DAYS';


I'm wondering if the two following statements should be correctly parsed. With this PR, they are successfully parsed. However, they do not make semantic sense, do they?

SELECT * FROM t WHERE a BETWEEN 3 DAYS AND 3 DAYS; SELECT * FROM t WHERE a BETWEEN INTERVAL '3 DAYS' AND INTERVAL '3 DAYS';

Based on briefly checking the SQL specification, I think it should be parsable with the following rules, starting from a between predicate:

- between predicate - row value predicand - row value constructor predicand - common value expression - interval value expression - ...

So the question here would be whether we should let parsing those statements fail or if further sanity checks in Hyrise should reject such statements.

The idea was to pass such checks to Hyrise. Also ensuring that intervals may only be input to '+' or '-' operations etc. would be out of the parser's scope for now. Regarding semantic sense, DB Fiddle suggests that Postgres has no problem with intervals in BETWEEN statements, e.g., SELECT INTERVAL '2 days' BETWEEN INTERVAL '1 day' AND INTERVAL '3 days'evaluates to 'true'.

Agreed. @klauck and I also discussed that and we also think that such semantic checks should be part of Hyrise.

https://sqliteonline.com/ also suggests that Postgres returns true for SELECT INTERVAL '2 days' BETWEEN INTERVAL '1 day' AND INTERVAL '3 days'.

@klauck we talked about introducing the data type INTERVAL [1 - 3]. However, I think it is out of this PR's scope.

[1] https://www.postgresql.org/docs/current/sql-createtable.html
[2] https://www.oracletutorial.com/oracle-basics/oracle-interval/
[3] https://www.ibm.com/docs/en/informix-servers/12.10?topic=types-sql-datetime-interval-data

src/parser/bison_parser.y

mweisgut · 2021-09-16T15:22:52Z

Referencing #184 and #178.

src/util/sqlhelper.cpp

src/parser/bison_parser.y

mweisgut · 2021-09-21T08:28:07Z

Looks good to me. Just one minor change request:
Could you please add SELECT (CAST('2002-5-01' as DATE) + INTERVAL '60 days');, which is the example query of #178 to queries-good.sql? Then, we can close the issue with this issue IMO.

@klauck could you take a final look at this PR?

klauck · 2021-09-21T09:49:13Z

src/sql/Expr.cpp


+Expr* Expr::makeIntervalLiteral(int64_t duration, DatetimeField unit) {
+    Expr* e = new Expr(kExprLiteralInterval);
+    e->name = strdup("INTERVAL");


I am not sure about the use of the name field here (and suggest removing the line (and also in makeExtract and makeCast)). I think the purpose of the name field was storing actual string values, which may differ among different Expression instances (e.g. different string literals) and not the name of the expression type.

Otherwise, all changes look fine for me. Thanks for your revision.

The three name fields are now removed, tests + print methods are adjusted accordingly.

dey4ss added 4 commits September 10, 2021 11:13

add date intervals

c56c9cb

add tests

a82ab35

format

babcfcb

add more examples, avoid memory leaks

f9fa700

Bensk1 requested a review from mweisgut September 13, 2021 07:09

mweisgut reviewed Sep 13, 2021

View reviewed changes

src/parser/flex_lexer.l Outdated Show resolved Hide resolved

remove double token

6644550

dey4ss added 3 commits September 14, 2021 01:16

comply SQL92

820b8a3

format

f988db3

fix typo

850cb6b

klauck reviewed Sep 14, 2021

View reviewed changes

dey4ss added 2 commits September 14, 2021 10:32

make Interval a literal

e9511db

distinguish singular and pluran time units

184938b

cleanup

2362ea7

mweisgut reviewed Sep 16, 2021

View reviewed changes

klauck reviewed Sep 16, 2021

View reviewed changes

src/parser/bison_parser.y Outdated Show resolved Hide resolved

split rules

f0770d5

dey4ss changed the title ~~Add Date Intervals~~ Add Interval Literal Sep 16, 2021

dey4ss commented Sep 16, 2021

View reviewed changes

src/util/sqlhelper.cpp Outdated Show resolved Hide resolved

mweisgut reviewed Sep 20, 2021

View reviewed changes

src/parser/bison_parser.y Outdated Show resolved Hide resolved

feedback

1d1ebae

mweisgut reviewed Sep 20, 2021

View reviewed changes

src/parser/bison_parser.y Show resolved Hide resolved

mweisgut reviewed Sep 20, 2021

View reviewed changes

src/parser/bison_parser.y Show resolved Hide resolved

add test for every time unit

e267dba

dey4ss added 2 commits September 20, 2021 06:01

support gcc

a104e1c

format test

b40d990

add example query

8e868c9

mweisgut approved these changes Sep 21, 2021

View reviewed changes

klauck reviewed Sep 21, 2021

View reviewed changes

remove extract/cast/interval name fields

f4df2e6

mweisgut merged commit f1afb9a into hyrise:master Sep 21, 2021

dey4ss deleted the date-interval branch September 21, 2021 11:51

dey4ss mentioned this pull request Oct 13, 2021

Coding Style #192

Closed

Add Interval Literal #190

Add Interval Literal #190

Uh oh!

Conversation

dey4ss commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dey4ss commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dey4ss commented Sep 13, 2021

Uh oh!

mweisgut commented Sep 14, 2021

Uh oh!

klauck Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

klauck commented Sep 14, 2021

Uh oh!

dey4ss commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweisgut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mweisgut Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweisgut Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

mweisgut Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mweisgut commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mweisgut commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klauck Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

klauck Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

dey4ss Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dey4ss commented Sep 10, 2021 •

edited

Loading

dey4ss commented Sep 10, 2021 •

edited

Loading

dey4ss Sep 14, 2021 •

edited

Loading

dey4ss commented Sep 15, 2021 •

edited

Loading

dey4ss Sep 16, 2021 •

edited

Loading

mweisgut Sep 16, 2021 •

edited

Loading

mweisgut commented Sep 21, 2021 •

edited

Loading

dey4ss Sep 21, 2021 •

edited

Loading