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

Set both a default radius and a dynamic radius column in near query #1597

Open
mnort9 opened this issue Dec 14, 2022 · 6 comments
Open

Set both a default radius and a dynamic radius column in near query #1597

mnort9 opened this issue Dec 14, 2022 · 6 comments

Comments

@mnort9
Copy link

mnort9 commented Dec 14, 2022

When using the near query and specifying a dynamic radius column, is it possible to also set a default radius if the dynamic radius is not set for that row?

Something like: Location.near([lat, lng], 25, :effective_radius)

This would be more convenient than setting a default value in the database that might change later.

@mnort9
Copy link
Author

mnort9 commented Dec 15, 2022

I don't see a way to do this, so I'm working on a PR.

Here's a couple options for the near arguments:

Location.near([lat, lng], :effective_radius, default_radius: 25)
Location.near([lat, lng], :effective_radius, fallback_radius: 25)

Optimally I think :effective_radius should be an option instead of an argument. It's a little wonky that the second argument can be either an integer or a symbol. This would flow a little better with the existing radius integer argument. So something like:

 Location.near([lat, lng], 25, radius_field: :effective_radius)

I don't think it'd be a breaking change if you still allow the db column as the second argument, but it might be a little confusing to allow both options.

@mnort9
Copy link
Author

mnort9 commented Dec 15, 2022

On second thought, the radius_field option would then require the second integer radius argument. So maybe the first default_radius option is better.

@alexreisner
Copy link
Owner

Hey @mnort9, and thanks for this. Can you tell me a little more about when and how you would use this feature? I'm a little confused about the use case for it.

@mnort9
Copy link
Author

mnort9 commented Dec 16, 2022

@alexreisner Sure. An example use case would be assigning leads to sales agents that have different geographic territories. Agents in rural areas might need a larger radius, so a dynamic radius is stored in the db.

When a lead needs to be assigned to an agent, we'd want a query like:

Find an agent who's territory this lead is in based on the agent's coordinates and (set radius OR the default radius of X miles).

The alternative would be to set the default radius for every sales agent in the db, but then if it ever needs to be changed, it needs to be backfilled. It'd be easier to simply set the fallback radius in the db query and can just adjust the query default.

@alexreisner
Copy link
Owner

@mnort9 sorry for the delay here. I've been thinking about this. Thanks for explaining the use case--it makes sense. I think I can merge, but just a couple things first:

  1. What do you think about naming the option :radius_column_default? I think it needs to be more specific than "default" as it applies only to a very specific usage of the method.
  2. Instead of using present?, can you test for a value using methods available outside of Rails/ActiveSupport?

@mnort9
Copy link
Author

mnort9 commented Mar 1, 2023

@alexreisner I think in this case :radius_column_default is fine for the keyword argument. I agree that being explicit is important since there are multiple contexts of "default".

Aside from this though, I think it may be worth considering moving :effective_radius to a keyword argument. The fact that the second argument could be either an integer or a symbol makes it a bit more cumbersome to add on functionality to the method.

This is a good example that we're having to use keyword arguments like :radius_column_default to get around it. I agree with specific naming in this case, but I think the flaw is in method arguments design.

An alternative to consider:

Location.near([lat, lng], 25) # Normal usage
Location.near([lat, lng], radius_field: :effective_radius) # With radius column
Location.near([lat, lng], 25, radius_field: :effective_radius) # With radius column and column default

This could be backwards compatible to still allow existing implementations to use the second argument as the radius column. Thoughts?

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