Omit redundant `using: :btree` for schema dumping by kamipo · Pull Request #27981 · rails/rails

kamipo · 2017-02-12T18:54:58Z

No description provided.

rafaelfranca

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

kamipo · 2017-02-13T15:00:49Z

activerecord/CHANGELOG.md

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


I added a CHANGELOG entry!

matthewd · 2017-02-13T15:38:49Z

👍🏻 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 · 2017-02-13T15:59:47Z

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 · 2017-02-13T16:04:43Z

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 · 2017-02-13T16:17:10Z

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 · 2017-02-13T16:30:31Z

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

matthewd · 2017-02-13T16:41:00Z

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

`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 ```

[remove redundant using: :btree](rails/rails#27981)

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

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

Note: migration has removed redundant btree statements rails/rails#27981

maclover7 added the activerecord label Feb 12, 2017

kamipo force-pushed the omit_redundant_using_btree branch from 7badc32 to 1faba42 Compare February 13, 2017 05:42

rafaelfranca approved these changes Feb 13, 2017

View reviewed changes

Omit redundant using: :btree for schema dumping

6d37cd9

kamipo force-pushed the omit_redundant_using_btree branch from 1faba42 to 6d37cd9 Compare February 13, 2017 14:12

kamipo commented Feb 13, 2017

View reviewed changes

rafaelfranca merged commit fab91c6 into rails:master Feb 13, 2017

kamipo deleted the omit_redundant_using_btree branch February 13, 2017 15:06

matthewd mentioned this pull request May 29, 2017

Migrations removing "using: :btree" from indexes #29269

Closed

chrisarcand mentioned this pull request May 30, 2017

Use Rails 5.1 schema spacing octobox/octobox#381

Merged

matthewd mentioned this pull request Jun 16, 2017

Nesting adapter specific table options #29472

Closed

matthewd mentioned this pull request Aug 2, 2017

Rails 5 schema.rb adding non-agnostic options. dev mysql -> test sqlite #26209

Closed

NikolausC mentioned this pull request Apr 17, 2018

WIP: Upgrade to Rails 5.1 sanger-archive/aker-projects-app#174

Closed

monfresh mentioned this pull request Jun 19, 2018

LG-309 Allow dynamic service provider updates in production 18F/identity-idp#2227

Merged

9 tasks

sonalkr132 mentioned this pull request Dec 20, 2018

Update db schema for rails 5 changes rubygems/rubygems.org#1860

Merged

jsugarman added a commit to ministryofjustice/Claim-for-Crown-Court-Defence that referenced this pull request Mar 6, 2019

Update db/schema.rb formatting following rails 5.2.3 upgrade

9a58fef

[remove redundant using: :btree](rails/rails#27981)

jsugarman added a commit to ministryofjustice/Claim-for-Crown-Court-Defence that referenced this pull request Mar 6, 2019

Update db/schema.rb formatting following rails 5.2.3 upgrade

b40004a

[remove redundant using: :btree](rails/rails#27981)

njall added a commit to ElixirTeSS/TeSS that referenced this pull request Jul 30, 2019

Added new tables for learning outcomes

fa6cec1

Note: migration has removed redundant btree statements rails/rails#27981

mkllnk mentioned this pull request May 25, 2021

Update schema syntax with Rails 5.2 openfoodfoundation/openfoodnetwork#7678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Omit redundant `using: :btree` for schema dumping#27981

Omit redundant `using: :btree` for schema dumping#27981
rafaelfranca merged 1 commit intorails:masterfrom
kamipo:omit_redundant_using_btree

kamipo commented Feb 12, 2017

Uh oh!

rafaelfranca left a comment

Uh oh!

kamipo Feb 13, 2017

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

rafaelfranca commented Feb 13, 2017

Uh oh!

kamipo commented Feb 13, 2017 •

edited

Loading

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

rafaelfranca commented Feb 13, 2017

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

		Ryuta Kamizono

Comments

Conversation

kamipo commented Feb 12, 2017

Uh oh!

rafaelfranca left a comment

Choose a reason for hiding this comment

Uh oh!

kamipo Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

rafaelfranca commented Feb 13, 2017

Uh oh!

kamipo commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

rafaelfranca commented Feb 13, 2017

Uh oh!

matthewd commented Feb 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kamipo commented Feb 13, 2017 •

edited

Loading