Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add support for FULLTEXT and SPATIAL indexes using the :type flag for MySQL #9944

Merged
merged 1 commit into from Mar 28, 2013

Conversation

Projects
None yet
2 participants
Contributor

kenmazaika commented Mar 27, 2013

This is a rebase of #8430.

The bulk of the changes as a result of #9891 no longer are needed to support the using index flag, except for dumping the using in the schema, which I had in my last pull request so I left it in.

This pull request is a pure rebase of #8430, if you want me to break this up so dumping the using in the schema is a separate pull request, I can do that, just let me know.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Mar 27, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -755,9 +761,11 @@ def add_index_options(table_name, column_name, options = {})
index_name = index_name(table_name, column: column_names)
if Hash === options # legacy support, since this param was a string
- options.assert_valid_keys(:unique, :order, :name, :where, :length, :internal, :using, :algorithm)
+ options = options.symbolize_keys
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

Why do we need to symbolize the keys?

@kenmazaika

kenmazaika Mar 27, 2013

Contributor

Well this is a little embarrassing. 😄

I was under the impression that the "new" ruby json hash syntax (ok not that new, but I typically don't use it) of:

{pizza: 'awesome'}

mapped to

{'pizza' => 'awesome'}

but it looks like it has always mapped to:

{:pizza => 'awesome' }

Since this is the case, it seems that only responding to the symbol :type (and not the string) is reasonable to do. (Before I was thinking the schema was generating it with a string key, not a symbol - so it needed to be reactive to both)

@kenmazaika

kenmazaika Mar 27, 2013

Contributor

I'll remove this line.

@rafaelfranca rafaelfranca commented on an outdated diff Mar 27, 2013

activerecord/test/cases/schema_dumper_test.rb
end
+
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

No need these extra blank lines

@rafaelfranca rafaelfranca commented on an outdated diff Mar 27, 2013

activerecord/test/schema/mysql2_specific_schema.rb
@@ -45,4 +55,5 @@
enum_column ENUM('true','false')
)
SQL
+
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

No need this blank line

@rafaelfranca rafaelfranca commented on an outdated diff Mar 27, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -505,6 +505,12 @@ def rename_column(table_name, column_name, new_column_name)
#
# Note: only supported by PostgreSQL and MySQL
#
+ # ====== Creating an index with a specific type
+ # add_index(:developers, :name, :type => :fulltext)
+ # generates
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

Put a blank line before and after this line

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Mar 27, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -505,6 +505,12 @@ def rename_column(table_name, column_name, new_column_name)
#
# Note: only supported by PostgreSQL and MySQL
#
+ # ====== Creating an index with a specific type
+ # add_index(:developers, :name, :type => :fulltext)
+ # generates
+ # CREATE FULLTEXT INDEX index_developers_on_name ON developers (name) -- MySQL
+ #
+ # Note: only supported by MySQL. Supported: [:fulltext, :spatial] on MyISAM tables
@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

Remove the extra whitespaces before Supported. Put a period in the end of line. Use the <tt> tag around the array

@kenmazaika

kenmazaika Mar 27, 2013

Contributor

To verify, this line should be:

#  Note: only supported by MySQL. Supported: <tt>[:fulltext, :spatial]</tt> on MyISAM tables.

correct?

edit: change whitespace

@rafaelfranca

rafaelfranca Mar 27, 2013

Owner

I think is better:

# Note: only supported by MySQL. Supported: <tt>:fulltext</tt> and <tt>:spatial</tt> on MyISAM tables.
Owner

rafaelfranca commented Mar 27, 2013

I did some comments in the code but the feature is good.

Can you split the using dumping on schema in a separated commit on this pull request?

Also remember to add a CHANGELOG entry

Contributor

kenmazaika commented Mar 27, 2013

Sure, I'll split this up tonight into two pull requests, one for each feature (and also make the whitespace changes, etc).

Should I make a changelog entry for each change, or only adding the mysql index types?

Owner

rafaelfranca commented Mar 27, 2013

Only the MySQL index types

Contributor

kenmazaika commented Mar 28, 2013

@rafaelfranca I updated this, and did a force push. I believe I addressed the issues you mentioned.

For reference, I kept the old commit around: where/rails@0f9d45c if you want to compare the diffs.

rafaelfranca added a commit that referenced this pull request Mar 28, 2013

Merge pull request #9944 from where/rebased-fulltext-spacial
Add support for FULLTEXT and SPATIAL indexes using the :type flag for MySQL

@rafaelfranca rafaelfranca merged commit cf46c31 into rails:master Mar 28, 2013

Owner

rafaelfranca commented Mar 28, 2013

Very good. Thank you ❤️ 💚 💙 💛 💜

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