-
-
Notifications
You must be signed in to change notification settings - Fork 218
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.
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
Update GMS to detect INSERT statements with row alias and return error. #2463
Conversation
8509d99
to
d10c2b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just one request for the commented out tests. A skipped test or changing them to the new error message would be preferable, just so they aren't so hard to find again in the future.
enginetest/queries/insert_queries.go
Outdated
/* Disabled, see https://github.com/dolthub/dolt/issues/7638 | ||
{ | ||
WriteQuery: "INSERT INTO mytable (i,s) values (1, 'hi') AS dt(new_i,new_s) ON DUPLICATE KEY UPDATE s=new_s", | ||
ExpectedWriteResult: []sql.Row{{types.NewOkResult(2)}}, | ||
SelectQuery: "SELECT * FROM mytable WHERE i = 1", | ||
ExpectedSelect: []sql.Row{{int64(1), "hi"}}, | ||
}, | ||
{ | ||
WriteQuery: "INSERT INTO mytable (i,s) values (1, 'hi') AS dt ON DUPLICATE KEY UPDATE mytable.s=dt.s", | ||
ExpectedWriteResult: []sql.Row{{types.NewOkResult(2)}}, | ||
SelectQuery: "SELECT * FROM mytable WHERE i = 1", | ||
ExpectedSelect: []sql.Row{{int64(1), "hir"}}, | ||
}, | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the new "insert row aliases are not currently supported" error cases? Can we leave them in and assert they return the new error message? That would be a little better than commenting them out. Alternatively, you could add them as skipped tests somewhere – we do track those skipped tests (James has been doing that lately) so at least they are still visible that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a Skip flag to this test type. How does this look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped tests look great. Looks like there is a compile error coming from dbr
being unknown now though.
c153637
to
cac5222
Compare
62ffbc9
to
91a37eb
Compare
We parse these statements but don't yet support them. So for now we return a helpful error.