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

insert_all/upsert_implementation using MERGE #869

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)

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?

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.

Copy link
Contributor Author

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.

[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"

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uniquify was the solution to this test/scenario

Book.insert_all [
  { author_id: 8, name: "Refactoring" },
  { author_id: 8, name: "Refactoring" }
]

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)

Book.insert_all [
  { id: 200, author_id: 8, name: "Refactoring" },
  { id: 201, author_id: 8, name: "Refactoring" }
]

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

Choose a reason for hiding this comment

The 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
Expand Down
2 changes: 1 addition & 1 deletion lib/active_record/connection_adapters/sqlserver_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def supports_insert_returning?
end

def supports_insert_on_duplicate_skip?
false
true
end

def supports_insert_on_duplicate_update?
Expand Down
5 changes: 5 additions & 0 deletions test/cases/coerced_tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1555,3 +1555,8 @@ class ReloadModelsTest < ActiveRecord::TestCase
# `activesupport/lib/active_support/testing/isolation.rb` exceeds what Windows can handle.
coerce_tests! :test_has_one_with_reload if RbConfig::CONFIG["host_os"] =~ /mswin|mingw/
end

class InsertAllTest < ActiveRecord::TestCase
# Skip this until upsert is supported
coerce_tests! :test_insert
end
15 changes: 15 additions & 0 deletions test/cases/insert_all_test_sqlserver.rb
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