-
Notifications
You must be signed in to change notification settings - Fork 563
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
insert_all/upsert_implementation using MERGE #869
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,6 +140,37 @@ def default_insert_value(column) | |
private :default_insert_value | ||
|
||
def build_insert_sql(insert) # :nodoc: | ||
if insert.skip_duplicates? | ||
# Do we have a unique_by index? Use index columns | ||
conflict_columns = if (unique_by = insert.send(:insert_all).unique_by) | ||
[unique_by.columns] | ||
else | ||
# Compare against every unique constraint (primary key included). | ||
# Discard constraints that are not fully included on insert.keys. Prevents invalid queries. | ||
# Example: ignore unique index for columns ["name"] if insert keys is ["description"] | ||
insert_all = insert.send(:insert_all) | ||
|
||
(insert_all.send(:unique_indexes).map(&:columns) + [insert_all.primary_keys]).select do |columns| | ||
columns.to_set.subset?(insert.keys) | ||
end | ||
end | ||
includes_primary_key = (insert.send(:insert_all).primary_keys.to_set & insert.keys).present? | ||
|
||
sql = +"" | ||
sql << "SET IDENTITY_INSERT #{insert.model.quoted_table_name} ON;" if includes_primary_key | ||
sql << "MERGE INTO #{insert.model.quoted_table_name} WITH (UPDLOCK, HOLDLOCK) AS target" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe HOLDLOCK is equivalent to SERIALIZABLE. I'd prefer the SQL standard term. Unless HOLDLOCK is usual in the SQL Server community? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will check that. Thanks! |
||
sql << " USING (SELECT DISTINCT * FROM (#{insert.values_list}) AS t1 (#{insert.send(:columns_list)})) AS source" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of the standard adapters uniquify insert rows. I don't think we need to do it here either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uniquify was the solution to this test/scenario
I don't like it but it works so I move on until all test pass. Then I found the failing test (still stuck with it)
My impression is that the solution to this will let me remove the uniquify. |
||
sql << " ON (#{conflict_columns.map { |columns| columns.map { |column| "target.#{quote_column_name(column)} = source.#{quote_column_name(column)}" }.join(" AND ") }.join(") OR (")})" | ||
sql << " WHEN NOT MATCHED BY TARGET THEN" | ||
sql << " INSERT (#{insert.send(:columns_list)}) VALUES (#{insert.keys.map { |column| "source.#{quote_column_name(column)}" }.join(", ")})" | ||
if returning = insert.send(:insert_all).returning | ||
sql << " OUTPUT " << returning.map { |column| "INSERTED.#{quote_column_name(column)}" }.join(", ") | ||
end | ||
sql << ";" | ||
sql << "SET IDENTITY_INSERT #{insert.model.quoted_table_name} OFF;" if includes_primary_key | ||
return sql | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeatedly appending to a string is somewhat slow. Faster in that case is to append strings to an array and then join the array at the end. Not sure how much difference that would make, but it would be just as easy to read, I think, and would probably be noticeable if we are dealing with many rows. |
||
end | ||
|
||
sql = +"INSERT #{insert.into}" | ||
|
||
if returning = insert.send(:insert_all).returning | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
|
||
require "cases/helper_sqlserver" | ||
require "models/book" | ||
|
||
class InsertAllTestSQLServer < ActiveRecord::TestCase | ||
fixtures :books | ||
|
||
# Issue https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/issues/847 | ||
it "execute insert_all with a single element" do | ||
assert_difference "Book.count", +1 do | ||
Book.insert_all [{ name: "Rework", author_id: 1 }] | ||
end | ||
end | ||
end |
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.
Can we not do
insert.insert_all
?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.
Suggest we cache insert_all in a local variable rather than have to look it up each time.
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.
insert_all is private, it's not possible to do
insert.insert_all
.Thanks for the suggestion but I'm still not paying attention to things like this. As I said, the PR is in an early stage. I have plans to improve the code (refactor) but right now I'm focused on produce the SQL that cover all tests.