Skip to content
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

Omit redundant using: :btree for schema dumping #27981

Merged
merged 1 commit into from Feb 13, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Feb 12, 2017

No description provided.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

This will need a CHANGELOG entry since it will change a public facing file. Could you add it?

@@ -1,3 +1,7 @@
* Omit redundant `using: :btree` for schema dumping.

*Ryuta Kamizono*
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a CHANGELOG entry!

@rafaelfranca rafaelfranca merged commit fab91c6 into rails:master Feb 13, 2017
@kamipo kamipo deleted the omit_redundant_using_btree branch February 13, 2017 15:06
@matthewd
Copy link
Member

👍🏻 I prefer this as a ~solution for #26209. Ignoring values that might actually mean something worries me, whereas omitting the DBMS's native defaults (NB: not configured defaults) seems harmless -- and makes for a more streamlined schema file too.

... the generic schema dumper really shouldn't know about :btree, though 😕

@rafaelfranca
Copy link
Member

Yeah. When I reviewed I had the same option about the generic schema dumper knowing about btree but since postgresql and mysql have it I merged, but totally agree we should move to the index object if the using should be dumped or not. @kamipo willing to work on it?

@kamipo
Copy link
Member Author

kamipo commented Feb 13, 2017

Yeah, I'm working on it here.

diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb
index cf4495326b..63909ee3e7 100644
--- a/activerecord/lib/active_record/schema_dumper.rb
+++ b/activerecord/lib/active_record/schema_dumper.rb
@@ -184,12 +184,13 @@ def index_parts(index)
           index.columns.inspect,
           "name: #{index.name.inspect}",
         ]
+        default_index = @connection.default_index_type?(index)
         index_parts << "unique: true" if index.unique
         index_parts << "length: { #{format_options(index.lengths)} }" if index.lengths.present?
         index_parts << "order: { #{format_options(index.orders)} }" if index.orders.present?
         index_parts << "where: #{index.where.inspect}" if index.where
-        index_parts << "using: #{index.using.inspect}" if index.using && index.using != :btree
-        index_parts << "type: #{index.type.inspect}" if index.type
+        index_parts << "using: #{index.using.inspect}" if index.using && !default_index
+        index_parts << "type: #{index.type.inspect}" if index.type && !default_index
         index_parts << "comment: #{index.comment.inspect}" if index.comment
         index_parts
       end

@matthewd
Copy link
Member

Might be simpler to have the adapter set e.g. index.using to nil when it's the default. (But I haven't really looked.)

@rafaelfranca
Copy link
Member

Would not that hide information? I'd expect to using always return a value because the database always have that value.

@matthewd
Copy link
Member

We're not providing a general database introspection service; these objects only exist for us to construct the schema dump.

sonalkr132 added a commit to sonalkr132/rubygems.org that referenced this pull request Dec 20, 2018
`id: :serial` is added because `id: :uuid` support was introduced.
rails/rails#21762
`using: :btree` was redundant rails/rails#27981
Standardaized column spacing was removed to avoid diffs about
aligining columns.
rails/rails@df84e98#r22545295

```diff
-   t.string "zipcode", limit: 5
+   t.string "zipcode",           limit: 5
+   t.string "extra_information", limit: 255
```
jsugarman added a commit to ministryofjustice/Claim-for-Crown-Court-Defence that referenced this pull request Mar 6, 2019
jsugarman added a commit to ministryofjustice/Claim-for-Crown-Court-Defence that referenced this pull request Mar 6, 2019
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 13, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch, it appears Rails has dropped
the inclusion of `using: :btree` as well
(rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 13, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 15, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 15, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 17, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 17, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 22, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Jul 23, 2019
This is no longer needed with Rails 5.2. opclass is the attribute used
per https://github.com/rails/rails/pull/19090/files.

Now that we've removed the monkey patch and restored the Rails schema
dumper, it appears Rails has dropped the inclusion of `using: :btree` as
well (rails/rails#27981).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64529
njall added a commit to ElixirTeSS/TeSS that referenced this pull request Jul 30, 2019
Note: migration has removed redundant btree statements rails/rails#27981
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants