Skip to content

Comments

Refactor index creation to use index definition visitor#39203

Merged
kamipo merged 2 commits intorails:masterfrom
kamipo:refactor_index_creation
May 9, 2020
Merged

Refactor index creation to use index definition visitor#39203
kamipo merged 2 commits intorails:masterfrom
kamipo:refactor_index_creation

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented May 9, 2020

Current SQL generation code is hard to maintenance, I've found a bug
that create index comment in bulk change table when I'm refactoring
that.

@simi
Copy link
Contributor

simi commented May 9, 2020

Nice refactoring! 👍

@kamipo kamipo force-pushed the refactor_index_creation branch from 549ca34 to 6ea1b48 Compare May 9, 2020 02:01
jeremy
jeremy approved these changes May 9, 2020
View reviewed changes
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Nice move. The comment bugfix feels hidden within this refactoring. I think it's worth making that fix a separate commit and mentioning it in the changelog.

activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb Outdated
Copy link
Member

@jeremy jeremy May 9, 2020

Choose a reason for hiding this comment

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

Is this a behavior change? Looks like support_index_using? was not checked previously.

Copy link
Member Author

@kamipo kamipo May 9, 2020

Choose a reason for hiding this comment

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

Nope, This is to keep the behavior as before.

Before, the code generation is not shared, now it is shared by SQLite3 and PostgreSQL, but SQLite3 doesn't support USING syntax, so it will raise syntax error unless the supports_index_using? guard.

Copy link
Member Author

@kamipo kamipo May 9, 2020

Choose a reason for hiding this comment

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

SQLite3:

rails/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

Line 823 in 1c84b1a

execute "CREATE #{index_type} #{index_if_not_exists_clause} #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} (#{index_columns})#{index_options}"

PostgreSQL:

rails/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

Line 446 in 1c84b1a

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

@kamipo kamipo force-pushed the refactor_index_creation branch from 6ea1b48 to 4a32780 Compare May 9, 2020 07:08
@kamipo
Copy link
Member Author

kamipo commented May 9, 2020

Nice move. The comment bugfix feels hidden within this refactoring. I think it's worth making that fix a separate commit and mentioning it in the changelog.

👍 I've separated a commit that fixes a bug and added a CHANGELOG entry.

5e1576d796cae96caaaa27e77d6a27e714d14314

kamipo added 2 commits May 10, 2020 02:01
@kamipo
Fix index creation to preserve comment in bulk change table on MySQL
fc7eab0 
I've found the bug when I'm refactoring index creation code in rails#39203.
@kamipo
Refactor index creation to use index definition visitor
2c1b012 
Current SQL generation code is hard to maintenance, I've found a bug
that create index comment in bulk change table when I'm refactoring
that.
@kamipo kamipo force-pushed the refactor_index_creation branch from 4a32780 to 2c1b012 Compare May 9, 2020 17:02
@kamipo kamipo merged commit 0ce1c2a into rails:master May 9, 2020
@kamipo kamipo deleted the refactor_index_creation branch May 9, 2020 17:21
kamipo added a commit that referenced this pull request May 9, 2020
@kamipo
Fix index creation to preserve comment in bulk change table on MySQL
cb683cb 
I've found the bug when I'm refactoring index creation code in #39203.
alpaca-tc added a commit to alpaca-tc/ridgepole that referenced this pull request Dec 27, 2020
@alpaca-tc
Fixed add_index/remove_index in 6.1.0
301bf56 
Breaking changes introduced by rails/rails#39203
@alpaca-tc alpaca-tc mentioned this pull request Dec 27, 2020
Merged
alpaca-tc added a commit to alpaca-tc/ridgepole that referenced this pull request Dec 28, 2020
@alpaca-tc
Fixed add_index/remove_index in 6.1.0
81d60df 
Breaking changes introduced by rails/rails#39203
This was referenced Feb 8, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremy jeremy jeremy approved these changes

Assignees

No one assigned

Labels

activerecord

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kamipo @simi @jeremy