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

Allows radius to be a symbol representing a column in DB #1107

Merged
merged 3 commits into from Aug 18, 2017

Conversation

Projects
None yet
3 participants
@leonelgalan
Contributor

leonelgalan commented Oct 11, 2016

Related to #334 (Near search using column for radius) and #449 (Allow symbol to mean radius comes from a db column)

I decided to not change .bounding_box based on the reasons discussed on #449, but instead build the bounding box based on the maximum radius and simply "filter" the results based on the distance between the min_radius (defaults to 0.0) and whatever is stored in radius_column. The performance hit is dependent on how variable the radiuses stored in the DB are.

Other concern discussed previously: Security. I haven't handled this one. But what I had in mind, was to compare the given radius_column to the model's column_names array: Place.column_names.column_names.include? radius_column.to_s, but decided not to implement it until we discussed it further.

I wasn't sure how to stub .maximum for ActiveRecord::Base. Don't hesitate correcting me on that one and pointing me in other directions for me to fix.

Finally, I didn't like how repeated the code look on the if/else and wrote this alternative implenetation instead, except it sacrificed readability. I decided to go with the original implementation instead:

conditions = [bounding_box_conditions + " AND (#{distance}) BETWEEN ? AND "]
if radius_column
  conditions[0] += radius_column.to_s
  conditions << min_radius
else
  conditions[0] += '?'
  conditions += [min_radius, radius]
end
@Dragsbaek

This comment has been minimized.

Dragsbaek commented Aug 14, 2017

What is the status of this?

@leonelgalan

This comment has been minimized.

Contributor

leonelgalan commented Aug 16, 2017

@Dragsbaek I'm running this on production and so far it's serve me well. I'm willing to rebase this and discuss it further if others see the value.

@alexreisner

This comment has been minimized.

Owner

alexreisner commented Aug 17, 2017

@leonelgalan thanks for this and sorry about the very long delay. A couple of things:

  1. What's the purpose of the maximum method?
  2. Seems like we should add the same capability for min_radius, for consistency. Any thoughts on that?
@leonelgalan

This comment has been minimized.

Contributor

leonelgalan commented Aug 17, 2017

  1. The one in the test helper? It's because we are simulating ActiveRecord::Base and I use ActiveRecord's maximum, which translates to the aggregating function MAX in the DB. I still wanted the bounding box to return only results within the maximum radius stored being requested.

  2. I haven't come up with that use case, only results farther than? With that query/use case, we don't have a bounding box anymore, so performance might be an issue. We have to filter out of the entire table, not just from the records within the box.

@alexreisner

This comment has been minimized.

Owner

alexreisner commented Aug 18, 2017

Sorry, I was really asking about why we're using MAX in the first place, but now I get it. The code is good. The problem is that it took me about 10 minutes to figure out how it works. I know you've already been thinking about readability (much appreciated!), so let's talk about it some more.

  1. The most confusing part of this--for me--was that the radius variable becomes something other than what's expected (used only for calculation of the bounding box, and then not referenced again). You've cleverly implemented this to modify as few lines of code as possible (thanks!) but I think this is a case where adding some more lines would add some helpful clarity.
  2. The one-line assignment of two variables is confusing. It's not obvious what it does, and what it does is crucial to understanding this whole thing: it introduces another path through the code.
  3. The alternate path through the code is discontinuous (you have to look in two different places to see it: bounding box calc, and SQL query assembly).

These are tricky things to solve, mainly because the code was already somewhat branched and ugly before the PR. Would really prefer to avoid a rewrite--I think we can modify the PR to clear things up. Here are my thoughts on doing that:

  1. The thing that triggers the alternate path through the code is radius.is_a?(Symbol), so let's use that consistently, rather than switching to if radius_column (although I completely understand why you did that, and yes, it could be argued it's more accurate...but hear me out).
  2. Don't overwrite radius. It's a method argument (user input), and hence this is confusing.
  3. Don't assign two variables on one line.
  4. Add some code comments, because aspects of this are rather complicated.

I propose something like this:

# If radius is a DB column name, bounding box should include
# all rows within the maximum radius appearing in that column.
# Note: performance is dependent on variability of radii.
bb_radius = radius.is_a?(Symbol) ? maximum(radius) : radius
b = Geocoder::Calculations.bounding_box([latitude, longitude], bb_radius, options)

...
if using_unextended_sqlite?
  conditions = bounding_box_conditions
else
  min_radius = options.fetch(:min_radius, 0).to_f
  # if radius is a DB column name,
  # find rows between min_radius and value in column
  if radius.is_a?(Symbol)
    c = "BETWEEN ? AND #{radius}"
    a = [min_radius]
  else
    c = "BETWEEN ? AND ?"
    a = [min_radius, radius]
  end
  conditions = [bounding_box_conditions + " AND (#{distance}) " + c] + a
end

Let me know your thoughts. And thanks again for this.

@leonelgalan

This comment has been minimized.

Contributor

leonelgalan commented Aug 18, 2017

@alexreisner, I think you already wrote it for me. I agree with your suggestions and with the proposed implementation. The only change I propose is the names of c and a. Together c and a are:

  • distance_conditions, or
  • radius_conditions

What they hold is:

  • b: A SQL Segment, and
  • a: Values or variables for the condition.

At the same time, they are quickly used, just like b is. So if you decide to keep those names I'm good with that too. I see some tests are failing when they shouldn't I'll take a look and give it a try.

@alexreisner

This comment has been minimized.

Owner

alexreisner commented Aug 18, 2017

I agree that those names are not very descriptive. But I'd argue that what they actually hold is:

  • c - a Condition string
  • a - an array of Arguments that get substituted into that string

So I'd be up for using more descriptive names, but I think they should, um, describe things in more or less that way. I think the way they are is alright too, only because they get used so quickly.

leonelgalan added some commits Aug 18, 2017

Proposed changes by @alexreisner:
- Keeping branching condition consistent
- Not overwrite the method argument: ‘radius’
- Not assigning two variables in a single line, and
- Add code comments where needed

By “proposed” I meant, I copy/paste them from his comment. But we are
in agreement.
@leonelgalan

This comment has been minimized.

Contributor

leonelgalan commented Aug 18, 2017

Tests for jruby are failing because the lack of bundler:

$ bundle --version
The program 'bundle' is currently not installed. To run 'bundle' please ask your administrator to install the package 'bundler'

And mysql builds with:

$ bundle exec rake
mysqladmin: DROP DATABASE geocoder_test failed;
error: 'Access denied for user 'travis'@'%' to database 'geocoder_test''

Should I take a look at those? Do you think rebasing might bring newer configuration that fixes the issues?

@alexreisner

This comment has been minimized.

Owner

alexreisner commented Aug 18, 2017

Ah, don't worry about those tests--they're unrelated. Going to merge this. Thanks!

@alexreisner alexreisner merged commit 5353fea into alexreisner:master Aug 18, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

ChangsoonKim added a commit to ChangsoonKim/geocoder that referenced this pull request Nov 23, 2017

Allows radius to be a symbol representing a column in DB (alexreisner…
…#1107)

* Allows radius to be a symbol representing a column in DB

* Proposed changes by @alexreisner:

- Keeping branching condition consistent
- Not overwrite the method argument: ‘radius’
- Not assigning two variables in a single line, and
- Add code comments where needed

By “proposed” I meant, I copy/paste them from his comment. But we are
in agreement.

* Fixes broken test in PR

zacviandier pushed a commit to teezily/geocoder that referenced this pull request Feb 9, 2018

Allows radius to be a symbol representing a column in DB (alexreisner…
…#1107)

* Allows radius to be a symbol representing a column in DB

* Proposed changes by @alexreisner:

- Keeping branching condition consistent
- Not overwrite the method argument: ‘radius’
- Not assigning two variables in a single line, and
- Add code comments where needed

By “proposed” I meant, I copy/paste them from his comment. But we are
in agreement.

* Fixes broken test in PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment