mrkamel / search_cop

Search engine like fulltext query support for ActiveRecord
MIT License
828 stars 39 forks source link

Citext support on Postgres #40

Open dolzenko opened 6 years ago

dolzenko commented 6 years ago

Citext column type is useful on Postgres for case-insesitive comparisons [0], currently searching for such column fails with

     NameError:
       uninitialized constant SearchCopGrammar::Attributes::Citext

placing following code in initializer fixes it (I could add a PR but not sure about tests)

module SearchCopGrammar
  module Attributes
    class Citext < String; end
  end
end

module SearchCop
  module Visitors
    class Visitor
      alias :visit_SearchCopGrammar_Attributes_Citext :visit_attribute
    end
  end
end

[0] https://www.postgresql.org/docs/10/static/citext.html

mrkamel commented 6 years ago

ok, thx, i'll look into it.

mrkamel commented 6 years ago

i'm not quite sure i want to add support for a database specific datatype, that needs to be explicity enabled (postgres module). You can easily get the same (search-wise) via a functional index. Why not use that instead?

dolzenko commented 6 years ago

Understandable, I think I went for citext instead of index mostly for 'purist' reasons

https://www.postgresql.org/message-id/CAO8h7BJMX5V1TqzScTx2Nr1jH5iUFG8A071y-g1b_kdzpu9PDw%40mail.gmail.com

Conceptually the index is an implementation detail and uniqueness should be associated only with constraints.

mrkamel commented 6 years ago

ok, well, then we probably need to define datatype aliases/mappings or some kind of plugin architecture, such that the main code base stays clean. However, the benefit of adding support for aliases/mappings seems limited. Take #28 for instance, which can't be easily mapped to an existing datatype due to the inet prefix required within queries. Adding a plugin architecture doesn't seem to provide much benefit either, because users will easily end up in sql injection issues as they need to handle all the sql generation themselves.