Add support for char(n) columns. #3402

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

oggy commented Oct 22, 2011

Pass :fixed => true for string columns, e.g.:

add_column :people, :zip, :string, :limit => 5, :fixed => true
Contributor

larzconwell commented Apr 29, 2012

What does fixed do exactly? Force it to be limited to the limit??

Member

steveklabnik commented Apr 30, 2012

This pull request no longer merges cleanly. Mind rebasing it?

Also, note that you can just do add_column :people, :zip, "char(12)", IIRC.

Contributor

larzconwell commented Apr 30, 2012

I think if we can do the same thing as easily as that, we should just close it (:

Member

steveklabnik commented Apr 30, 2012

I would agree, but that's for @oggy, @tenderlove, or another person working on ActiveRecord to decide.

Contributor

larzconwell commented Apr 30, 2012

Yep true

Contributor

oggy commented May 20, 2012

CHAR(n) means the column will occupy exactly n bytes per row. The advantage is the string can be stored slightly more compactly, as length information does not have to be encoded into the value. (MySQL, for example, requires 1 extra byte per row for VARCHAR columns.) It also makes the schema a little more self-documenting, and can potentially facilitate construction of length validations through metaprogramming.

As has been suggested, one can specify the type as CHAR(n), however:

  • The schema will silently fail to roundtrip correctly through db/schema.rb -- you need to remember to use the SQL dump format.
  • You can't reflect on fixedness in a standard way (e.g., postgres does it slightly differently).
  • Providing the option encourages good DB practices, rather than making me feel like I have to subvert ActiveRecord just to be a good DB citizen.

It's not a huge deal, but because of the above, because char is ANSI, and because the patch is small and simple, I think proper char support should be added to core.

I've rebased my branch on current master. Thanks for considering this.

Member

steveklabnik commented Sep 15, 2012

Needs more rebasing. :/ I think I'm 👍 on this now, looking back.

Member

steveklabnik commented Nov 17, 2012

@oggy ping! this is still out of date.

@oggy oggy Add support for char(n) columns.
Pass :fixed => true for string columns, e.g.:

    add_column :people, :zip, :string, :limit => 5, :fixed => true
8b4730d
Contributor

oggy commented Nov 18, 2012

@steveklabnik updated

Looking at this again, it would be nice to replace those long parameter lists (type, limit, precision, scale, fixed) with a hash or object. As it is, this will likely break database adapters outside of the core 3. Getting rid of the parameter lists would still break things this time, but would future-proof it better. Let me know.

Member

lukaszx0 commented Mar 13, 2013

Hey, I've rebased this PR again, on my own. Ran tests against mysql, pg, sqlite. It's ok.

It's in my fork (with original commit author of course).

@steveklabnik if nothing changed and this is going to be merged, you can find it here: https://github.com/strzalek/rails/tree/oggy-char-columns
I don't want to create PR mess opening new one, so maybe you could merge it by hand?

Member

steveklabnik commented Mar 13, 2013

@jonleighton jonleighton commented on the diff Mar 15, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -544,6 +544,10 @@ def type_to_sql(type, limit = nil, precision = nil, scale = nil) #:nodoc:
end
elsif (type != :primary_key) && (limit ||= native.is_a?(Hash) && native[:limit])
+ if fixed
+ column_type_sql.sub!(/\Acharacter varying/, 'character') # postgres
+ column_type_sql.sub!(/\Avar/, '') # everyone else
@jonleighton

jonleighton Mar 15, 2013

Member

Can this be handled in the actual database adapters? I.e. by putting PG specific code in the PG adapter.

Maybe by adding e.g. a :fixed_string entry to the native_database_types hash, and using "fixed_#{type}".to_sym if fixed is true.

Member

jonleighton commented Mar 15, 2013

Seems okay I think, if my comment above is addressed.

Member

jonleighton commented Mar 15, 2013

I'd like to get @tenderlove's opinion before we merge this though. Also another idea is we could have a :fixed_string type rather than having :fixed as an option.

Member

lukaszx0 commented Mar 15, 2013

Hey @oggy, care to adressing Jon's commets? If not, for any reason, I can finish it.

Contributor

oggy commented Mar 15, 2013

@strzalek Please go ahead - thanks!

Member

lukaszx0 commented Mar 15, 2013

@oggy ok

@tenderlove which direction should I take? Stick with proposed :fixed => true option, or introduce fixed_string native type as @jonleighton suggested?

Contributor

Empact commented Jul 20, 2013

Noticed this had been sitting around for a while and implemented a version with a :fixed_string type rather than a :fixed option in hopes of helping out. #11516

Owner

matthewd commented Apr 24, 2014

Closing in favour of #11516

matthewd closed this Apr 24, 2014

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