kotovalexarian-likes-github/rails--rails
1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00
Code Releases Activity

Merge pull request #39203 from kamipo/refactor_index_creation

Browse source 
Refactor index creation to use index definition visitor
This commit is contained in:
Ryuta Kamizono 2020-05-10 02:21:06 +09:00 committed by GitHub
commit 0ce1c2ab31
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 148 additions and 76 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
activerecord/CHANGELOG.md
View file

@ -1,3 +1,7 @@
* Fix index creation to preserve index comment in bulk change table on MySQL.
*Ryuta Kamizono*
* Allow `unscope` to be aware of table name qualified values.
It is possible to unscope only the column in the specified table.

27
activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb
View file

@ -15,7 +15,7 @@ module ActiveRecord
delegate :quote_column_name, :quote_table_name, :quote_default_expression, :type_to_sql,
:options_include_default?, :supports_indexes_in_create?, :supports_foreign_keys?, :foreign_key_options,
to: :@conn, private: true
:quoted_columns_for_index, :supports_partial_index?, to: :@conn, private: true
private
def visit_AlterTable(o)
@ -81,6 +81,31 @@ module ActiveRecord
"DROP CONSTRAINT #{quote_column_name(name)}"
end
def visit_CreateIndexDefinition(o)
index = o.index
sql = ["CREATE"]
sql << "UNIQUE" if index.unique
sql << "INDEX"
sql << "IF NOT EXISTS" if o.if_not_exists
sql << o.algorithm if o.algorithm
sql << index.type if index.type
sql << "#{quote_column_name(index.name)} ON #{quote_table_name(index.table)}"
sql << "USING #{index.using}" if supports_index_using? && index.using
sql << "(#{quoted_columns(index)})"
sql << "WHERE #{index.where}" if supports_partial_index? && index.where
sql.join(" ")
end
def quoted_columns(o)
String === o.columns ? o.columns : quoted_columns_for_index(o.columns, o.column_options)
end
def supports_index_using?
true
end
def table_options(o)
table_options = {}
table_options[:comment] = o.comment

10
activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
View file

@ -33,6 +33,14 @@ module ActiveRecord
@comment = comment
end
def column_options
{
length: lengths,
order: orders,
opclass: opclasses,
}
end
private
def concise_options(options)
if columns.size == options.size && options.values.uniq.size == 1
@ -69,6 +77,8 @@ module ActiveRecord
ChangeColumnDefinition = Struct.new(:column, :name) #:nodoc:
CreateIndexDefinition = Struct.new(:index, :algorithm, :if_not_exists) # :nodoc:
PrimaryKeyDefinition = Struct.new(:name) # :nodoc:
ForeignKeyDefinition = Struct.new(:from_table, :to_table, :options) do #:nodoc:

69
activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
View file

@ -819,8 +819,10 @@ module ActiveRecord
#
# For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration].
def add_index(table_name, column_name, options = {})
index_name, index_type, index_columns, index_if_not_exists_clause, index_options = add_index_options(table_name, column_name, **options)
execute "CREATE #{index_type} #{index_if_not_exists_clause} #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} (#{index_columns})#{index_options}"
index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
execute schema_creation.accept(create_index)
end
# Removes the given index from the table.
@ -1219,35 +1221,43 @@ module ActiveRecord
Table.new(table_name, base)
end
def add_index_options(table_name, column_name, comment: nil, **options) # :nodoc:
def add_index_options(table_name, column_name, name: nil, if_not_exists: false, internal: false, **options) # :nodoc:
options.assert_valid_keys(:unique, :length, :order, :opclass, :where, :type, :using, :comment, :algorithm)
column_names = index_column_names(column_name)
options.assert_valid_keys(:unique, :order, :name, :where, :length, :internal, :using, :algorithm, :type, :opclass, :if_not_exists)
index_type = options[:type].to_s if options.key?(:type)
index_type ||= options[:unique] ? "UNIQUE" : ""
index_name = options[:name].to_s if options.key?(:name)
index_name = name&.to_s
index_name ||= index_name(table_name, column_names)
if options.key?(:algorithm)
algorithm = index_algorithms.fetch(options[:algorithm]) {
raise ArgumentError.new("Algorithm must be one of the following: #{index_algorithms.keys.map(&:inspect).join(', ')}")
}
validate_index_length!(table_name, index_name, internal)
index = IndexDefinition.new(
table_name, index_name,
options[:unique],
column_names,
lengths: options[:length] || {},
orders: options[:order] || {},
opclasses: options[:opclass] || {},
where: options[:where],
type: options[:type],
using: options[:using],
comment: options[:comment]
)
[index, index_algorithm(options[:algorithm]), if_not_exists]
end
def index_algorithm(algorithm) # :nodoc:
index_algorithms.fetch(algorithm) do
raise ArgumentError, "Algorithm must be one of the following: #{index_algorithms.keys.map(&:inspect).join(', ')}"
end if algorithm
end
def quoted_columns_for_index(column_names, options) # :nodoc:
quoted_columns = column_names.each_with_object({}) do |name, result|
result[name.to_sym] = quote_column_name(name).dup
end
using = "USING #{options[:using]}" if options[:using].present?
if supports_partial_index?
index_options = options[:where] ? " WHERE #{options[:where]}" : ""
end
if_not_exists_index_clause = options[:if_not_exists] ? "INDEX IF NOT EXISTS" : "INDEX"
validate_index_length!(table_name, index_name, options.fetch(:internal, false))
index_columns = quoted_columns_for_index(column_names, **options).join(", ")
[index_name, index_type, index_columns, if_not_exists_index_clause, index_options, algorithm, using, comment]
add_options_for_index_columns(quoted_columns, **options).values.join(", ")
end
def options_include_default?(options)
@ -1308,13 +1318,6 @@ module ActiveRecord
quoted_columns
end
def quoted_columns_for_index(column_names, **options)
return [column_names] if column_names.is_a?(String)
quoted_columns = Hash[column_names.map { |name| [name.to_sym, quote_column_name(name).dup] }]
add_options_for_index_columns(quoted_columns, **options).values
end
def index_name_for_remove(table_name, column_name, options)
if column_name.is_a?(Hash)
options = column_name.dup

14
activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
View file

@ -362,9 +362,10 @@ module ActiveRecord
def add_index(table_name, column_name, options = {}) #:nodoc:
return if options[:if_not_exists] && index_exists?(table_name, column_name, options)
index_name, index_type, index_columns, _, _, index_algorithm, index_using, comment = add_index_options(table_name, column_name, **options)
sql = +"CREATE #{index_type} INDEX #{quote_column_name(index_name)} #{index_using} ON #{quote_table_name(table_name)} (#{index_columns}) #{index_algorithm}"
execute add_sql_comment!(sql, comment)
index, algorithm, _ = add_index_options(table_name, column_name, **options)
create_index = CreateIndexDefinition.new(index, algorithm)
execute schema_creation.accept(create_index)
end
def add_sql_comment!(sql, comment) # :nodoc:
@ -676,9 +677,10 @@ module ActiveRecord
end
def add_index_for_alter(table_name, column_name, options = {})
index_name, index_type, index_columns, _, _, index_algorithm, index_using = add_index_options(table_name, column_name, **options)
index_algorithm[0, 0] = ", " if index_algorithm.present?
"ADD #{index_type} INDEX #{quote_column_name(index_name)} #{index_using} (#{index_columns})#{index_algorithm}"
index, algorithm, _ = add_index_options(table_name, column_name, **options)
algorithm = ", #{algorithm}" if algorithm
"ADD #{schema_creation.accept(index)}#{algorithm}"
end
def remove_index_for_alter(table_name, column_name = nil, options = {})

24
activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb
View file

@ -20,6 +20,26 @@ module ActiveRecord
add_column_position!(change_column_sql, column_options(o.column))
end
def visit_CreateIndexDefinition(o)
sql = visit_IndexDefinition(o.index, true)
sql << " #{o.algorithm}" if o.algorithm
sql
end
def visit_IndexDefinition(o, create = false)
index_type = o.type&.to_s&.upcase || o.unique && "UNIQUE"
sql = create ? ["CREATE"] : []
sql << index_type if index_type
sql << "INDEX"
sql << quote_column_name(o.name)
sql << "USING #{o.using}" if o.using
sql << "ON #{quote_table_name(o.table)}" if create
sql << "(#{quoted_columns(o)})"
add_sql_comment!(sql.join(" "), o.comment)
end
def add_table_options!(create_sql, options)
add_sql_comment!(super, options[:comment])
end
@ -62,8 +82,8 @@ module ActiveRecord
end
def index_in_create(table_name, column_name, options)
index_name, index_type, index_columns, _, _, _, index_using, comment = @conn.add_index_options(table_name, column_name, **options)
add_sql_comment!((+"#{index_type} INDEX #{quote_column_name(index_name)} #{index_using} (#{index_columns})"), comment)
index, _ = @conn.add_index_options(table_name, column_name, **options)
accept(index)
end
end
end

11
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
View file

@ -442,10 +442,13 @@ module ActiveRecord
end
def add_index(table_name, column_name, options = {}) #:nodoc:
index_name, index_type, index_columns_and_opclasses, index_if_not_exists_clause, index_options, index_algorithm, index_using, comment = add_index_options(table_name, column_name, **options)
execute("CREATE #{index_type} #{index_if_not_exists_clause} #{index_algorithm} #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} #{index_using} (#{index_columns_and_opclasses})#{index_options}").tap do
execute "COMMENT ON INDEX #{quote_column_name(index_name)} IS #{quote(comment)}" if comment
end
index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
result = execute schema_creation.accept(create_index)
execute "COMMENT ON INDEX #{quote_column_name(index.name)} IS #{quote(index.comment)}" if index.comment
result
end
def remove_index(table_name, column_name = nil, options = {}) # :nodoc:

4
activerecord/lib/active_record/connection_adapters/sqlite3/schema_creation.rb
View file

@ -5,6 +5,10 @@ module ActiveRecord
module SQLite3
class SchemaCreation < SchemaCreation # :nodoc:
private
def supports_index_using?
false
end
def add_column_options!(sql, options)
if options[:collation]
sql << " COLLATE \"#{options[:collation]}\""

30
activerecord/test/cases/adapters/mysql2/active_schema_test.rb
View file

@ -27,38 +27,38 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase
end
def test_add_index
expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`) "
expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`)"
assert_equal expected, add_index(:people, :last_name, length: nil)
expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`(10)) "
expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`(10))"
assert_equal expected, add_index(:people, :last_name, length: 10)
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`(15)) "
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`(15))"
assert_equal expected, add_index(:people, [:last_name, :first_name], length: 15)
assert_equal expected, add_index(:people, ["last_name", "first_name"], length: 15)
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`) "
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`)"
assert_equal expected, add_index(:people, [:last_name, :first_name], length: { last_name: 15 })
assert_equal expected, add_index(:people, ["last_name", "first_name"], length: { last_name: 15 })
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`(10)) "
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` ON `people` (`last_name`(15), `first_name`(10))"
assert_equal expected, add_index(:people, [:last_name, :first_name], length: { last_name: 15, first_name: 10 })
assert_equal expected, add_index(:people, ["last_name", :first_name], length: { last_name: 15, "first_name" => 10 })
%w(SPATIAL FULLTEXT UNIQUE).each do |type|
expected = "CREATE #{type} INDEX `index_people_on_last_name` ON `people` (`last_name`) "
expected = "CREATE #{type} INDEX `index_people_on_last_name` ON `people` (`last_name`)"
assert_equal expected, add_index(:people, :last_name, type: type)
end
%w(btree hash).each do |using|
expected = "CREATE INDEX `index_people_on_last_name` USING #{using} ON `people` (`last_name`) "
expected = "CREATE INDEX `index_people_on_last_name` USING #{using} ON `people` (`last_name`)"
assert_equal expected, add_index(:people, :last_name, using: using)
end
expected = "CREATE INDEX `index_people_on_last_name` USING btree ON `people` (`last_name`(10)) "
expected = "CREATE INDEX `index_people_on_last_name` USING btree ON `people` (`last_name`(10))"
assert_equal expected, add_index(:people, :last_name, length: 10, using: :btree)
expected = "CREATE INDEX `index_people_on_last_name` USING btree ON `people` (`last_name`(10)) ALGORITHM = COPY"
expected = "CREATE INDEX `index_people_on_last_name` USING btree ON `people` (`last_name`(10)) ALGORITHM = COPY"
assert_equal expected, add_index(:people, :last_name, length: 10, using: :btree, algorithm: :copy)
with_real_execute do
@ -74,20 +74,20 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase
add_index(:people, :last_name, algorithm: :coyp)
end
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` USING btree ON `people` (`last_name`(15), `first_name`(15)) "
expected = "CREATE INDEX `index_people_on_last_name_and_first_name` USING btree ON `people` (`last_name`(15), `first_name`(15))"
assert_equal expected, add_index(:people, [:last_name, :first_name], length: 15, using: :btree)
end
def test_index_in_create
%w(SPATIAL FULLTEXT UNIQUE).each do |type|
expected = /\ACREATE TABLE `people` \(#{type} INDEX `index_people_on_last_name` \(`last_name`\)\)/
expected = /\ACREATE TABLE `people` \(#{type} INDEX `index_people_on_last_name` \(`last_name`\)\)/
actual = ActiveRecord::Base.connection.create_table(:people, id: false) do |t|
t.index :last_name, type: type
end
assert_match expected, actual
end
expected = /\ACREATE TABLE `people` \( INDEX `index_people_on_last_name` USING btree \(`last_name`\(10\)\)\)/
expected = /\ACREATE TABLE `people` \(INDEX `index_people_on_last_name` USING btree \(`last_name`\(10\)\)\)/
actual = ActiveRecord::Base.connection.create_table(:people, id: false) do |t|
t.index :last_name, length: 10, using: :btree
end
@ -96,7 +96,7 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase
def test_index_in_bulk_change
%w(SPATIAL FULLTEXT UNIQUE).each do |type|
expected = "ALTER TABLE `people` ADD #{type} INDEX `index_people_on_last_name` (`last_name`)"
expected = "ALTER TABLE `people` ADD #{type} INDEX `index_people_on_last_name` (`last_name`)"
assert_sql(expected) do
ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t|
t.index :last_name, type: type
@ -104,7 +104,7 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase
end
end
expected = "ALTER TABLE `people` ADD INDEX `index_people_on_last_name` USING btree (`last_name`(10)), ALGORITHM = COPY"
expected = "ALTER TABLE `people` ADD INDEX `index_people_on_last_name` USING btree (`last_name`(10)), ALGORITHM = COPY"
assert_sql(expected) do
ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t|
t.index :last_name, length: 10, using: :btree, algorithm: :copy
@ -170,7 +170,7 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase
end
def test_indexes_in_create
expected = /\ACREATE TEMPORARY TABLE `temp` \( INDEX `index_temp_on_zip` \(`zip`\)\)(?: ROW_FORMAT=DYNAMIC)? AS SELECT id, name, zip FROM a_really_complicated_query/
expected = /\ACREATE TEMPORARY TABLE `temp` \(INDEX `index_temp_on_zip` \(`zip`\)\)(?: ROW_FORMAT=DYNAMIC)? AS SELECT id, name, zip FROM a_really_complicated_query/
actual = ActiveRecord::Base.connection.create_table(:temp, temporary: true, as: "SELECT id, name, zip FROM a_really_complicated_query") do |t|
t.index :zip
end

26
activerecord/test/cases/adapters/postgresql/active_schema_test.rb
View file

@ -28,46 +28,46 @@ class PostgresqlActiveSchemaTest < ActiveRecord::PostgreSQLTestCase
end
def test_add_index
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name" ON "people" ("last_name") WHERE state = 'active')
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name" ON "people" ("last_name") WHERE state = 'active')
assert_equal expected, add_index(:people, :last_name, unique: true, where: "state = 'active'")
expected = %(CREATE UNIQUE INDEX "index_people_on_lower_last_name" ON "people" (lower(last_name)))
expected = %(CREATE UNIQUE INDEX "index_people_on_lower_last_name" ON "people" (lower(last_name)))
assert_equal expected, add_index(:people, "lower(last_name)", unique: true)
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name_varchar_pattern_ops" ON "people" (last_name varchar_pattern_ops))
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name_varchar_pattern_ops" ON "people" (last_name varchar_pattern_ops))
assert_equal expected, add_index(:people, "last_name varchar_pattern_ops", unique: true)
expected = %(CREATE INDEX CONCURRENTLY "index_people_on_last_name" ON "people" ("last_name"))
expected = %(CREATE INDEX CONCURRENTLY "index_people_on_last_name" ON "people" ("last_name"))
assert_equal expected, add_index(:people, :last_name, algorithm: :concurrently)
expected = %(CREATE INDEX "index_people_on_last_name_and_first_name" ON "people" ("last_name" DESC, "first_name" ASC))
expected = %(CREATE INDEX "index_people_on_last_name_and_first_name" ON "people" ("last_name" DESC, "first_name" ASC))
assert_equal expected, add_index(:people, [:last_name, :first_name], order: { last_name: :desc, first_name: :asc })
assert_equal expected, add_index(:people, ["last_name", :first_name], order: { last_name: :desc, "first_name" => :asc })
%w(gin gist hash btree).each do |type|
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" USING #{type} ("last_name"))
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" USING #{type} ("last_name"))
assert_equal expected, add_index(:people, :last_name, using: type)
expected = %(CREATE INDEX CONCURRENTLY "index_people_on_last_name" ON "people" USING #{type} ("last_name"))
expected = %(CREATE INDEX CONCURRENTLY "index_people_on_last_name" ON "people" USING #{type} ("last_name"))
assert_equal expected, add_index(:people, :last_name, using: type, algorithm: :concurrently)
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name" ON "people" USING #{type} ("last_name") WHERE state = 'active')
expected = %(CREATE UNIQUE INDEX "index_people_on_last_name" ON "people" USING #{type} ("last_name") WHERE state = 'active')
assert_equal expected, add_index(:people, :last_name, using: type, unique: true, where: "state = 'active'")
expected = %(CREATE UNIQUE INDEX "index_people_on_lower_last_name" ON "people" USING #{type} (lower(last_name)))
expected = %(CREATE UNIQUE INDEX "index_people_on_lower_last_name" ON "people" USING #{type} (lower(last_name)))
assert_equal expected, add_index(:people, "lower(last_name)", using: type, unique: true)
end
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" USING gist ("last_name" bpchar_pattern_ops))
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" USING gist ("last_name" bpchar_pattern_ops))
assert_equal expected, add_index(:people, :last_name, using: :gist, opclass: { last_name: :bpchar_pattern_ops })
expected = %(CREATE INDEX "index_people_on_last_name_and_first_name" ON "people" ("last_name" DESC NULLS LAST, "first_name" ASC))
expected = %(CREATE INDEX "index_people_on_last_name_and_first_name" ON "people" ("last_name" DESC NULLS LAST, "first_name" ASC))
assert_equal expected, add_index(:people, [:last_name, :first_name], order: { last_name: "DESC NULLS LAST", first_name: :asc })
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" ("last_name" NULLS FIRST))
expected = %(CREATE INDEX "index_people_on_last_name" ON "people" ("last_name" NULLS FIRST))
assert_equal expected, add_index(:people, :last_name, order: "NULLS FIRST")
expected = %(CREATE INDEX IF NOT EXISTS "index_people_on_last_name" ON "people" ("last_name"))
expected = %(CREATE INDEX IF NOT EXISTS "index_people_on_last_name" ON "people" ("last_name"))
assert_equal expected, add_index(:people, :last_name, if_not_exists: true)
assert_raise ArgumentError do

5
activerecord/test/cases/migration_test.rb
View file

@ -1084,7 +1084,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
classname = ActiveRecord::Base.connection.class.name[/[^:]*$/]
expected_query_count = {
"Mysql2Adapter" => 1, # mysql2 supports creating two indexes using one statement
"PostgreSQLAdapter" => 2,
"PostgreSQLAdapter" => 3,
}.fetch(classname) {
raise "need an expected query count for #{classname}"
}
@ -1092,7 +1092,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
assert_queries(expected_query_count) do
with_bulk_change_table do |t|
t.index :username, unique: true, name: :awesome_username_index
t.index [:name, :age]
t.index [:name, :age], comment: "This is a comment"
end
end
@ -1100,6 +1100,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
name_age_index = index(:index_delete_me_on_name_and_age)
assert_equal ["name", "age"].sort, name_age_index.columns.sort
assert_equal "This is a comment", name_age_index.comment
assert_not name_age_index.unique
assert index(:awesome_username_index).unique