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

Using a changefeed with a geo intersection filter fails #4063

Closed
reidja opened this issue Apr 15, 2015 · 8 comments
Closed

Using a changefeed with a geo intersection filter fails #4063

reidja opened this issue Apr 15, 2015 · 8 comments
Assignees
Milestone

Comments

@reidja
Copy link

reidja commented Apr 15, 2015

When attempting to use an intersection filter in a changefeed the command fails:

Example command:

var bbox = r.polygon([25, 75], [75, 75], [75, 25], [25, 25]);
r.db('weather').table('metar').filter(r.row('location').intersects(bbox)).changes()

Error result:

RqlRuntimeError: Cannot call `changes` after a non-deterministic function in ...

Using rethinkdb 2.0.0+1 (ubuntu)

Would there be a way to force this command when you know your platforms are the same? (sort of like nonAtomic in updates?)

I'm willing to address this change with a pull request if someone could point me in the right direction?

@danielmewes
Copy link
Member

Thanks for opening the issue @reidja.

@AtnNn, @reidja and I had briefly discussed this on IRC.

intersects is non-deterministic because it can give different results on different servers. However in contrast to some other non-deterministic operations (like accessing another table, or random operations), it is deterministic per server.

@mlucy do changefeeds like these actually require that the function is deterministic across servers?

If not, I propose that we make the non-determinism flag for terms more fine-grained to allow this.
We should also consider how this will interact with resumable changefeeds in the future, especially for the case where we support resuming a changefeed after an old primary has gone down.

An ignoreNonDeterminism flag might work, but we should understand (and explain) very well which consequences that can have. In the case of updates we can provide nonAtomic because we can actually fall back to an alternative processing scheme which maintains all guarantees, except for atomicity.

@danielmewes danielmewes added this to the 2.1 milestone Apr 15, 2015
@danielmewes
Copy link
Member

@reidja Here's a quick work-around if you're building from source (instructions: http://rethinkdb.com/docs/build/). Apply this patch to the branch v2.0.x:

diff --git a/src/rdb_protocol/terms/geo.cc b/src/rdb_protocol/terms/geo.cc
index 6c16acf..81a196c 100644
--- a/src/rdb_protocol/terms/geo.cc
+++ b/src/rdb_protocol/terms/geo.cc
@@ -207,6 +207,7 @@ public:
     intersects_term_t(compile_env_t *env, const protob_t<const Term> &term)
         : geo_obj_or_seq_op_term_t(env, term, poly_type_t::FILTER, argspec_t(2)) { }
 private:
+    bool is_deterministic() const { return true; }
     scoped_ptr_t<val_t> obj_eval_geo(
             scope_env_t *env, args_t *args, const scoped_ptr_t<val_t> &v0) const {
         scoped_ptr_t<val_t> other = args->arg(env, 1);

As long as you use a single server and you don't use intersects in a secondary index function (geo indexes and getIntersecting are ok), it should be safe.

@mlucy
Copy link
Member

mlucy commented Apr 15, 2015

@danielmewes -- we can make it work as long as it's deterministic per-server. I agree we should make our notion of nodeterminism more granular to allow for this.

@danielmewes
Copy link
Member

Ok sounds like we have a solution then. Assuming it's not super complicated, we should try to implement it for 2.1.

@reidja
Copy link
Author

reidja commented Apr 16, 2015

This is awesome, worked like a charm. Thank you @danielmewes

jesseditson added a commit to jesseditson/rethinkdb that referenced this issue May 29, 2015
@danielmewes danielmewes modified the milestones: 2.2, 2.2-polish Aug 27, 2015
@knownasilya
Copy link

What's the status on this?

@danielmewes
Copy link
Member

@knownasilya Sorry, no update yet. We're trying to get this into the 2.2.0 release, but I can't promise it yet (there are a bunch of other really important features that we're trying to get in as well).

@encryptio
Copy link
Contributor

In review with @danielmewes, CR 3308

@danielmewes danielmewes modified the milestones: 2.2, 2.2-polish Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants