Support for Geospatial Commands

[ayosec]

Add support for geospatial commands, available since Redis 3.2.0.

Added commands

• GEOADD
• GEODIST
• GEOHASH
• GEOPOS
• GEORADIUS
• GEORADIUSBYMEMBER

Changes in Travis

The Redis installed by default in Travis is 3.0, so these commands are not available.

I tried to use ppa:chris-lea/redis-server, but it still installs Redis 3.0.

To use Redis 3.2.9, it is downloaded and built from sources. The build time varies between 20 and 50 seconds.

common module for tests

In be773d7, I moved some code from tests/test_basic.rs to a common module, so it can be used in a new tests/test_geospatial.rs test. It does not change anything, but add some noise in the diff.

Example

Full example is available in examples/geospatial.rs.

// Add some members to the geospatial index.

let added: isize = con.geo_add(
    "gis",
    &[
        (geo::Coord::lon_lat("13.361389", "38.115556"), "Palermo"),
        (geo::Coord::lon_lat("15.087269", "37.502669"), "Catania"),
        (geo::Coord::lon_lat("13.5833332", "37.316667"), "Agrigento"),
    ],
)?;

println!("[geo_add] Added {} members.", added);

// Get the position of one of them.

let position: Vec<geo::Coord<f64>> = con.geo_pos("gis", "Palermo")?;

println!("[geo_pos] Position for Palermo: {:?}", position);

// Search members near (13.5, 37.75)

let options = geo::RadiusOptions::default()
    .order(geo::RadiusOrder::Asc)
    .with_dist()
    .limit(2);

let items: Vec<geo::RadiusSearchResult> =
    con.geo_radius("gis", 13.5, 37.75, 150.0, geo::Unit::Kilometers, options)?;

for item in items {
    println!(
        "[geo_radius] {}, dist = {} Km",
        item.name,
        item.dist.unwrap_or(f64::NAN)
    );
}

[badboy]

Wow, thanks for the effort. That's quite some code and I'd like to get this in, but I currently don't have all the capacity to review this, so it might take some time.

I rekicked the failed test. It was an unrelated problem it seems.

[categulario]

It seems like the names of the new functions are not consistent with the existent ones, for example:

• geo_add instead of geoadd
• geo_add is adding multiple items. Maybe geoadd_multiple and geoadd?

just to keep things regular

[grippy]

@badboy There was a lot of work that looks like it went into this PR and almost 20 months later it's still sitting here. At least 75% of this PR is related to tests, examples, and docs. So the actual code review is pretty small -- everything should be fixable in followup PR's.

Obviously, this PR has a few merge conflicts now, but what's the hold up on solving those and merging this? I understand it takes time to review and at this point @ayosec might've moved onto other things by now. But sans feedback it's hard for anyone else to pick this up and drive it to completion.

Anyway, I'm mostly asking because I'm evaluating Redis Streams and noticed there's an issue for adding this set of commands to this lib. Just want to make sure if I start working on this issue that the PR won't sit idle once it's ready.

[badboy]

I do appreciate the effort, yet I can't make any promises on any timeline.

I work on redis-rs in my free time, I am not a heavy Redis user anymore and quite honestly have never once used the geo functionality. That's why reviewing this was always rather low on my priority list.

It doesn't matter if 75% of the PR is not the actual code, I will still end up reviewing it (after all, if it lands, people will expect me to maintain it anyway). Especially given the fact that the tests are the way I can verify the code does what it is supposed to.
By now this PR unfortunately bit-rotted and is not mergeable in its current state, even if reviewed. I don't have the time to rebase it & fix against the current code.

I can't make any promises for your work either for similar reasons. If you see the need for that I don't want to discourage you.
Small PRs are always much more welcome than a huge PR that includes everything of course.

[grippy]

@badboy Thanks for the reply. I completely understand not wanting to maintain something you don't use anymore -- especially if you don't have the experience with the new commands. Have you considered adding more maintainers or handing this project off to someone else?

There's at least a dozen new commands for Streams (with varying complexity). I don't know how beneficial it would be to add them piecemeal over a series of PR's. However, if it makes reviewing them easier, I'm willing to give it a shot for one command.

[badboy]

There's actually more people with commit rights on this repository and I have some docs in the pipeline to explain better how to actually do releases and stuff, so that anyone could easily do that.

Re streams: discussing is better placed in the relevant issue, but IMO it should be possible to implement streams as its own crate on top of this, this way you wouldn't even be blocked on me or other maintainers.

[ayosec]

I updated the patch with the current master.

Tests in AppVeyor failed because the Redis server is too old, and doesn't support geospatial commands.

[badboy]

Thanks for the updating.

Tests in AppVeyor failed because the Redis server is too old, and doesn't support geospatial commands.

That means we need to somehow disable the tests on AppVeyor somehow (back then for redis-rb we did version detection for the tests, hmmm...)

[Marwes]

I would suggest adding a test-geospatial default feature and then disable default features on appveyor and explicitly enable every feature but test-geospatial. That way the problem gets limited to appveyor only, without needing to do any version detection.

[badboy]

Thanks again for the work that went into this.
I left comments inline, this is on a very good way and I'd like to merge this before the next release.

Re naming: I think having the underscores in function names makes it much easier to read than georadiusbymember

[ayosec]

I think that all suggestions are applied. Is there anything else to do?

[badboy]

I think that all suggestions are applied. Is there anything else to do?

A final round of review. Doing that now!

[badboy]

Oh we don't need this anymore at all? Nice!

[badboy]

We should document this feature, we can do that in a follow-up

[badboy]

Merged and added to the changelog