INTERVAL support, date pseudo literals TIMESTAMP and TIME literals. #255
Conversation
kchasapis
force-pushed
the
interval_date_psedo_literals
branch
from
08fe178 to
d2da311
Compare
src/sql/CreateStatement.h
Outdated
| ColumnType type; | ||
| bool nullable; | ||
| std::vector<ReferencesSpecification*>* references; | ||
| Expr* defaultValue{nullptr}; |
There was a problem hiding this comment.
This member is unused, isn't it?
kchasapis
force-pushed
the
interval_date_psedo_literals
branch
from
d2da311 to
9109fe0
Compare
The interval was not allowed to be a column data type. With this patch we add this support. We add support for the pseudo literals CURRENT_TIME, CURRENT_TIMESTAMP, CURRENT_DATE. Also, we add support for TIMESTAMP and TIME literals. The user can now explicitly set the type of the string value it follows with TIMESTAMP 'value' and TIME 'value. DATE was already supported.
kchasapis
force-pushed
the
interval_date_psedo_literals
branch
from
9109fe0 to
ef49bda
Compare
Thanks for your message. I have added unit tests. Please, let me know if there is anything else needed from my side to get started in the review. |
| kExprCurrentTimestamp, | ||
| kExprCurrentDate, | ||
| kExprCurrentTime, |
There was a problem hiding this comment.
We discussed this internally and we prefer to have only an expression type per date/time/timestamp, not for the "current" version of the literal. Instead, please add a (boolean) flag to the Expr struct indicating whether the literal has the special "current" value.
Mini: The expression types are not really ordered by name. Could you bring them in the order date, time, timestamp, interval?
| static Expr* makeTimestampLiteral(char* val); | ||
|
|
||
| static Expr* makeTimeLiteral(char* val); | ||
|
|
||
| static Expr* makeIntervalLiteral(int64_t duration, DatetimeField unit); | ||
|
|
||
| static Expr* makeCurrentTimestamp(); | ||
|
|
||
| static Expr* makeCurrentDate(); | ||
|
|
||
| static Expr* makeCurrentTime(); |
There was a problem hiding this comment.
Please see my previous comment. The "current" versions of these functions are not required when we always pass a char*. If the pointer is a nullptr, we could automatically set the flag to mark the "current" literal version.
| @@ -1,4 +1,4 @@ | |||
| # This file contains a list of strings that are NOT valid SQL queries. | |||
| # This file contains a list of strings that are valid SQL queries. | |||
| | INTERVAL INTVAL datetime_field { | ||
| $$ = Expr::makeIntervalLiteral($2, $3); | ||
| } |
There was a problem hiding this comment.
Do you have a use case for this version? According to the SQL:2023 standard, only
INTERVAL 'INTVAL' duration_field
would be valid (note the quotation marks). However, we supported some more versions according to Postgres. INTERVAL INTVAL datetime_field does not seem to work in Postgres (see DB Fiddle, statements in comments raise errors).
| current_timestamp : CURRENT_TIMESTAMP { $$ = Expr::makeCurrentTimestamp(); }; | ||
|
|
||
| current_date : CURRENT_DATE { $$ = Expr::makeCurrentDate(); }; | ||
|
|
||
| current_time : CURRENT_TIME { $$ = Expr::makeCurrentTime(); }; |
There was a problem hiding this comment.
Can you please merge these with the regular versions? E.g., merge current_timestamp with timestamp_literal to the following:
timestamp_literal : TIMESTAMP STRING {
$$ = Expr::makeTimestampLiteral($2);
}
| CURRENT_TIMESTAMP {
$$ = Expr::makeTimestampLiteral(nullptr);
};| CURRENT_TIMESTAMP TOKEN(CURRENT_TIMESTAMP) | ||
| CURRENT_DATE TOKEN(CURRENT_DATE) | ||
| CURRENT_TIME TOKEN(CURRENT_TIME) |
There was a problem hiding this comment.
Can you please order lexicographically (date < time < timestamp)?
| %type <expr> expr operand scalar_expr unary_expr binary_expr logic_expr exists_expr extract_expr cast_expr | ||
| %type <expr> function_expr between_expr expr_alias param_expr | ||
| %type <expr> column_name literal int_literal num_literal string_literal bool_literal date_literal interval_literal | ||
| %type <expr> timestamp_literal time_literal current_timestamp current_date current_time |
There was a problem hiding this comment.
(see previous comment) Please remove the current_ versions here.
| isType(kExprLiteralNull) || isType(kExprLiteralDate) || isType(kExprLiteralInterval) || | ||
| isType(kExprLiteralTimestamp) || isType(kExprLiteralTime); |
There was a problem hiding this comment.
The "current" versions are missing here. However, we would suggest that they don't have own types anyways.
| SQLParser::parse( | ||
| "CREATE TABLE interval_table (c_interval INTERVAL )", | ||
| &result); |
There was a problem hiding this comment.
You can use TEST_PARSE_SINGLE_SQL here. This macro also asserts that the statement is valid and that it returns a single statement of the desired type and of the desired class.
| TEST_PARSE_SINGLE_SQL("SELECT * FROM students WHERE graduation_date = CURRENT_DATE;", | ||
| kStmtSelect, SelectStatement, result, stmt); | ||
|
|
||
| ASSERT_TRUE(result.isValid()); |
There was a problem hiding this comment.
pick here: TEST_PARSE_SINGLE_SQL already asserts this.
2 participants
The interval was not allowed to be a column data type. With this patch we add this support.
We add support for the pseudo literals
CURRENT_TIME, CURRENT_TIMESTAMP, CURRENT_DATE.
Also, we add support for TIMESTAMP and TIME literals. The user can now explicitly set the type of the string value it follows with TIMESTAMP 'value' and TIME 'value.
DATE was already supported.