Fix bug to allow parenthesis query after `INSERT INTO` #1157

jonathanlehto · 2024-02-29T16:04:38Z

Fixing a bug where the columns after hive check would consume a query

alamb · 2024-02-29T19:54:19Z

Hi @jonathanlehto -- can you please fix the CI tests?

I think one needs cargo fmt and there appears to be another error https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/8098985632/job/22142812958?pr=1157

jonathanlehto · 2024-02-29T22:02:08Z

Apologies, I may not be as familiar with MySQL syntax, but it looks like the test case error is due to asserting that the parser will fail the REPLACE statement. From comparing the INSERT and REPLACE docs, it looks like that statement should be succeeding (I could very well be wrong).

If that test case should fail, I can get another update in. I just want to make sure we are on the same page first!

alamb

Thanks for bearing with me @jonathanlehto

I left some questions about this PR

alamb · 2024-03-01T19:14:01Z

src/parser/mod.rs

+ // make sure we are not about to consume a query
+ let mut after_columns = vec![];
+ match self.peek_nth_token(1).token {
+ Token::Word(w) if w.keyword != Keyword::SELECT => {


Checking for select seems incorrect here

What about a query like

INSERT INTO tbla (cola) (VALUES (1))

The Hive manual https://cwiki.apache.org/confluence/display/hive/languagemanual+dml doesn't appear to say anything about this syntax

Standard syntax: INSERT OVERWRITE TABLE tablename1 [PARTITION (partcol1=val1, partcol2=val2 ...) [IF NOT EXISTS]] select_statement1 FROM from_statement; INSERT INTO TABLE tablename1 [PARTITION (partcol1=val1, partcol2=val2 ...)] select_statement1 FROM from_statement; Hive extension (multiple inserts): FROM from_statement INSERT OVERWRITE TABLE tablename1 [PARTITION (partcol1=val1, partcol2=val2 ...) [IF NOT EXISTS]] select_statement1 [INSERT OVERWRITE TABLE tablename2 [PARTITION ... [IF NOT EXISTS]] select_statement2] [INSERT INTO TABLE tablename2 [PARTITION ...] select_statement2] ...; FROM from_statement INSERT INTO TABLE tablename1 [PARTITION (partcol1=val1, partcol2=val2 ...)] select_statement1 [INSERT INTO TABLE tablename2 [PARTITION ...] select_statement2] [INSERT OVERWRITE TABLE tablename2 [PARTITION ... [IF NOT EXISTS]] select_statement2] ...; Hive extension (dynamic partition inserts): INSERT OVERWRITE TABLE tablename PARTITION (partcol1[=val1], partcol2[=val2] ...) select_statement FROM from_statement; INSERT INTO TABLE tablename PARTITION (partcol1[=val1], partcol2[=val2] ...) select_statement FROM from_statement;

I may be missing something

From reading those docs, it looks like the PARTITION keyword is required. I moved the after_columns parsing into that statement. I don't know why I didn't do that originally, but I think this should resolve the situation. I added another test case

I may have mis-communicated this. I went ahead and confirmed that the failing test case is incorrect.

Test case:

#[test] fn test_replace_into_select() { let sql = r#"REPLACE INTO t1 (a, b, c) (SELECT * FROM t2)"#; assert!(Parser::parse_sql(&GenericDialect {}, sql).is_err()); }

I created a local mysql table and successfully ran these commands.

mysql> INSERT INTO tab (col) (SELECT col FROM tab); Query OK, 1 row affected (0.02 sec) Records: 1 Duplicates: 0 Warnings: 0 mysql> REPLACE INTO tab (col) (SELECT col FROM tab); Query OK, 2 rows affected (0.02 sec) Records: 2 Duplicates: 0 Warnings: 0

However, you can not wrap values in mysql, copied below. From what I can tell, only Trino / Athena allow values statement to be wrapped.

mysql> INSERT INTO tab (col) VALUES (1); Query OK, 1 row affected (0.02 sec) mysql> INSERT INTO tab (col) (VALUES (1)); ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '(1))' at line 1

Unfortunately, I don't have the time to check every Dialect that is supported, but I know that Athena / Trino will succeed and at least MySQL will fail.

In this case, do we only want to succeed a parenthesis values statement in common, and fail for every other dialect? There seems to be another bug here

I rebased and updated this with what my best guess for a solution would be. I only allow VALUES as a subquery in the GenericDialect. Let me know your thoughts

alamb · 2024-03-01T19:15:05Z

tests/sqlparser_common.rs

@@ -106,6 +106,9 @@ fn parse_insert_values() {
 }

 verified_stmt("INSERT INTO customer WITH foo AS (SELECT 1) SELECT * FROM foo UNION VALUES (1)");
+
+ // allow parenthesis query after insert into
+ verified_stmt("INSERT INTO tbla (cola) (SELECT cola FROM tblb)");


What does this syntax mean? Is it the same as

INSERT INTO tbla (cola) SELECT cola FROM tblb

(no parens?)

I believe so, well at the least, running an explain statement in Athena (Trino) yields no difference between

INSERT INTO tbla (cola) SELECT cola FROM tblb
and
INSERT INTO tbla (cola) (SELECT cola FROM tblb)

And registers both as valid queries. From what I recall, I think you can generally wrap any select statements in parenthesis

alamb · 2024-03-03T07:13:13Z

🤔 it seems like the CI is still failing on this PR

coveralls · 2024-03-05T16:44:29Z

Pull Request Test Coverage Report for Build 8238393022

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

25 of 26 (96.15%) changed or added relevant lines in 1 file are covered.
376 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.05%) to 87.873%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	25	26	96.15%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	66	82.93%
src/ast/mod.rs	310	79.97%

Totals
Change from base Build 8128645646:	0.05%
Covered Lines:	20506
Relevant Lines:	23336

💛 - Coveralls

alamb · 2024-03-11T18:24:09Z

It appears the tests are still failing on this PR

jonathanlehto · 2024-03-11T19:09:49Z

I updated the mr to fix the lint and codestyle errors. I was hoping to get your thoughts on this comment, so, if you have some time, I would love to know if you think that implementation is okay!

alamb · 2024-04-09T20:24:05Z

src/parser/mod.rs

+
+ let is_generic = dialect_of!(self is GenericDialect);
+ if !is_generic {
+ if let SetExpr::Values(_) = *subquery.body {


This looks like it is applying a semantic check: https://github.com/sqlparser-rs/sqlparser-rs?tab=readme-ov-file#syntax-vs-semantics

I think any such errors should be generated downstream. Can you please remove this check?

alamb

Thanks @jonathanlehto -- sorry for the delay in reviewing -- the time I can spend on this crate is quite limited and I don't have the bandwidth to study problems in depth typically.

I took a look over this PR again and left some feedback. Let me know what you think

alamb · 2024-04-09T20:30:09Z

src/parser/mod.rs

+ fn test_insert_into_values() {
+ let sql = r#"INSERT INTO t1 (a) VALUES(1)"#;
+
+ assert!(Parser::parse_sql(&GenericDialect {}, sql).is_ok());


Can you please put these tests in common with other similar tests, like this:

https://github.com/sqlparser-rs/sqlparser-rs/blob/2eabfb82083f97bb01883cd3c66c0b035343e3ba/tests/sqlparser_common.rs#L495-L501

alamb · 2024-04-09T20:30:40Z

Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look

jonathanlehto mentioned this pull request Feb 29, 2024

UNLOAD, SET TBLPROPERTIES, SERDEPROPERTIES, EXECUTE USING, SET LOCATION, DELIMITED, EXTENDED / FORMATTED #1137

Closed

jonathanlehto changed the title ~~added bugfix~~ Added Query Consumption Bug Fix Feb 29, 2024

alamb mentioned this pull request Feb 29, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 26, 2024 apache/datafusion#9345

Closed

9 tasks

alamb changed the title ~~Added Query Consumption Bug Fix~~ Fix bug to allow parenthesis query after INSERT INTO Feb 29, 2024

alamb reviewed Mar 1, 2024

View reviewed changes

jonathanlehto added 7 commits March 5, 2024 10:29

added bugfix

7d43ac0

fixing codestyle

ade716a

second pass at codestyle

fad429e

moved partition check into partition statement

95241bf

adding another test case

ca8d634

simplified change

7443e10

rebased, updated to not allow values as valid subquery

5c946a0

jonathanlehto force-pushed the hive_columns_bug branch from 7f8bce9 to 5c946a0 Compare March 5, 2024 16:41

jonathanlehto added 2 commits March 11, 2024 15:05

fixing lint and codestyle

1252b10

adding commas

9738562

alamb reviewed Apr 9, 2024

View reviewed changes

alamb marked this pull request as draft April 9, 2024 20:30

alamb mentioned this pull request Apr 9, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 apache/datafusion#10002

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug to allow parenthesis query after `INSERT INTO` #1157

Fix bug to allow parenthesis query after `INSERT INTO` #1157

jonathanlehto commented Feb 29, 2024

alamb commented Feb 29, 2024

jonathanlehto commented Feb 29, 2024

alamb left a comment

alamb Mar 1, 2024

jonathanlehto Mar 1, 2024

jonathanlehto Mar 5, 2024 •

edited

jonathanlehto Mar 5, 2024

alamb Mar 1, 2024

jonathanlehto Mar 1, 2024 •

edited

alamb commented Mar 3, 2024

coveralls commented Mar 5, 2024 •

edited

alamb commented Mar 11, 2024

jonathanlehto commented Mar 11, 2024 •

edited

alamb Apr 9, 2024 •

edited

alamb left a comment

alamb Apr 9, 2024

alamb commented Apr 9, 2024

Fix bug to allow parenthesis query after INSERT INTO #1157

Are you sure you want to change the base?

Fix bug to allow parenthesis query after INSERT INTO #1157

Conversation

jonathanlehto commented Feb 29, 2024

alamb commented Feb 29, 2024

jonathanlehto commented Feb 29, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 1, 2024

Choose a reason for hiding this comment

jonathanlehto Mar 1, 2024

Choose a reason for hiding this comment

jonathanlehto Mar 5, 2024 • edited

Choose a reason for hiding this comment

jonathanlehto Mar 5, 2024

Choose a reason for hiding this comment

alamb Mar 1, 2024

Choose a reason for hiding this comment

jonathanlehto Mar 1, 2024 • edited

Choose a reason for hiding this comment

alamb commented Mar 3, 2024

coveralls commented Mar 5, 2024 • edited

Pull Request Test Coverage Report for Build 8238393022

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

alamb commented Mar 11, 2024

jonathanlehto commented Mar 11, 2024 • edited

alamb Apr 9, 2024 • edited

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 9, 2024

Choose a reason for hiding this comment

alamb commented Apr 9, 2024

Fix bug to allow parenthesis query after `INSERT INTO` #1157

Fix bug to allow parenthesis query after `INSERT INTO` #1157

jonathanlehto Mar 5, 2024 •

edited

jonathanlehto Mar 1, 2024 •

edited

coveralls commented Mar 5, 2024 •

edited

jonathanlehto commented Mar 11, 2024 •

edited

alamb Apr 9, 2024 •

edited