Add indexes to create_join_table method #7028

Merged
merged 1 commit into from Jul 18, 2012

Projects

None yet

5 participants

Contributor
lexmag commented Jul 10, 2012

No description provided.

@rafaelfranca rafaelfranca commented on the diff Jul 10, 2012
activerecord/lib/active_record/migration/join_table.rb
@@ -4,13 +4,15 @@ module JoinTable #:nodoc:
private
def find_join_table_name(table_1, table_2, options = {})
- options.delete(:table_name) { join_table_name(table_1, table_2) }
+ options.delete(:table_name) || join_table_name(table_1, table_2)
rafaelfranca
rafaelfranca Jul 10, 2012 Owner

Why this change?

lexmag
lexmag Jul 10, 2012 Contributor

It's more speedy and readable. And block usually used if we need yield self. WDYT?

rafaelfranca
rafaelfranca Jul 10, 2012 Owner

This is the idiom of Hash#delete but I didn't not investigate if it is speedy.

Seems fine to me

rafaelfranca
rafaelfranca Jul 10, 2012 Owner

Another thing. I didn't remember now why we need to delete. Do you know?

lexmag
lexmag Jul 10, 2012 Contributor

We can use Hash#fetch

rafaelfranca
rafaelfranca Jul 10, 2012 Owner

fetch use block too. We can use the conditional, but I just want to know why we need the delete.

lexmag
lexmag Jul 10, 2012 Contributor

I dunno. Seems delete not required.

rafaelfranca
rafaelfranca Jul 10, 2012 Owner

Yes, so I think we need to delete the :index option here too:

has_index = options.fetch(:indexes, true)
lexmag
lexmag Jul 10, 2012 Contributor

Yes. I'll fix.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jul 10, 2012
...ord/connection_adapters/abstract/schema_statements.rb
@@ -187,6 +187,9 @@ def create_table(table_name, options = {})
# Any extra options you want appended to the columns definition.
# [<tt>:options</tt>]
# Any extra options you want appended to the table definition.
+ # [<tt>:indexes</tt>]
rafaelfranca
rafaelfranca Jul 10, 2012 Owner

I think that is better to name this options as :index

lexmag
lexmag Jul 10, 2012 Contributor

I'll rename.

Contributor
lexmag commented Jul 10, 2012

Done.

Member

The default cannot be changed to true, it will break previous migrations.

In any case, I am opposed to automatically add them. Depending on how you use the association, you may need just one index, sometimes two, and depending on the database, an index with two columns would be fine.

We could generate them automatically in the migration, but I don't think create_join_table should do it.

Contributor
lexmag commented Jul 11, 2012

How about this approach?

create_join_table(:artists, :musics, index: { artists: false, musics: { unique: true }})

produce only [:music_id, :artist_id] uniq index

Member

Does that offer a substantial benefit over using the current API to declare indexes?

Contributor
lexmag commented Jul 11, 2012

It's easier way for declare two indexes at the same time. It isn't substantial benefit, but benefit.

create_join_table(:artists, :musics, index: true)

And may be it help somehow newbies.

What do you think about another approach to add indexes - extend the functionally https://github.com/rails/rails/blob/master/activerecord/lib/rails/generators/active_record/migration/migration_generator.rb#L16 method to recognize CreateJoinTableTable1Table2 migration names? For instance

rails g migration CreateJoinTableArtistsMusics

will generate

create_join_table :artists, :musics
add_index :artists_musics, [artist_id, :music_id]
add_index :artists_musics, [music_id, :artist_id]

of, if add block to create_join_table method

create_join_table :artists, :musics do |t|
  t.index [artist_id, :music_id]
  t.index [music_id, :artist_id]
end
Member

I am not sure if hiding the decision factors I outlined above under index:
true would be doing a favor to noobs.

Maybe have a way to generate a join table via generator would be better,
yes. Maybe something like:

rails g migration join_table users favorites

So whenever the name of the table is join_table inside the generator, we
could generate what you proposed. Let's see what others think.

Owner

Hmm, what @josevalim said make sense. When I created create_join_table some Core team members argued that it is not a good idea to be used in a generator because it would make harder to change from a has_and_belongs_to_many association to a has_many :through.

Contributor

Big +1. Nice job @lexmag. I spent most of the day working on what is the result of this pull request, so it's bittersweet to see it here already!

I'm in favor of the generator @josevalim proposes.

@davejachimiak davejachimiak and 2 others commented on an outdated diff Jul 15, 2012
...ord/connection_adapters/abstract/schema_statements.rb
create_table(join_table_name, options.merge!(:id => false)) do |td|
- td.integer :"#{table_1.to_s.singularize}_id", column_options
- td.integer :"#{table_2.to_s.singularize}_id", column_options
+ td.integer t1_column, column_options
+ td.integer t2_column, column_options
+ if has_index
+ index_options = join_table_index_options(has_index)
+
+ td.index [t1_column, t2_column], index_options
+ td.index [t2_column, t1_column], index_options
davejachimiak
davejachimiak Jul 15, 2012 Contributor

Just curious: what's the benefit of creating two multi-column indexes here? Do they order differently on create and update? Or would they be the same exact index?

Also - watch out for MySQL; it will only use one index per table query. So having both indexes there would be an unnecessary cost to db performance. To account for this, you may have to create a method like def one_index_per_table_query?; false; end on the abstract adapter, and def one_index_per_table_query?; true; end on the MySQL abstract adapter, and use that method here to create only one join table index for people using MySQL. Tests would have to reflect that biz logic, too.

al2o3cr
al2o3cr Jul 15, 2012 Contributor

@davejachimiak - one reason for the two-way indexes is that at least some DBs can only use multicolumn indexes if the given values are part of a leftmost prefix. See here for more detail:

http://dev.mysql.com/doc/refman/5.0/en/mysql-indexes.html#id593580

TL;DR - only putting one means that either t1.t2s or t2.t1s doesn't get to use an index at all on MySQL.

lexmag
lexmag Jul 15, 2012 Contributor

Many to many relation indexes is big topic. It depends on how you search. Better let users to define the indices by themselves.

davejachimiak
davejachimiak Jul 15, 2012 Contributor

@al2o3cr gotcha - edification much appreciated. @lexmag indeed - nice work.

Contributor
lexmag commented Jul 15, 2012

@josevalim, what do you think about this generator?

@davejachimiak davejachimiak commented on the diff Jul 16, 2012
...record/test/cases/migration/create_join_table_test.rb
@@ -42,22 +42,36 @@ def test_create_join_table_with_the_proper_order
end
def test_create_join_table_with_the_table_name
- connection.create_join_table :artists, :musics, :table_name => :catalog
+ connection.create_join_table :artists, :musics, table_name: :catalog
davejachimiak
davejachimiak Jul 16, 2012 Contributor

Hash rockets will still be standard for Rails 4, right?

rafaelfranca
rafaelfranca Jul 16, 2012 Owner

For generated code, no. Also, we are changing in place where we are touching the code.

davejachimiak
davejachimiak Jul 16, 2012 Contributor

By generated code do you mean config/application.rb, db/schema.rb, etc.?

davejachimiak
davejachimiak Jul 16, 2012 Contributor

Nice. Cleaner.

@josevalim josevalim commented on an outdated diff Jul 17, 2012
guides/source/migrations.textile
@@ -462,6 +462,9 @@ create_join_table :products, :categories, :column_options => {:null => true}
will create the +product_id+ and +category_id+ with the +:null+ option as +true+.
+NOTE: By default the appropriate indexes will be created.
josevalim
josevalim Jul 17, 2012 Member

This is no longer true.

@josevalim josevalim and 1 other commented on an outdated diff Jul 17, 2012
...rators/active_record/migration/migration_generator.rb
+ attr.instance_variable_set(:@index_name, [attr, attributes[i - 1]].map{ |a| :"#{a.name.singularize}_id"})
josevalim
josevalim Jul 17, 2012 Member

Can we please create a public API in attr to comport this? We shouldn't be calling instance_variable_set. Also this line is too long.

josevalim
josevalim Jul 17, 2012 Member

Actually, this is automatically adding an index, there is no way to rollback. I would like so suggest a different approach that may also simplify things a bit:

  1. Always generate create_join_table with a block;

  2. Always generate t.index for both attributes, the difference is: if the user passed users:index, t.index will appear uncommented, if not, it will be commented.

The reason is the same others commented here, we cannot make a smart guess about the application indexes and since indexes are not free, we should not add them at will.

Thanks a lot for your work.

lexmag
lexmag Jul 17, 2012 Contributor

@josevalim thanks.

Yes, i'l add index_name setter.

This generates index only for attr if index option passed, if not, it will be skipped. But the approach you proposed is more clear.

josevalim
josevalim Jul 17, 2012 Member

If you are going with my proposal, I believe we won't even need the setter (and the has_index? and attributes_with_index methods too).

lexmag
lexmag Jul 17, 2012 Contributor

I will.
has_index? and attributes_with_index aren't need, but the setter - i'm not sure. Only if not use attributes.each and use straightforward first, second methods.

@lexmag lexmag Add join table migration generator
For instance, running

    rails g migration CreateMediaJoinTable artists musics:uniq

will create a migration with

    create_join_table :artists, :musics do |t|
      # t.index [:artist_id, :music_id]
      t.index [:music_id, :artist_id], unique: true
    end
211d88b
Contributor
lexmag commented Jul 17, 2012

Rebased.

@josevalim josevalim merged commit 58ccc9f into rails:master Jul 18, 2012
Member

Thanks! I have merged this but when writing the changelog. The correct way to invoke the generator today is:

    rails g migration create_join_table_for_artists_and_musics artists musics

However, I realized that a user may also write this:

    rails g migration create_join_table_for_artists_and_musics artist_id music_id

Since we are expecting the attributes, it makes sense. Maybe we could add a method called pluralized_name to the generated attribute and then support both invocation syntax... I would honestly prefer the second one.

Could you work on it too @lexmag ? Thanks a lot.

Contributor
lexmag commented Jul 18, 2012

@josevalim thanks.
I'll do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment