index geo payload #366
Merged
index geo payload #366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agourlay
force-pushed
the
index-geo-payload
branch
from
8322560
to
3be36bd
Compare
|
WDYT degarding splitting of |
|
@generall I agree with you sentiment, I will split the geohash functions from the geo indexing 👍 |
agourlay
force-pushed
the
index-geo-payload
branch
from
3be36bd
to
dce84a7
Compare
generall
reviewed
Mar 9, 2022
agourlay
force-pushed
the
index-geo-payload
branch
2 times, most recently
from
8634f4a
to
fec7d38
Compare
agourlay
force-pushed
the
index-geo-payload
branch
from
908eb0c
to
257466a
Compare
agourlay
force-pushed
the
index-geo-payload
branch
from
214ae9d
to
b8895be
Compare
generall
requested changes
Mar 14, 2022
| values_count | ||
| } else { | ||
| // between 0 and 1 (there is at least one payload per point) | ||
| let payload_per_point = total_points_count as f64 / self.payload_count as f64; |
There was a problem hiding this comment.
It should be called point_per_payload, isn't it?
| } else { | ||
| // between 0 and 1 (there is at least one payload per point) | ||
| let payload_per_point = total_points_count as f64 / self.payload_count as f64; | ||
| values_count.saturating_sub((values_count as f64 * payload_per_point) as usize) |
There was a problem hiding this comment.
That could be used as some approximation to expected value, but not min
| primary_clauses: vec![], | ||
| min, | ||
| exp: expected_count, | ||
| max: expected_count, |
There was a problem hiding this comment.
But max != expected in this case. For max we should guarantee, that it is impossible to match more than this amount of points with this query
| values_count.saturating_sub((values_count as f64 * payload_per_point) as usize) | ||
| }; | ||
| // don't overflow max number of points | ||
| let expected_count = total_points_count.min(values_count); |
agourlay
commented
Mar 17, 2022
| encode((lon, lat).into(), GEOHASH_MAX_LENGTH) | ||
| } | ||
|
|
||
| pub fn geo_hash_to_box(geo_hash: &GeoHash) -> GeoBoundingBox { | ||
| let rectangle = decode_bbox(geo_hash).unwrap(); | ||
| let top_left = GeoPoint { | ||
| lat: rectangle.max().x, | ||
| lon: rectangle.min().y, | ||
| lat: rectangle.max().y, |
There was a problem hiding this comment.
mapping x/y to lat/lon is so error prone 😞
agourlay
commented
Mar 17, 2022
agourlay
commented
Mar 17, 2022
agourlay
commented
Mar 18, 2022
agourlay
commented
Mar 18, 2022
| if n < 1.0 { | ||
| return 1.0; // By definition | ||
| } | ||
| (2. * PI * n).sqrt().ln() + n * (n / E).ln() |
There was a problem hiding this comment.
the std lib defines also TAU that we could use here
/// The full circle constant (τ)
///
/// Equal to 2π.
#[stable(feature = "tau_constant", since = "1.47.0")]
pub const TAU: f64 = 6.28318530717958647692528676655900577_f64;
agourlay
commented
Mar 18, 2022
generall
approved these changes
Mar 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new indexing scheme for geo payloads #362
Notes:
geo_indexfile has been renamed togeo_hashto make space for the newPersistedGeoMapIndex