Skip to content
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

Don't use a temporary table for batching inserts. #8806

Merged
merged 1 commit into from
Jun 9, 2017
Merged

Conversation

AndriySvyryd
Copy link
Member

Fixes #8415
Fixes #8322

|| typeName.Equals("timestamp", StringComparison.OrdinalIgnoreCase))
? (property.IsNullable ? "varbinary(8)" : "binary(8)")
: typeName;
if (property.ClrType == typeof(byte[])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the type mapper?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very update-specific, I doubt it could be reused.

.AppendJoin(
writeOperations,
SqlGenerationHelper,
(sb, o, helper) => { helper.DelimitIdentifier(sb, o.ColumnName); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove braces?

@anpete
Copy link
Contributor

anpete commented Jun 9, 2017

This doesn't implement the ordering elimination optimization so we should still track that somehow.

"DECLARE @inserted0 TABLE ([Id] int, [_Position] [int]);" + Environment.NewLine +
"MERGE [dbo].[Ducks] USING @toInsert0 AS i ON 1=0" + Environment.NewLine +
"MERGE [dbo].[Ducks] USING (" + Environment.NewLine +
Copy link
Contributor

@anpete anpete Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change only affects one SQL baseline? Do we have enough coverage for SQL gen here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, consider using verbatim strings here like we do in query - The Env.NLs add a lot of clutter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that ideally a single change in SQL gen should require a single change in the baseline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I believe in 🦄 and 🌈 😄

@AndriySvyryd
Copy link
Member Author

I'll keep #8415 open

@AndriySvyryd AndriySvyryd merged commit cbb6802 into dev Jun 9, 2017
@AndriySvyryd AndriySvyryd deleted the Issue8415 branch June 9, 2017 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants