-
Notifications
You must be signed in to change notification settings - Fork 25k
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
[Transform] add support for geo_line aggregation in pivot function #69299
[Transform] add support for geo_line aggregation in pivot function #69299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hendrikmuhs since the license of this agg is enforced by the spatial plugin, users will run into a licensing error if they try to PUT
a transform but they don't have a license that has the geo_line
agg.
I think that is good enough as we do attempt to validate and parse on PUT
.
If they are on a trial, and then trial times out while the transform is running, we will mark it as failed, but given we have checkpoints, etc. recovering from that scenario is not too bad (they won't lose data).
...src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java
Outdated
Show resolved
Hide resolved
e063c20
to
7a0e99f
Compare
API error messages on license failures:
Error from the transform wizard in kibana: Both of these seem pretty self-explanatory to me. |
Pinging @elastic/ml-core (Team:ML) |
@elastic/ml-ui ping. It would be neat to get geo data to be visualized in the transform wizard (sort of like how we do it now for anomaly detection lat_long and data visualizer). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments, but your changes look good to me. I am not tooo familiar with the transform framework and touch-points, but the coverage you have seems reasonable to me. I'm approving because I think that you can address my comments how you choose and I do not think there needs to be another round
import java.util.Map; | ||
|
||
public interface GeoShapeMetricAggregation { | ||
Map<String, Object> geoJSON(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the intention of this method is to return the underlying GeoJSON Geometry
object, and the InternalGeoLine
's XContent object is the wrapping Feature
object, I think it would make more sense to rename this to geoJSONGeometry
.
Also, I think adding some javadoc here to explain its purpose would help a passerby in the future.
.field("type", "LineString") | ||
.array("coordinates", coordinates.toArray()) | ||
.endObject() | ||
.field("geometry", geoJSON()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…-add-geo_line-agg-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice,
as for documentation, we have a list of supported aggs by transforms here:
docs/reference/rest-api/common-parms.asciidoc
That's my only ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…lastic#69299) This commit adds support for the Gold+ licensed `geo_line` aggregation. This aggregation takes a collection of `geo_point` values and constructs a line according to some sort value. Adding to transforms allows users to create these potentially expensive lines out of band of visualizations and then do additional aggs/queries against the pivoted data. Examples would be: "Do these daily user paths ever intersect?" "Does this path enter and leave this area?"
…69299) (#69483) This commit adds support for the Gold+ licensed `geo_line` aggregation. This aggregation takes a collection of `geo_point` values and constructs a line according to some sort value. Adding to transforms allows users to create these potentially expensive lines out of band of visualizations and then do additional aggs/queries against the pivoted data. Examples would be: "Do these daily user paths ever intersect?" "Does this path enter and leave this area?"
This commit adds support for the Gold+ licensed
geo_line
aggregation.This aggregation takes a collection of
geo_point
values and constructs a lineaccording to some sort value. Adding to transforms allows users to create these
potentially expensive lines out of band of visualizations and then do additional aggs/queries
against the pivoted data.
Examples would be:
"Do these daily user paths ever intersect?"
"Does this path enter and leave this area?"
Plus, it looks really cool in kibana maps