Skip to content
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

No way to add geodist into custom weight expression #686

Closed
kingcu opened this issue Jan 2, 2014 · 5 comments
Closed

No way to add geodist into custom weight expression #686

kingcu opened this issue Jan 2, 2014 · 5 comments

Comments

@kingcu
Copy link

kingcu commented Jan 2, 2014

Due to the order of execution of middleware, specifically geographer executing after sphinxql, there is no way to include geodist into the custom weight expression due to the ordering of select clauses. This was not an issue in TS 2.

Here's a simplified example from my project:

SELECT *, @weight * (1*geodist + 0.5*has_name) as custom_weight,
GEODIST(0.7944781882164005, -2.141068608238236, latitude, longitude) AS geodist
FROM route_core WHERE geodist BETWEEN 0.0 AND 80467.2
ORDER BY custom_weight DESC

This errors because the GEODIST clause falls after the weight clause, which references geodist.

After digging through the source for a bit, I cannot see a simple/clean way to change this behavior. I am happy to dive in and get aggressive (change execution of middleware, finalize the sphinxql statement/query after other middleware have executed) but thought I'd see if there was something easy I might have overlooked. Let me know if you have any suggestions, I'm happy to take a stab and submit a PR.

pat added a commit to pat/riddle that referenced this issue Jan 3, 2014
pat added a commit that referenced this issue Jan 3, 2014
This is so it can be referred to by other elements of the SELECT clause. Closes #686.
@pat
Copy link
Owner

pat commented Jan 3, 2014

Just fixed this in the develop branch, though it also requires the latest from Riddle's develop branch too.

If you want to give this a spin, that'd be great, but please note that develop is essentially TS 3.1.0 (I just want to have some contributions for capistrano scripts sorted), and so you should note the following commit messages: f93ae2a 4ef0296 88fd674

gem 'riddle',
  :git    => 'git://github.com/pat/riddle.git',
  :branch => 'develop',
  :ref    => '20f6026663'
gem 'thinking-sphinx',
  :git    => 'git://github.com/pat/thinking-sphinx.git',
  :branch => 'develop',
  :ref    => 'f72d2cc883'

@pat pat closed this as completed Jan 3, 2014
@kingcu
Copy link
Author

kingcu commented Jan 3, 2014

Thanks pat, this works as expected. I should have looked further than just TS for a solution.

There is an issue with riddle in the develop branch however, stemming from pat/riddle#79 - without '*' included in the select by default, I get an error:

undefined method `constantize' for nil:NilClass
/home/vagrant/.rvm/gems/ruby-2.0.0-p353/bundler/gems/thinking-sphinx-f72d2cc883a9/lib/thinking_sphinx/middlewares/active_record_translator.rb:58:in `block in results_for_models'

Looks like column names aren't being pulled in when searching. Changing my select to be "*, #{custom_select}" fixed it however, so not sure if this is the intended behavior or not. Obviously a separate issue, but thought I'd mention it.

@pat
Copy link
Owner

pat commented Jan 3, 2014

I think going forward this will be the necessary behaviour - which will be covered in the TS 3.1.0 release notes. I'd rather let people choose more carefully what they'd like to select, instead of enforcing the * in every query.

@kingcu
Copy link
Author

kingcu commented Jan 3, 2014

Makes perfect sense, though if it's possible (I am no sphinx/TS guru) to include the column names by default it should be considered, considering that it's an expectation to be able to get actual model instances back for a search. Another conversation for another ticket. Thanks for the lightning fast response on this one!

@pat
Copy link
Owner

pat commented Jan 4, 2014

It's not actually always expected that ActiveRecord objects will be returned - if you're using the search_for_ids record, you just get integers. There's also the possibility of getting the raw Sphinx results back as well.

I guess we'll see how things go once 3.1.0 is being used by a wider audience, maybe I'll have to rethink this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants