Add value ranges for GeoTile aggregation metrics metrics in the meta layer #71611
Conversation
iverase
added
the
:Analytics/Geo
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| @@ -226,9 +234,31 @@ public void setSize(int size) { | |||
| } | |||
|
|
|||
| public void setAggBuilder(AggregatorFactories.Builder aggBuilder) { | |||
| // TODO: validation | |||
| for (AggregationBuilder aggregation : aggBuilder.getAggregatorFactories()) { | |||
There was a problem hiding this comment.
Let's wait with this. I think I have some idea on how to make this more generic.
There was a problem hiding this comment.
I still wonder if 1) this should responsibility of the callers and 2) if we want to add these pipeline aggs, we need to throw an exception if we don't recognize the agg
There was a problem hiding this comment.
My hope is that a user of this API does not need to be an expert on Elasticsearch so we can provide this information from them. I am hoping to only support a subset of the aggregations, the ones that make sense.
| @@ -57,10 +57,10 @@ public void box(VectorTile.Tile.Feature.Builder featureBuilder, double minLon, d | |||
| } | |||
|
|
|||
| private int lat(double lat) { | |||
| return (int) (pointYScale * (VectorTileUtils.latToSphericalMercator(lat) - rectangle.getMinY())) + extent; | |||
| return (int) Math.round(pointYScale * (VectorTileUtils.latToSphericalMercator(lat) - rectangle.getMinY())) + extent; | |||
There was a problem hiding this comment.
We should still probably get this in.
|
Thanks @imotov , I merged your changes into this PR, and the output of the pipeline aggregations are now added to the meta layer using your approach (very cool) |
|
Nice! |
| // top term and percentile should be supported | ||
| throw new IllegalArgumentException("Unknown feature type [" + type + "]"); | ||
| } | ||
| addToXContentToFeature(featureBuilder, layerProps, aggregation); |
There was a problem hiding this comment.
Let's add the result of aggregations from the Content as well
|
I moved the methods that add properties to the layer to a hyper class and rename some of the methods. @imotov should be push this PR? |
| } | ||
| } | ||
| for (PipelineAggregationBuilder aggregation : aggBuilder.getPipelineAggregatorFactories()) { | ||
| // should not have pipeline aggregations |
Yes but I think there is nothing in the aggregation builder that tells you the type of aggregation? |
|
Indeed, we need to figure this part out. |
In order to support automatic styling, the consumers of vector tiles need to know the range of values added to the aggs layer. Therefore this PR adds this values ranges on the meta later. Values are computed using min / max pipeline aggregations.
So for example, there is the document counts range in the form:
In addition, for any metric aggregation there will be two entries: