-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Use OUTPUT INTO for generated values in SqlServer to support tables with triggers #3869
Conversation
commandStringBuilder | ||
.Append($"DECLARE @generated{commandPosition} TABLE (") | ||
.AppendJoin(readOperations.Select(c => | ||
$"{SqlGenerator.DelimitIdentifier(c.ColumnName)} {GetTypeNameForCopy(c.Property)}")) |
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.
Beware that @bricelam hates this 😄
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.
Yes, I do. Concatenation is cheaper.
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.
Everywhere in this file or just this instance?
Did you think about adding actual triggers to some tables in the functional tests? |
I had a few questions but this looks provided that you give me satisfactory answers 😄 Not removing the "needs-review" tag in case you want to get another pair of eyes on it. |
I'll try to add FTs with triggers, but it might take some time. |
Ok, can you file a bug on that and then ? |
…ith triggers Add commandPosition argument to methods on IUpdateSqlGenerator to provide a unique id for a modification command in a batch Fixes #1441
611d903
to
2627626
Compare
Added FTs |
Neat! |
Add commandPosition argument to methods on IUpdateSqlGenerator to provide a unique id for a modification command in a batch
Fixes #1441