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

Support Descending Indexes for MySQL #28773

Merged
merged 2 commits into from Apr 16, 2017

Conversation

Projects
None yet
4 participants
@kamipo
Member

kamipo commented Apr 15, 2017

MySQL 8.0.1 and higher supports descending indexes: DESC in an index
definition is no longer ignored.

See https://dev.mysql.com/doc/refman/8.0/en/descending-indexes.html.

table, name,
unique = false,
columns = [],
lengths: {},

This comment has been minimized.

@maclover7

maclover7 Apr 15, 2017

Member

Why use keyword arguments for only some of the fields?

@maclover7

maclover7 Apr 15, 2017

Member

Why use keyword arguments for only some of the fields?

This comment has been minimized.

@kamipo

kamipo Apr 15, 2017

Member

These keyword arguments are optional, not always used by any adapters.

@kamipo

kamipo Apr 15, 2017

Member

These keyword arguments are optional, not always used by any adapters.

This comment has been minimized.

@maclover7

maclover7 Apr 15, 2017

Member

I'm probably just missing something, but why are we making this change in the PR?

@maclover7

maclover7 Apr 15, 2017

Member

I'm probably just missing something, but why are we making this change in the PR?

This comment has been minimized.

@kamipo

kamipo Apr 15, 2017

Member

I sometimes hit the incorrect initializing value issue (e.g. in #26745).
lengths and orders should be initialized as a Hash. But now these are initialized as [] in mysql2 and postgresql adapters, nil in sqlite3 adater (for the reason, we use index.lengths.present? instead of index.lengths.any?).

https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L338
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L143
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L313
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/schema_dumper.rb#L188-L189

I hit the issue again in this change. I thought that the issue is related thing in this change. But if we prefer to separate the fixing issue, I can create new PR that fixing the issue, then rebase this PR on master once the PR has merged.

@kamipo

kamipo Apr 15, 2017

Member

I sometimes hit the incorrect initializing value issue (e.g. in #26745).
lengths and orders should be initialized as a Hash. But now these are initialized as [] in mysql2 and postgresql adapters, nil in sqlite3 adater (for the reason, we use index.lengths.present? instead of index.lengths.any?).

https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L338
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L143
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L313
https://github.com/rails/rails/blob/v5.1.0.rc1/activerecord/lib/active_record/schema_dumper.rb#L188-L189

I hit the issue again in this change. I thought that the issue is related thing in this change. But if we prefer to separate the fixing issue, I can create new PR that fixing the issue, then rebase this PR on master once the PR has merged.

@@ -2,6 +2,41 @@ module ActiveRecord
module ConnectionAdapters
module SQLite3
module SchemaStatements # :nodoc:
# Returns an array of indexes for the given table.

This comment has been minimized.

@maclover7

maclover7 Apr 15, 2017

Member

Can we put this method back where it was? I know it's technically supposed to be here, but it shouldn't be moved in this commit/PR (unrelated to adding support for something in MySQL)

@maclover7

maclover7 Apr 15, 2017

Member

Can we put this method back where it was? I know it's technically supposed to be here, but it shouldn't be moved in this commit/PR (unrelated to adding support for something in MySQL)

This comment has been minimized.

@kamipo

kamipo Apr 15, 2017

Member

Should I separate a commit/PR that refactoring indexes (changing IndexDefinition, consistent placed, and tiny tweaks) in this commit/PR?

@kamipo

kamipo Apr 15, 2017

Member

Should I separate a commit/PR that refactoring indexes (changing IndexDefinition, consistent placed, and tiny tweaks) in this commit/PR?

This comment has been minimized.

@maclover7

maclover7 Apr 15, 2017

Member

Yep, I guess a separate commit in the PR is fine. My preference would be to leave things the way they are, since, just as an example, moving #indexes for the SQLite3 adapter is kind of a cosmetic change, which we generally try and avoid 😢

@maclover7

maclover7 Apr 15, 2017

Member

Yep, I guess a separate commit in the PR is fine. My preference would be to leave things the way they are, since, just as an example, moving #indexes for the SQLite3 adapter is kind of a cosmetic change, which we generally try and avoid 😢

kamipo added some commits Apr 15, 2017

Refactor `indexes` things in connection adapters
* Use keyword arguments in `IndexDefinition` to ease to ignore unused
  options and to avoid to initialize incorrect empty value.
* Place it in `SchemaStatements` for consistency.
* And tiny tweaks.
Support Descending Indexes for MySQL
MySQL 8.0.1 and higher supports descending indexes: `DESC` in an index
definition is no longer ignored.

See https://dev.mysql.com/doc/refman/8.0/en/descending-indexes.html.

@matthewd matthewd merged commit e1e3be7 into rails:master Apr 16, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:support_descending_indexes branch Apr 16, 2017

@maclover7 maclover7 removed the needs work label Apr 17, 2017

koic added a commit to koic/oracle-enhanced that referenced this pull request Apr 18, 2017

@voltechs

This comment has been minimized.

Show comment
Hide comment
@voltechs

voltechs May 2, 2017

Any reason why we're not returning lengths in the IndexDefinition? I've been working on a gem and the curious omission of this has got me stumped.

Any reason why we're not returning lengths in the IndexDefinition? I've been working on a gem and the curious omission of this has got me stumped.

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