Support `CREATE TABLE ON UPDATE <expr>` Function #685

CEOJINSUNG · 2022-10-22T05:54:51Z

Resolve #684

merge : commit update

AugustoFKL · 2022-10-29T21:52:16Z

src/parser.rs

@@ -2921,9 +2921,8 @@ impl<'a> Parser<'a> {
        } else if self.parse_keywords(&[Keyword::ON, Keyword::UPDATE])
            && dialect_of!(self is MySqlDialect)


@CEOJINSUNG please add the GenericDialect here.

Thank you for advising good comment

AugustoFKL · 2022-10-29T21:58:26Z

src/ast/ddl.rs

@@ -437,6 +437,7 @@ pub enum ColumnOption {
    DialectSpecific(Vec<Token>),
    CharacterSet(ObjectName),
    Comment(String),
+    OnUpdate(Expr),


Why does it take an expression?

ON UPDATE CURRENT TIMESTAMP is an expression by itself (1). You can take TIMESTAMP [(p)] for the parameter, as long as I'm aware of it.

There's no reason to make it complex to the upstream with expressions, as there's no ON UPDATE column_1, for example.

You could make it OnUpdateCurrentTimestamp(Option<u64>) to get the precision, maybe?

I don't think there are things like ON UPDATE CURRENT TIMESTAMP WITH TIMEZONE exist at all, so only having the precision should be enough.

I tried this with mysql and the syntax is accepted

mysql> CREATE TABLE foo (`modification_time` DATETIME ON UPDATE CURRENT_TIMESTAMP()); Query OK, 0 rows affected (0.01 sec) mysql> show create table foo; +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ | foo | CREATE TABLE `foo` ( `modification_time` datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci | +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec)

Which is very strange to me as the documentation doesn't seem to allow that syntax 🤔
https://dev.mysql.com/doc/refman/8.0/en/create-table.html

reference_definition: REFERENCES tbl_name (key_part,...) [MATCH FULL | MATCH PARTIAL | MATCH SIMPLE] [ON DELETE reference_option] [ON UPDATE reference_option] reference_option: RESTRICT | CASCADE | SET NULL | NO ACTION | SET DEFAULT

@alamb this is a specific syntax from MySQL (1)

The problem is, the documentation states that the ON UPDATE CURRENT_TIMESTAMP is a special expression uniquely. The only possible modification seems to be the precision information ON UPDATE CURRENT_TIMESTAMP(n), which is dependant on the data type for that column.

But this PR seems to expect any expression, which doesn't make sense, and it makes the upstream handle all other possible expressions as errors, which we should do.

[1] : https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html

I understand "expression" issue you are raising. OnUpdateCurrentTimestamp(Option<u64>) is also good proposal which I was thinking about it. But I am confused in "CurrentTimestamp".

https://www.w3schools.com/mysql/func_mysql_current_timestamp.asp

https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html

In this document, CurrentTimestamp belongs to Function and MySQL make it as special expression without using parentheses. In MySQL, it seems to get ON UPDATE . Maybe it would be good to make reservation expression function options which has no parentheses and make users write both CURRENT_TIMESTAMP and CURRENTP_TIMESTAMP()

@CEOJINSUNG I kinda agree, but the function and the expression are not the same.

Either way, maybe you can have an enum there?

Like

pub enum OnUpdateCurrentTimestampInfo { None, Parenthesis, ParenthesisAndPrecision }

Other approach would be something like:

OnUpdateCurrentTimestamp(Option<Option<u64>>)

Where the internal option is the precision, and the external the presence of parenthesis. But seems less idiomatic and a little obscure for me.

@AugustoFKL Thanks for the suggestion But in sqlparse-rs structure, Function belongs to Expr. And I think it is not good structure because there is no scalability. OnUpdateCurrentTimestampInfo looks like only using for MySQL.

There are another functions without parenthesis like DIV, SYSDATE Function.

SELECT 10 DIV 5; // MySQL SELECT SYSDATE FROM DUAL; // ORACLE

An extensible structure that can be freely added to functions used without parentheses such as DIV and SYSDATE is required like below NoArgsFunction.

pub enum NoArgsFunction { CurrentTimpeStamp, DIV, SYSDATE }

Since SQL is used in various places such as MySQL, Oracle, PosgreSQL, etc., OnupdateCurrentTimeStamp does not seem to be a very good structure even for the open-close principle. Therefore, I think it would be better to respond so that elements used without parentheses can be freely added to functions without arguments.

@CEOJINSUNG but can you say that this is a function per se?

To me it looks like a really special case.

coveralls · 2022-10-31T18:38:22Z

Pull Request Test Coverage Report for Build 3366856922

8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
410 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.02%) to 85.97%

Files with Coverage Reduction	New Missed Lines	%
src/ast/value.rs	7	87.5%
src/ast/data_type.rs	13	88.62%
tests/sqlparser_common.rs	54	96.85%
src/parser.rs	336	83.71%

Totals
Change from base Build 3292360592:	0.02%
Covered Lines:	10919
Relevant Lines:	12701

💛 - Coveralls

alamb

I think this PR is ok to go -- thank you @CEOJINSUNG

If possible, can you please answer @AugustoFKL 's question?

alamb · 2022-10-31T18:46:03Z

src/ast/ddl.rs

@@ -437,6 +437,7 @@ pub enum ColumnOption {
    DialectSpecific(Vec<Token>),
    CharacterSet(ObjectName),
    Comment(String),
+    OnUpdate(Expr),


I tried this with mysql and the syntax is accepted

mysql> CREATE TABLE foo (`modification_time` DATETIME ON UPDATE CURRENT_TIMESTAMP()); Query OK, 0 rows affected (0.01 sec) mysql> show create table foo; +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ | foo | CREATE TABLE `foo` ( `modification_time` datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci | +-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec)

Which is very strange to me as the documentation doesn't seem to allow that syntax 🤔
https://dev.mysql.com/doc/refman/8.0/en/create-table.html

reference_definition: REFERENCES tbl_name (key_part,...) [MATCH FULL | MATCH PARTIAL | MATCH SIMPLE] [ON DELETE reference_option] [ON UPDATE reference_option] reference_option: RESTRICT | CASCADE | SET NULL | NO ACTION | SET DEFAULT

alamb

I think this looks good to me and the rationale for a more permissive Expr makes sense -- @AugustoFKL are you ok with it?

AugustoFKL

@alamb actually no. I'm still not convinced about the expression usage.

Would be TOO permissive, making anyone in the upstream that would use it really confused.

alamb · 2022-11-11T21:16:04Z

Marking as a draft until we get a resolution on #685 (review)

alamb · 2022-12-28T13:57:11Z

Given there there are several proposals for this same feature:
#586 from @Sibz (apparently MySQL also supports now() in addition to current_timestamp
#602 from @RainJoe

Would be TOO permissive, making anyone in the upstream that would use it really confused.

While this PR is more general than MySQL would allow I think the distinction of what functions to support in this area should be left to downstream implementations as described in https://github.com/sqlparser-rs/sqlparser-rs#extensible-sql-lexer-and-parser-for-rust

This crate avoids semantic analysis because it varies drastically between dialects and implementations. If you want to do semantic analysis, feel free to use this project as a base

Thus I plan to merge this PR in

CEOJINSUNG and others added 2 commits October 22, 2022 14:28

Merge pull request #1 from sqlparser-rs/main

a7322d4

merge : commit update

feat : OnUpdate Function Implement

c33e776

AugustoFKL suggested changes Oct 29, 2022

View reviewed changes

alamb approved these changes Oct 31, 2022

View reviewed changes

feat : add GenericDialect Options

ddbb5df

CEOJINSUNG requested a review from alamb November 1, 2022 08:49

alamb approved these changes Nov 2, 2022

View reviewed changes

AugustoFKL reviewed Nov 2, 2022

View reviewed changes

alamb marked this pull request as draft November 11, 2022 21:15

alamb marked this pull request as ready for review December 28, 2022 13:57

alamb changed the title ~~Support ON UPDATE Function~~ Support CREATE TABLE ON UPDATE <expr> Function Dec 28, 2022

alamb approved these changes Dec 28, 2022

View reviewed changes

alamb merged commit 3e99046 into sqlparser-rs:main Dec 28, 2022

This was referenced Dec 28, 2022

Fix create statement for mysql #602

Closed

fix: mysql ON UPDATE not taking an expression #586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `CREATE TABLE ON UPDATE <expr>` Function #685

Support `CREATE TABLE ON UPDATE <expr>` Function #685

CEOJINSUNG commented Oct 22, 2022

AugustoFKL Oct 29, 2022

CEOJINSUNG Nov 1, 2022

AugustoFKL Oct 29, 2022

alamb Oct 31, 2022 •

edited

AugustoFKL Nov 1, 2022

CEOJINSUNG Nov 1, 2022 •

edited

AugustoFKL Nov 1, 2022

CEOJINSUNG Nov 3, 2022

AugustoFKL Nov 3, 2022

coveralls commented Oct 31, 2022 •

edited

alamb left a comment

alamb Oct 31, 2022 •

edited

alamb left a comment

AugustoFKL left a comment

alamb commented Nov 11, 2022 •

edited

alamb commented Dec 28, 2022

		@@ -2921,9 +2921,8 @@ impl<'a> Parser<'a> {
		} else if self.parse_keywords(&[Keyword::ON, Keyword::UPDATE])
		&& dialect_of!(self is MySqlDialect)

Support CREATE TABLE ON UPDATE <expr> Function #685

Support CREATE TABLE ON UPDATE <expr> Function #685

Conversation

CEOJINSUNG commented Oct 22, 2022

AugustoFKL Oct 29, 2022

Choose a reason for hiding this comment

CEOJINSUNG Nov 1, 2022

Choose a reason for hiding this comment

AugustoFKL Oct 29, 2022

Choose a reason for hiding this comment

alamb Oct 31, 2022 • edited

Choose a reason for hiding this comment

AugustoFKL Nov 1, 2022

Choose a reason for hiding this comment

CEOJINSUNG Nov 1, 2022 • edited

Choose a reason for hiding this comment

AugustoFKL Nov 1, 2022

Choose a reason for hiding this comment

CEOJINSUNG Nov 3, 2022

Choose a reason for hiding this comment

AugustoFKL Nov 3, 2022

Choose a reason for hiding this comment

coveralls commented Oct 31, 2022 • edited

Pull Request Test Coverage Report for Build 3366856922

💛 - Coveralls

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 31, 2022 • edited

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

AugustoFKL left a comment

Choose a reason for hiding this comment

alamb commented Nov 11, 2022 • edited

alamb commented Dec 28, 2022

Support `CREATE TABLE ON UPDATE <expr>` Function #685

Support `CREATE TABLE ON UPDATE <expr>` Function #685

alamb Oct 31, 2022 •

edited

CEOJINSUNG Nov 1, 2022 •

edited

coveralls commented Oct 31, 2022 •

edited

alamb Oct 31, 2022 •

edited

alamb commented Nov 11, 2022 •

edited