Awesome! Thanks for the review Mike! Updated patch to address comments is attached.
this is mixing up separate changes I think? One change is cutover to doc values for the point filtering of each lat/lon, and the other is changing the lower detail level and higher prec step?
Indeed. The former gives search performance improvements, the latter gives indexing performance improvements. I can split these into 2 patches if desired? That way we can separately investigate the impact of changing the precision value?
Shouldn't you iterate through all values and accept the docs if any of them were in-bounds? Can you add a test case that exposes this?
++ Thanks for pointing that out! I had intended to change that. Fixed in the attached patch - I also added explicit multi-valued documents and testing to cover this. Random multi-valued documents would be nice, though I don't think it blocks the patch?
Couldn't GeoPointTermsEnum just have an abstract acceptLatLon method?
++ I had gone back and forth about this a couple times. With DV post filtering it makes more sense to now have GeoPointTermsEnum be abstract with an abstract postFilter method. Before, most of the logic was shared, only crosses and within were fully overridden in Poly and Distance query classes. I went ahead and made the change in the attached patch.
It looks like you continue using full precision terms to approximate the shape's boundary right?
No, the Range instances are now using lower precision terms for the boundaries (up to PRECISION_STEP * MAX_SHIFT - which works out to no higher than level 18). GPTQConstantScoreWrapper iterates the docIds in the postings list. So full precision terms (32 > level >18) are never used (really just wasting space in the index). I suppose I could modify GeoPointField to only index up to a shift of PRECISION_STEP * MAX_SHIFT and further reduce the index size?