geotile_grid implementation #37842

nyurik · 2019-01-25T00:45:40Z

Implements geotile_grid aggregation, refactoring previous implementation #30240

This code uses the same base classes as geohash_grid agg, but uses a different hashing
algorithm to allow zoom consistency. Each grid bucket is aligned to Web Mercator tiles.

elasticmachine · 2019-01-25T00:46:01Z

Pinging @elastic/es-analytics-geo

polyfractal

Sorry, I know this is labeled WIP. I was just passing by and noticed something I wanted to leave a comment on, then left a few more. Feel free to ignore if you're still iterating on things :)

polyfractal · 2019-01-25T03:06:24Z

server/src/main/java/org/elasticsearch/common/geo/QuadkeyUtils.java

+    }
+
+    private static double tile2lon(final double x, final double tiles) {
+        return x / tiles * 360.0 - 180;


Tiny nit, should we put parenthesis around the components here just so the order of operation is immediately clear?

And not knowing anything about the algo, is there a chance tiles could ever be zero (e.g. divide by zero scenario)?

I refactored to put all logic in a single method. Tiles is a result of (4^N), so cannot be 0

polyfractal · 2019-01-25T03:11:18Z

server/src/main/java/org/elasticsearch/common/geo/QuadkeyUtils.java

+                Math.tan(Math.toRadians(lat)) + 1 / Math.cos(Math.toRadians(lat))
+            ) / Math.PI) / 2 * tiles);
+        if (xTile < 0)
+            xTile = 0;


Style nit: I think we try to avoid conditionals without braces, too easy to accidentally introduce bugs if you're not paying attention when touching the code.

Could replace with xTile = Math.min(xTile, 0), if you're looking for a concise alternative. Probably similar for the rest too.

thx, refactored.

polyfractal · 2019-01-25T03:17:57Z

.../src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalQuadkeyGrid.java

+ */
+public class InternalQuadkeyGrid extends InternalGeoGrid<InternalQuadkeyGridBucket> {
+
+    private static final String NAME = "geohash_grid";


Think this name needs to change? And ditto for javadoc on the class

thx, good catch. The NAME shouldn't have even been here - its a dup. FIxed here and in InternalGeoHashGrid.

polyfractal · 2019-01-25T03:31:22Z

server/src/main/java/org/elasticsearch/common/geo/QuadkeyUtils.java

+     * @param parser {@link XContentParser} to parse the value from
+     * @return int representing precision
+     */
+    public static int parsePrecision(XContentParser parser) throws IOException, ElasticsearchParseException {


Two questions:

Should we support unit-less precision levels? It looks like it adds a lot of autodetect magic below, and we've consistently been moving towards requiring units everywhere in ES. I don't know enough about the algo to say, but is there something tangible that we gain by supporting unit-less precision over just 10km, etc?

Is there a reason we fall back to the "old" style of xcontent parsing instead of using the newer static parsers here too?

for quadkey, most of the time it will be unitless - e.g .map visualizations will use current_zoom + 2 as the precision to get 16 dots per visualization tile.

I tried to make as few changes as possible to the existing code to make reviewing easier. This code is an exact copy of the GeoUtils.parsePrecision(), adjusted for different validation and parsing. We might consider rewriting them, but I think we should do them at the same time and possibly consolidate it for reuse.

@polyfractal, per @talevy 's suggestion, I removed non-integer precision - it is not needed by our use cases, and we do not know if they will be needed by other users. This also simplifies precision parsing.

This change allows other grid aggregations to reuse the same tests. The change mostly just moves code to the base classes, trying to keep changes to a bare minimum.

Implements support for the quadkey geo aggregation.

imotov · 2019-01-25T22:09:23Z

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java

+     * Create a new {@link InternalQuadkeyGrid} aggregation with the given name.
+     */
+    public static QuadkeyGridAggregationBuilder quadkeyGrid(String name) {
+        return new QuadkeyGridAggregationBuilder(name);


Maybe we can replace this one and the previous one with AggregationBuilders's method. Otherwise it is not tested.

@imotov I added more tests to use this function. Let me know if anything else is needed. Also, there were some unrelated CI issues, hopefully master merge will fix them.

imotov

LGTM. But since I didn't sleep for the last 27 hours, it might make sense for @polyfractal to have another look.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java

talevy

Overall looks good! left a few questions

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java

talevy · 2019-01-31T16:13:38Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java

+
+        int xTile = (int) Math.floor((normalizeLon(longitude) + 180) / 360 * tiles);
+
+        double latSin = Math.sin(Math.toRadians(normalizeLat(latitude)));


are we concerned that the latitude is not clipped here to ensure that the projection is square?

latitude = Math.min(Math.max(lat, -85.05112878), 85.05112878);

this is regarding:

The latitude and longitude are assumed to be on the WGS 84 datum. Even though Bing Maps uses a spherical projection, it’s important to convert all geographic coordinates into a common datum, and WGS 84 was chosen to be that datum. The longitude is assumed to range from -180 to +180 degrees, and the latitude must be clipped to range from -85.05112878 to 85.05112878. This avoids a singularity at the poles, and it causes the projected map to be square. [1]

and

Because the Mercator projects the poles at infinity, Google Maps cannot show the poles. Instead it cuts off coverage at 85.051129° north and south. This is not considered a limitation, given the purpose of the service. The value 85.051129° is the latitude at which the full map becomes a square...[2]

[1] https://docs.microsoft.com/en-us/bingmaps/articles/bing-maps-tile-system
[2] https://en.wikipedia.org/wiki/Web_Mercator_projection

also, should there be tests for points in this region?

We already test for points in that region, e.g. assertEquals(0x77FFFF4580000000L, longEncode(179.999, 89.999, 29));, but i will add a few more. The mathematics automatically takes care of those values, assigning geopoints in that region with the min/max Y value.

polyfractal

Left a few minor questions/comments, but LGTM otherwise. Will defer to @talevy on algo review :)

polyfractal · 2019-01-31T18:26:43Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java

+        // Zoom value is placed in front of all the bits used for the geotile
+        // e.g. when max zoom is 29, the largest index would use 58 bits (57th..0th),
+        // leaving 5 bits unused for zoom. See MAX_ZOOM comment above.
+        return ((long) precision << ZOOM_SHIFT) | ((long) xTile << MAX_ZOOM) | ((long) yTile);


Should we just make all the relevant variables/static defaults long instead of int, to save on all the casting? Not because it matters for performance, but just for readability.

I don't think we're relying on int overflows anywhere, so it should be safe to just use longs?

I don't care strongly, so feel free to ignore this (or if I missed somewhere else that needs them as ints). Was just something I noticed in passing :)

Thx, changed xTile & yTile to long.

polyfractal · 2019-01-31T18:29:21Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java

+            if (parts.length == 3) {
+                return zxyToGeoPoint(Integer.parseInt(parts[0]), Integer.parseInt(parts[1]), Integer.parseInt(parts[2]));
+            }
+        } catch (IllegalArgumentException e) {


Just curious about this arrangement... what's the reason for just catching IllegalArgumentException and then rethrowing it? Should we just catch any Exception? I'm assuming this is here to make the exception message a little nicer?

Yes, it was for better exception formatting. The new approach suggested by @talevy is to split checks into two groups, and (sadly) throw the same exception in two places.

polyfractal · 2019-01-31T18:32:05Z

.../main/java/org/elasticsearch/search/aggregations/bucket/geogrid/ParsedGeoTileGridBucket.java

+    }
+
+    static ParsedGeoTileGridBucket fromXContent(XContentParser parser) throws IOException {
+        return parseXContent(parser, false, ParsedGeoTileGridBucket::new, (p, bucket) -> bucket.hashAsString = p.textOrNull());


Can the hash be null? (e.g. curious about p.textOrNull(). Won't that cause keyToGeoPoint() to NPE later?

thx, changed it to p.text() instead

polyfractal · 2019-01-31T18:39:56Z

...st/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java

+
+package org.elasticsearch.search.aggregations.bucket.geogrid;
+
+public class GeoTileGridAggregatorTests extends GeoGridAggregatorTestCase<InternalGeoTileGridBucket> {


Could we add a few tests for invalid construction (expectThrows() on bad precision, bad zoom, etc) to this suite? I know it's indirectly tested in GeoTileGridParserTests, but that's done through JSON parsing and not the builder itself.

❤️

I added an extra test to GeoTileGridAggregatorTests to test precision. Also, added a few more tests to the utils class to check for invalid input vals.

talevy

LGTM if CI is happy.

thanks for all the iterations Yuri, I like where it landed!

nyurik requested a review from talevy January 25, 2019 00:45

nyurik added WIP :Analytics/Geo Indexing, search aggregations of geo points and shapes labels Jan 25, 2019

$polyfractal$

polyfractal reviewed Jan 25, 2019

View reviewed changes

nyurik added 2 commits January 25, 2019 00:02

Refactored GeoHashGrid unit tests

8848778

This change allows other grid aggregations to reuse the same tests. The change mostly just moves code to the base classes, trying to keep changes to a bare minimum.

Quadkey implementation

c927e15

Implements support for the quadkey geo aggregation.

nyurik force-pushed the quadkey-v3 branch from 003e2c4 to c927e15 Compare January 25, 2019 05:34

nyurik added 10 commits January 25, 2019 00:45

fix rebased artifacts

8590403

quadkey unit tests

8d63835

rename createInternalGeoHashGridBucket to createInternalGeoGridBucket

8201303

Merge branch 'geohashgrid_tests-v2' into quadkey-v3

79abc7c

add lat/lng adjustment, declarations

eebcb66

fix quadkey tests

acdb804

Merge remote-tracking branch 'origin/master' into quadkey-v3

477fc4f

QuadkeyUtils tests

c5fee30

Added quadkey docs, changed dflt to 8

3eeadb1

Merge remote-tracking branch 'origin/master' into quadkey-v3

9ea893e

nyurik changed the title ~~(WIP) Quadkey implementation~~ Quadkey implementation Jan 25, 2019

nyurik added 5 commits January 25, 2019 17:16

string.format fix

288178d

fix doc example

c1bc488

Merge remote-tracking branch 'origin/master' into quadkey-v3

6d69245

fix unittest

caf478e

renamed to geotile_grid

5ac5c82

nyurik changed the title ~~Quadkey implementation~~ geotile_grid implementation Jan 26, 2019

style fix

01b2a3f

nyurik removed the WIP label Jan 26, 2019

imotov reviewed Jan 28, 2019

View reviewed changes

nyurik added 2 commits January 28, 2019 13:17

Add GeoTileGridTests to AggregationsTests

c597529

Add geotile_grid test to ShardReduceIT

778452d

nyurik added 5 commits January 30, 2019 13:47

reformat asciidoc

01c2423

optimize geotile hashing algorithm

be1132f

stringEncode test extra by Tal

b6cd525

Docs, optimize lat/lng encoding

d040f41

Merge remote-tracking branch 'origin/master' into quadkey-v3

bfdd0e9

nyurik requested review from imotov and polyfractal January 31, 2019 05:44

Merge remote-tracking branch 'origin/master' into quadkey-v3

857f105

imotov approved these changes Jan 31, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java Outdated Show resolved Hide resolved

talevy suggested changes Jan 31, 2019

View reviewed changes

nyurik added 2 commits January 31, 2019 12:32

address review comments

5369cbb

extra test for polar coords (thx Tal)

8c0125c

$polyfractal$

polyfractal approved these changes Jan 31, 2019

View reviewed changes

nyurik added 3 commits January 31, 2019 15:08

address review comments

680a507

Merge remote-tracking branch 'origin/master' into quadkey-v3

f007644

test builder precision

880db72

nyurik requested a review from talevy January 31, 2019 20:27

talevy approved these changes Jan 31, 2019

View reviewed changes

nreese mentioned this pull request Jan 31, 2019

[Maps] geotile_grid aggregation elastic/kibana#29477

Closed

Merge remote-tracking branch 'origin/master' into quadkey-v3

a8f8183

nyurik merged commit f3cde06 into elastic:master Feb 1, 2019

nyurik deleted the quadkey-v3 branch February 1, 2019 00:11

nyurik added the v7.0.0 label Feb 1, 2019

talevy added the >feature label Feb 1, 2019

This was referenced Feb 1, 2019

(WIP) Implement geo_grid aggregation #37277

Closed

Implement quadkey geo grid aggregation #30240

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

alexfrancoeur mentioned this pull request Feb 7, 2019

Move Kibana map visualizations to geotile_grid elastic/kibana#30417

Closed

craigtaverner mentioned this pull request Mar 24, 2023

Comprehensive aggregation REST tests #26220

Open

69 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

geotile_grid implementation #37842

geotile_grid implementation #37842

nyurik commented Jan 25, 2019 •

edited

Loading

elasticmachine commented Jan 25, 2019

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Jan 25, 2019

nyurik Jan 25, 2019

$@polyfractal$ polyfractal Jan 25, 2019

nyurik Jan 25, 2019

$@polyfractal$ polyfractal Jan 25, 2019

nyurik Jan 25, 2019

$@polyfractal$ polyfractal Jan 25, 2019

nyurik Jan 25, 2019

nyurik Jan 30, 2019

imotov Jan 25, 2019

nyurik Jan 30, 2019

imotov left a comment

talevy left a comment •

edited

Loading

talevy Jan 31, 2019

talevy Jan 31, 2019

nyurik Jan 31, 2019

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Jan 31, 2019

nyurik Jan 31, 2019

$@polyfractal$ polyfractal Jan 31, 2019

nyurik Jan 31, 2019

$@polyfractal$ polyfractal Jan 31, 2019

nyurik Jan 31, 2019

$@polyfractal$ polyfractal Jan 31, 2019

nyurik Jan 31, 2019

talevy left a comment


		int xTile = (int) Math.floor((normalizeLon(longitude) + 180) / 360 * tiles);

		double latSin = Math.sin(Math.toRadians(normalizeLat(latitude)));


		package org.elasticsearch.search.aggregations.bucket.geogrid;

		public class GeoTileGridAggregatorTests extends GeoGridAggregatorTestCase<InternalGeoTileGridBucket> {

geotile_grid implementation #37842

geotile_grid implementation #37842

Conversation

nyurik commented Jan 25, 2019 • edited Loading

elasticmachine commented Jan 25, 2019

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov left a comment

Choose a reason for hiding this comment

talevy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talevy left a comment

Choose a reason for hiding this comment

nyurik commented Jan 25, 2019 •

edited

Loading

$@polyfractal$ polyfractal left a comment

talevy left a comment •

edited

Loading

$@polyfractal$ polyfractal left a comment