fix: mysql ON UPDATE not taking an expression #586

Sibz · 2022-08-19T21:22:37Z

Attempt to resolve the #543, the PR resolves the issue however I was not sure on the test construction so there is currently a pending todo before this can be merged. As per my comments on #543 any assistance/guidance would be appreciated.

Also not sure the extra enum variant is the best way to do this, but wasn't able to pull the tokens and expr from DialectSpecific variant.

closes #543

tests/sqlparser_mysql.rs

alamb · 2022-08-31T11:57:37Z

I think the enum idea is a good one

alamb · 2022-09-27T13:05:02Z

FYI, I am planning to make a new sqlparser-rs release in the next day or two. This PR has some outstanding comments and appears to have have stalled

alamb · 2022-10-15T11:43:05Z

Marking as draft to signify this PR has been reviewed. Please mark this PR as ready for review when it is ready again.

closes apache#543

Sibz · 2022-12-19T04:23:15Z

Commit updated:
Added verification the expr is one of 'CURRENT_TIMESTAMP', 'LOCALTIME', 'LOCALTIMESTAMP', 'NOW'.
Fixed test
Added test for invalid expr parser error

alamb

Thank you @Sibz

coveralls · 2022-12-19T20:01:00Z

Pull Request Test Coverage Report for Build 3769028368

37 of 40 (92.5%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 86.354%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser.rs	11	14	78.57%

Totals
Change from base Build 3720056662:	0.02%
Covered Lines:	12802
Relevant Lines:	14825

💛 - Coveralls

Sibz · 2022-12-21T01:13:24Z

@alamb Just had a quick review and found an improvement could be made, so added a refactor commit. If you would prefer I can squash into the primary commit and force push.

src/parser.rs

parse::check_on_update_expr_is_valid - using local const instead of vec for valid object names

Sibz · 2022-12-24T00:38:21Z

Added NOW keyword, included that in the parse time functions match arm
Fix check name to be case insensitive
Added test for lowercase now() fn use.

alamb

Thank you @Sibz -- this is looking very close.

Can you please remove the validation code? If so I think this PR is good to go.

alamb · 2022-12-28T12:53:11Z

src/parser.rs

@@ -6318,6 +6317,31 @@ impl<'a> Parser<'a> {
    pub fn index(&self) -> usize {
        self.index
    }
+
+    fn check_on_update_expr_is_valid(expr: &Expr) -> Result<(), ParserError> {


my preference is to leave this type of "semantic" check downstream -- so I would prefer that sqlparser accepts any expression in this place, and then downstream crates can enforce limits like the expr must be one of the specified time functions.

@AugustoFKL and I tried to explain this difference in the readme recently

https://github.com/sqlparser-rs/sqlparser-rs#extensible-sql-lexer-and-parser-for-rust

This crate provides only a syntax parser, and tries to avoid applying
any SQL semantics, and accepts queries that specific databases would
reject, even when using that Database's specific Dialect. For
example, CREATE TABLE(x int, x int) is accepted by this crate, even
though most SQL engines will reject this statement due to the repeated
column name x.

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

alamb · 2022-12-28T12:55:18Z

src/parser.rs

-                Token::make_keyword("ON UPDATE"),
-            ])))
+            let expr = self.parse_expr()?;
+            Self::check_on_update_expr_is_valid(&expr)?;


I recommend just accepting expr here

alamb · 2022-12-28T13:32:44Z

Given #602 has something similar, perhaps I can try and push this one through

alamb · 2022-12-28T14:00:23Z

I believe this PR is no longer necessary after #685. Can you please confirm @Sibz ?

Sibz changed the title ~~fix: mysql ON UPDATE not taking and expression~~ fix: mysql ON UPDATE not taking an expression Aug 19, 2022

Sibz force-pushed the fix-mysql-on-update branch from 2585a03 to 02f41c2 Compare August 19, 2022 21:44

alamb reviewed Aug 31, 2022

View reviewed changes

tests/sqlparser_mysql.rs Outdated Show resolved Hide resolved

alamb marked this pull request as draft October 15, 2022 11:43

Sibz force-pushed the fix-mysql-on-update branch 3 times, most recently from 7180583 to 16b0dc5 Compare December 19, 2022 03:43

fix: mysql ON UPDATE not taking an expression

653cd2b

closes apache#543

Sibz force-pushed the fix-mysql-on-update branch from 16b0dc5 to 653cd2b Compare December 19, 2022 04:19

Sibz marked this pull request as ready for review December 19, 2022 04:23

alamb approved these changes Dec 19, 2022

View reviewed changes

Sibz requested a review from alamb December 21, 2022 01:13

alamb reviewed Dec 21, 2022

View reviewed changes

src/parser.rs Outdated Show resolved Hide resolved

Sibz force-pushed the fix-mysql-on-update branch 4 times, most recently from 8322d7f to c2a0559 Compare December 24, 2022 00:15

refactor: removed allocation in check fn

e6b0c64

parse::check_on_update_expr_is_valid - using local const instead of vec for valid object names

Sibz force-pushed the fix-mysql-on-update branch from c2a0559 to e6b0c64 Compare December 24, 2022 00:32

alamb reviewed Dec 28, 2022

View reviewed changes

alamb marked this pull request as draft December 28, 2022 13:29

alamb mentioned this pull request Dec 28, 2022

Support CREATE TABLE ON UPDATE <expr> Function #685

Merged

alamb closed this Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: mysql ON UPDATE not taking an expression #586

fix: mysql ON UPDATE not taking an expression #586

Uh oh!

Sibz commented Aug 19, 2022 •

edited

Loading

Uh oh!

Uh oh!

alamb commented Aug 31, 2022

Uh oh!

alamb commented Sep 27, 2022

Uh oh!

alamb commented Oct 15, 2022

Uh oh!

Sibz commented Dec 19, 2022

Uh oh!

alamb left a comment

Uh oh!

coveralls commented Dec 19, 2022 •

edited

Loading

Uh oh!

Sibz commented Dec 21, 2022 •

edited

Loading

Uh oh!

Uh oh!

Sibz commented Dec 24, 2022

Uh oh!

alamb left a comment

Uh oh!

alamb Dec 28, 2022

Uh oh!

alamb Dec 28, 2022

Uh oh!

alamb commented Dec 28, 2022

Uh oh!

alamb commented Dec 28, 2022

Uh oh!

Uh oh!

fix: mysql ON UPDATE not taking an expression #586

fix: mysql ON UPDATE not taking an expression #586

Uh oh!

Conversation

Sibz commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alamb commented Aug 31, 2022

Uh oh!

alamb commented Sep 27, 2022

Uh oh!

alamb commented Oct 15, 2022

Uh oh!

Sibz commented Dec 19, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3769028368

💛 - Coveralls

Uh oh!

Sibz commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sibz commented Dec 24, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 28, 2022

Uh oh!

alamb commented Dec 28, 2022

Uh oh!

Uh oh!

Sibz commented Aug 19, 2022 •

edited

Loading

coveralls commented Dec 19, 2022 •

edited

Loading

Sibz commented Dec 21, 2022 •

edited

Loading