Skip to content

Support CREATE TABLE ON UPDATE <expr> Function #685

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

Merged
merged 3 commits into from
Dec 28, 2022
Merged

Support CREATE TABLE ON UPDATE <expr> Function #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7322d4
Merge pull request #1 from sqlparser-rs/main
CEOJINSUNG Oct 22, 2022
c33e776
feat : OnUpdate Function Implement
CEOJINSUNG Oct 22, 2022
ddbb5df
feat : add GenericDialect Options
CEOJINSUNG Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ast/ddl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ pub enum ColumnOption {
DialectSpecific(Vec<Token>),
CharacterSet(ObjectName),
Comment(String),
OnUpdate(Expr),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@alamb alamb Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@CEOJINSUNG CEOJINSUNG Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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".

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

To me it looks like a really special case.

}

impl fmt::Display for ColumnOption {
Expand Down Expand Up @@ -471,6 +472,7 @@ impl fmt::Display for ColumnOption {
DialectSpecific(val) => write!(f, "{}", display_separated(val, " ")),
CharacterSet(n) => write!(f, "CHARACTER SET {}", n),
Comment(v) => write!(f, "COMMENT '{}'", escape_single_quote_string(v)),
OnUpdate(expr) => write!(f, "ON UPDATE {}", expr),
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2919,11 +2919,10 @@ impl<'a> Parser<'a> {
Token::make_keyword("AUTOINCREMENT"),
])))
} else if self.parse_keywords(&[Keyword::ON, Keyword::UPDATE])
&& dialect_of!(self is MySqlDialect)
&& dialect_of!(self is MySqlDialect | GenericDialect)
{
Ok(Some(ColumnOption::DialectSpecific(vec![
Token::make_keyword("ON UPDATE"),
])))
let expr = self.parse_expr()?;
Ok(Some(ColumnOption::OnUpdate(expr)))
} else {
Ok(None)
}
Expand Down
12 changes: 8 additions & 4 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,7 @@ fn parse_kill() {

#[test]
fn parse_table_colum_option_on_update() {
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE)";
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE CURRENT_TIMESTAMP())";
match mysql().verified_stmt(sql1) {
Statement::CreateTable { name, columns, .. } => {
assert_eq!(name.to_string(), "foo");
Expand All @@ -1018,9 +1018,13 @@ fn parse_table_colum_option_on_update() {
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::DialectSpecific(vec![Token::make_keyword(
"ON UPDATE"
)]),
option: ColumnOption::OnUpdate(Expr::Function(Function {
name: ObjectName(vec![Ident::new("CURRENT_TIMESTAMP")]),
args: vec![],
over: None,
distinct: false,
special: false,
})),
},],
}],
columns
Expand Down