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

Added caching support to geohash_filter #6478

Closed
wants to merge 3 commits into from

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Jun 12, 2014

No description provided.

@s1monw
Copy link
Contributor

s1monw commented Jun 12, 2014

I don't think we should cache this by default. It's very unlikely that you have cache hits here. In-fact this filter should rather not be cached at all and never be turned into a big bitset since this fitlter is likely going to be very fast (it's really just 9 terms after all?) I am not sure if we should do this?

@jpountz
Copy link
Contributor

jpountz commented Jun 12, 2014

+1 to add the ability to cache it
-1 to cache it by default

@bleskes
Copy link
Contributor Author

bleskes commented Jun 12, 2014

I'm a bit on the fence regarding the default - I was triggered to add it by someone using the geohash_cell filter as the fastest way to limit the result to a certain area. They were using level 4 with neighbours which translates to 120KM by 60KM box and put it under a bool filter which causes it to generate a bit set anyway. Maybe cache by default on level 4 or up?

I don't have a good grip on the different usages of the geohash_cell filter, so if the people fill it shouldn't be cached, I'm good with that. Let me know.

@s1monw
Copy link
Contributor

s1monw commented Jun 12, 2014

yeah I'd let the user decide ie. follow @jpountz suggestions.

@s1monw s1monw removed the review label Jun 12, 2014
@bleskes
Copy link
Contributor Author

bleskes commented Jun 12, 2014

kk. Pushed another commit with the cache turned off + added cache/cachekey params to the java builder.

@bleskes bleskes added the review label Jun 12, 2014
@s1monw s1monw removed the review label Jun 12, 2014
@bleskes
Copy link
Contributor Author

bleskes commented Jun 12, 2014

@s1monw pushed another commit with an extra test

@s1monw
Copy link
Contributor

s1monw commented Jun 12, 2014

LGTM ;)

@bleskes bleskes closed this in 7fb16c7 Jun 12, 2014
bleskes added a commit that referenced this pull request Jun 12, 2014
Caching is turned off by default.

Closes #6478
@bleskes
Copy link
Contributor Author

bleskes commented Jun 12, 2014

merged. Thx @s1monw , @jpountz

@bleskes bleskes deleted the geohash_cell_caching branch June 12, 2014 20:24
@clintongormley clintongormley changed the title Added caching support to geohash_filter Geo: Added caching support to geohash_filter Jul 16, 2014
@clintongormley clintongormley added the :Analytics/Geo Indexing, search aggregations of geo points and shapes label Jun 7, 2015
@clintongormley clintongormley changed the title Geo: Added caching support to geohash_filter Added caching support to geohash_filter Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement v1.3.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants