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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow simultaneously searching on both tsvector & non-tsvector columns #152
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,11 +54,11 @@ def tsquery | |
end | ||
|
||
def tsdocument | ||
if options[:tsvector_column] | ||
if options[:tsvector_column] && columns.empty? | ||
column_name = connection.quote_column_name(options[:tsvector_column]) | ||
"#{quoted_table_name}.#{column_name}" | ||
else | ||
columns.map do |search_column| | ||
return_str = columns.map do |search_column| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's gotta be better variable name than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A better approach would be to create an array of expressions and then only do the |
||
tsvector = Arel::Nodes::NamedFunction.new( | ||
"to_tsvector", | ||
[dictionary, Arel.sql(normalize(search_column.to_sql))] | ||
|
@@ -70,6 +70,13 @@ def tsdocument | |
"setweight(#{tsvector}, #{connection.quote(search_column.weight)})" | ||
end | ||
end.join(" || ") | ||
|
||
if options[:tsvector_column] | ||
column_name = connection.quote_column_name(options[:tsvector_column]) | ||
return_str += " || #{quoted_table_name}.#{column_name}" | ||
end | ||
|
||
return_str | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -668,7 +668,54 @@ | |
end | ||
end | ||
|
||
context "using a tsvector column" do | ||
context "using a tsvector column and a single column" do | ||
with_model :ModelWithTsvectorAndSingle do | ||
table do |t| | ||
t.text 'name' | ||
t.text 'content' | ||
t.tsvector 'content_tsvector' | ||
end | ||
|
||
model { include PgSearch } | ||
end | ||
|
||
let!(:expected) { ModelWithTsvectorAndSingle.create!(:name => 'Adam', content: 'phooey') } | ||
let!(:unexpected) { ModelWithTsvectorAndSingle.create!(:name => 'Bob', :content => 'longcat is looooooooong') } | ||
|
||
before do | ||
ActiveRecord::Base.connection.execute <<-SQL.strip_heredoc | ||
UPDATE #{ModelWithTsvectorAndSingle.quoted_table_name} | ||
SET content_tsvector = to_tsvector('english'::regconfig, #{ModelWithTsvectorAndSingle.quoted_table_name}."content") | ||
SQL | ||
|
||
ModelWithTsvectorAndSingle.pg_search_scope :search_by_content_with_tsvector, | ||
:against => [:name], | ||
:using => { | ||
:tsearch => { | ||
:tsvector_column => 'content_tsvector', | ||
:dictionary => 'english' | ||
} | ||
} | ||
end | ||
|
||
it "should use to_tsvector in the query" do | ||
ModelWithTsvectorAndSingle.search_by_content_with_tsvector("tiles").to_sql.should =~ /to_tsvector/ | ||
end | ||
|
||
it "should find by the tsvector column" do | ||
ModelWithTsvectorAndSingle.search_by_content_with_tsvector("phooey").map(&:id).should == [expected.id] | ||
end | ||
|
||
it "should find by the single column" do | ||
ModelWithTsvectorAndSingle.search_by_content_with_tsvector("Adam").map(&:id).should == [expected.id] | ||
end | ||
|
||
it "should find by a combination of the two" do | ||
ModelWithTsvectorAndSingle.search_by_content_with_tsvector("Adam phooey").map(&:id).should == [expected.id] | ||
end | ||
end | ||
|
||
context "using a tsvector column with" do | ||
with_model :ModelWithTsvector do | ||
table do |t| | ||
t.text 'content' | ||
|
@@ -688,7 +735,7 @@ | |
SQL | ||
|
||
ModelWithTsvector.pg_search_scope :search_by_content_with_tsvector, | ||
:against => :content, | ||
:against => [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really confused about this -- it seems to me that if you're specifying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation, the idea is that the So this test was meant to show that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for why it's there, we still need to have an |
||
:using => { | ||
:tsearch => { | ||
:tsvector_column => 'content_tsvector', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might break backwards compatibility, see my comment in the spec below.