firebase / firebase-js-sdk

Firebase Javascript SDK
https://firebase.google.com/docs/web/setup
Other
4.86k stars 894 forks source link

Firestore GeoPoint query ignores longitude #826

Closed hamidrj closed 6 years ago

hamidrj commented 6 years ago

Firestore query on GeoPoint fields, compares the latitude but ignores longitude. Result contains all the documents with correct latitude, but all available longitude, ignoring the query input.

In the case of getting all the documents within map's bounding box, it returns with all the points vertically within the view, but horizontally, across the entire globe.

Following example should return only one location (40, -80), but it returns two including (50,-90) .

Relevant Code:

    const path='locations';

    const southWest=new firebase.firestore.GeoPoint( 35, -85);
    const northEast=new firebase.firestore.GeoPoint( 60, -75);

    Promise.all( [
      firebase.firestore().collection(path).doc().set( {location: new firebase.firestore.GeoPoint( 30, -70 )}),
      firebase.firestore().collection(path).doc().set( {location: new firebase.firestore.GeoPoint( 40, -80 )}),
      firebase.firestore().collection(path).doc().set( {location: new firebase.firestore.GeoPoint( 50, -90 )})
    ])
    .then(()=>{
      return firebase.firestore().collection(path)
       .where(`location`, '>' , southWest )
       .where(`location`, '<' , northEast )
       .get()
    })
   .then(snapshot=>{
     snapshot.docs.forEach(doc=>{
       console.log(JSON.stringify(doc.data()))
     })
   })
rsgowman commented 6 years ago

Unfortunately, this is currently working as intended. The comparison algorithm for GeoPoints compares the latitude first, and only in cases where the latitude is the same, does it examine the longitude.

The function in question is here:

https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/geo_point.ts#L77

Unfortunately, we don't support geoqueries yet, so it's not really possible to do this in a useful manner. Here's some useful stackoverflow discussions on the topic:

https://stackoverflow.com/questions/46607760/how-to-query-closest-geopoints-in-a-collection-in-firebase-cloud-firestore/ https://stackoverflow.com/questions/46553682/is-there-a-way-to-use-geofire-with-firestore

A possible workaround might be to run a where query for just the latitude, and then manually query the result set for longitude.

hamidrj commented 6 years ago

Thank you for reply. I believe the issue is the "or" operation (||) in GeoPoint._compareTo, it should instead be "and" operation (&&) in firebase-js-sdk/blob/master/packages/firestore/src/api/geo_point.ts#L77:

  _compareTo(other: GeoPoint): number {
    return (
      primitiveComparator(this._lat, other._lat) &&
      primitiveComparator(this._long, other._long)
    );
  }
wilhuff commented 6 years ago

The current implementation is correct. The semantics of GeoPoint comparison in Firestore are:

The comparison is done this way because for the backend just treats GeoPoints as a pair. The SDKs have to match the server behavior.

We recognize that this isn't a useful definition of queries, and except for concerns about data interoperability with Cloud Datastore, we wouldn't have published the GeoPoint type yet because of this. We intend to implement proper geo query support, though this will likely come after Firestore hits GA.

merlinnot commented 6 years ago

Just FYI, you can easily emulate geo-queries in Firestore using some space-filling curves.

hamidrj commented 6 years ago

@wilhuff , I'm afraid it doesn't work like the semantics you mentioned. In the example provided, the latitude is within the range and compare returns true, however according to your semantic, it should compare longitude next, but it doesn't.

hamidrj commented 6 years ago

@wilhuff, Appologies, I missed the "equal" in your semantic. so for comparison such as ">" or "<", are you implying the latitude should be equal?

wilhuff commented 6 years ago

primitiveComparator does three-way comparison, not equality checks.

Bounding box queries just aren't going to work. The results you're getting are expected. (50, -90) has a latitude within the range of 35 to 60. Longitude isn't even considered in this case.

Longitude would only be considered if you had a point whose latitude was equal to one of the boundaries of the range you're considering.

MvRemmerden commented 6 years ago

@wilhuff Is there any timeline on when proper geo query support will come?

wilhuff commented 6 years ago

Sorry, we have nothing to announce at this time.

I don't have a good written summary of this, but we did announce a few things at Cloud Next '18--these things are just around the corner. Nearly all of our effort lately has been focused on transitioning from a limited beta to something much more broadly useful. I realize this is no consolation for this specific feature, but at least you can see we've been busy :-).

ollyde commented 5 years ago

Any news on this?

For anyone else while we wait, GeoFirestore is really good, can handle millions of objects I've seen so far pretty well. But make a separate collection (for example 'GeoCacheUsers' or whatever and query that.)

wilhuff commented 5 years ago

From an external point of view nothing has really changed. We're much closer to GA for the features we have, but that feature set does not include geo queries. I can't comment on when geo queries are coming but they are definitely on our list of things to do.

georgeMorales commented 5 years ago

I have worked a bit with these libraries https://github.com/geofirestore/geofirestore-js https://github.com/codediodeio/geofirex and I have been able to implement queries but the data obtained can not be paged. Does anyone know another alternative? Without pagination you can not build something for the real world ...

lostpebble commented 5 years ago

I can't comment on when geo queries are coming but they are definitely on our list of things to do.

For those arriving here, I wouldn't hold your breath. I've been waiting for Geo Queries on Cloud Datastore (which Firestore is pretty much just a new iteration of) for over 5 years now. There are much better geo-native solutions out there if you want to get it done. Anything geo built on top of Firestore / Datastore is a hack at best, and from what I've seen vastly under-performs just picking the right tool the first time.

I really wish Google Cloud would give us a nice native option for this though - I mean they certainly do have the tech for it (Google Maps etc.) but they clearly aren't open to sharing that just yet...

willbattel commented 5 years ago

@wilhuff is there an issue tracker somewhere for this that we can go indicate our interest, like there are for some other GCP services? We're excited that Firestore is now in GA and we're eager to have Geoquery functionality. I know you're not able to give timelines/roadmaps but, as I'm sure you can understand, it makes it very challenging for some of us to be in the dark when creating applications on Firebase.

I'm tempted to use geohashing but it seems kind of silly...

puf commented 5 years ago

@willbattel Any built-in support would use geohashing or a similar mechanism under the hood. The best options right now are to either roll your own (as I've shown here, and Jeff's much more concise version here, or use one of the third-party GeoFire-derived libraries.

wilhuff commented 5 years ago

@willbattel There is a public issue tracker for Firestore.

Internal discussions around this have mostly centered on S2 geometry instead of geohashes but making this work efficiently requires disjunctive query support on the backend. That's where our focus is now. We won't add any formal support for geoqueries unless we can do it substantially better than what can be done just over the top of the existing API.

Note that part of not giving timelines and roadmaps is to avoid you relying on future features when making a decision to use Firestore today. Please express interest in this feature, but please don't count on it :-).

marcglasberg commented 5 years ago

@wilhuff Is there an issue for "disjunctive query support on the backend" so that we can vote for it?

wilhuff commented 5 years ago

All the issues are on the tracker I mentioned above. The issue I'm specifically referring to is support for IN queries. That would allow us to query for multiple regions in a single query.

Thaina commented 5 years ago

@wilhuff Would the disjunctive query are possible for every field or firebase server team focused on geopoint specifically ?

While I wish for geolocation query, I have so many usage around normal compound range query too

thebrianchen commented 5 years ago

@Thaina Disjunctive queries are a way to query for whether or not document fields contain specific discrete values and does not support ranges. There will be 2 types of IN queries:

// Query for bugs containing tags matching the provided array. let query = db.collection("bugs") .where("tags", "array-contains-any", ["high-priority", "internal"]);

Thaina commented 5 years ago

@thebrianchen So how that would be used in geolocation tough?

thebrianchen commented 5 years ago

@Thaina You can implement geolocation with IN queries using S2Geometry. To query for a specific area, you could perform an IN query across multiple S2 cells that cover the area. Disjunctive queries support the IN filters that are needed to create bounded areas. Note that Firestore does not implement an index of S2 cells from GeoPoint values so IN queries are building block for geolocation, but aren't sufficient for fully implementing the feature.

Thaina commented 5 years ago

@thebrianchen Which means you expect that, instead we could store the geopoint datastructure that firestore provided and could query geolocation with it, we need to always convert it to S2Geometry or something so we could query it?

So what it the point to use geopoint at all then?

I think currently now firestore team spend too much times on the workaround that really go around avoiding the actual feature we really need. We only need multi dimension query

wilhuff commented 5 years ago

This issue is a discussion about how bounding box queries don't work with GeoPoints, which remains true.

Further discussion about how to implement GeoQueries given Firestore's capabilities today or how to implement GeoQueries given IN queries belongs elsewhere. I'll reiterate that Firestore does not support GeoQueries and that except for concerns about interoperability with data already stored in Cloud Datastore, we would not have included a GeoPoint type at all.

Thaina commented 5 years ago

@wilhuff I see. Thank you for your clarification that we actually should avoid geopoint if we need to do geolocation related app