Skip to content

Add support for char(n) columns. #3402

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

Closed
wants to merge 1 commit into from
Closed

Conversation

oggy
Copy link
Contributor

@oggy oggy commented Oct 22, 2011

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

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

@larzconwell
Copy link
Contributor

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

@steveklabnik
Copy link
Member

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

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

@larzconwell
Copy link
Contributor

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

@steveklabnik
Copy link
Member

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

@larzconwell
Copy link
Contributor

Yep true

@oggy
Copy link
Contributor Author

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.

@steveklabnik
Copy link
Member

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

@steveklabnik
Copy link
Member

@oggy ping! this is still out of date.

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

    add_column :people, :zip, :string, :limit => 5, :fixed => true
@oggy
Copy link
Contributor Author

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.

@lukaszx0
Copy link
Member

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?

@steveklabnik
Copy link
Member

@carlosantoniodasilva , @tenderlove or @jonleighton , care to weigh in?

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

@jonleighton
Copy link
Member

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

@jonleighton
Copy link
Member

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.

@lukaszx0
Copy link
Member

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

@oggy
Copy link
Contributor Author

oggy commented Mar 15, 2013

@strzalek Please go ahead - thanks!

@lukaszx0
Copy link
Member

@oggy ok

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

@Empact
Copy link
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

@matthewd
Copy link
Member

Closing in favour of #11516

@matthewd matthewd closed this Apr 24, 2014
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.

7 participants