MSSQL: Add support for functionality MERGE output clause
#1790
Conversation
This reverts commit 3a29477. The commit has a lot of stuff I don't think i've added
dilovancelik
changed the title
There was a problem hiding this comment.
Thanks @dilovancelik! The changes look reasonable to me overall, left some comments
src/parser/mod.rs
Outdated
| self.expect_keyword_is(Keyword::INTO)?; | ||
| let temporary = self | ||
| .parse_one_of_keywords(&[Keyword::TEMP, Keyword::TEMPORARY]) | ||
| .is_some(); | ||
| let unlogged = self.parse_keyword(Keyword::UNLOGGED); | ||
| let table = self.parse_keyword(Keyword::TABLE); | ||
| let name = self.parse_object_name(false)?; | ||
| let into_table = SelectInto { | ||
| temporary, | ||
| unlogged, | ||
| table, | ||
| name, | ||
| }; |
There was a problem hiding this comment.
This looks to be the same code as in parse_select? could we factor that into a function like parse_into_clause() that could be reused in both places?
There was a problem hiding this comment.
Yeah that makes sense. I made it, it's own function, which I called parse_select_into, since it returns the SelectInto structure, that way its consistent
Co-authored-by: Ifeanyi Ubah <[email protected]>
Co-authored-by: Ifeanyi Ubah <[email protected]>
Co-authored-by: Ifeanyi Ubah <[email protected]>
Co-authored-by: Ifeanyi Ubah <[email protected]>
Co-authored-by: Ifeanyi Ubah <[email protected]>
@iffyio thanks a lot for the comments, I just applied your suggestions. For the other comments, I believe they make a lot of sense, and have implemented what you suggested. I have not resolved those conversations, since I don't know if you would like to do it your self after checking my new commits :) |
| }) | ||
| } | ||
|
|
||
| fn parse_select_into(&mut self) -> Result<SelectInto, ParserError> { |
There was a problem hiding this comment.
oh could we also update this usage to call this parse_select_into() function as well?
There was a problem hiding this comment.
good idea, I've changed it to use the new function!
|
I updated the old usage to use the new parse_select_into function and ran all tests :) |
iffyio
changed the title
MERGE output clause
|
@dilovancelik when you get the chance to could you merge in latest from main, in order to fix the CI issue? |
|
should be added now, and thank you for the review |
|
@dilovancelik Oh could you take a look at the added commits, not sure how the merge from main was done but there's some extraneous diffs that's been brought into the PR |
|
Hey I did a rebase, and I think something went wrong, now I just did a classic merge, and et looks like the extra commits are gone. Sorry. |
|
I don't understudy why run clippy fails, It does. not do it on my machine, and it also seems to be we code which is not part of my PR. Is there anything I can do to help out with it? |
|
Oh yes so that failure is my bad and came from the last PR that landed on main yesterday, it fails on rust 1.86. To be clear its not related to this PR but its possible to resolve, @dilovancelik would you be able to resolve the lint failure in this PR and I can merge both in the same go? Sorry for the hassle |
|
No worries, I just couldn't get the error locally so couldn't fix. That however was because my main was one commit behind. I synced and removed the borrow which threw the error. Again thanks for all the feedback, this has been a really pleasant first OS experience |
|
Awesome thanks again @dilovancelik! |
Hey I have added a feature to handle OUTPUT Statements in the end of merge statements, which is used in MS SQL fixes #1789 that I created.
This is my first Pull Request to an open source project so please let me know, if there is something missing, or something I did wrong.
Thank you for a great crate.