Refactor GeoPoint and GeoShape with generics #89388

craigtaverner · 2022-08-16T14:51:01Z

In preparation for supporting CartesianPoint and CartesianShape
in aggregations, this PR adds a common interface between GeoPoint
and CartesianPoint, and then uses that to split out some key common
code that will be used in CartesianPoint and CartesianShape aggregations

elasticsearchmachine · 2022-08-16T14:51:25Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

iverase

I did a first pass on the points part and there is one thing I think we can improve. We are currently mixing geo classes with generic classes which makes the code confusing.

For example AbstractLeafGeoPointFieldData is using internally MultiPointValues and I think it should be using MultiGeoPointValues instead. I think the trick is not to use the generics extending ElasticPoint but using the generic extending MultiPointValues<?>. like:

public abstract class LeafPointFieldData<T extends MultiPointValues<?>> implements LeafFieldData {

    /**
     * Return geo-point or point values.
     */
    public abstract T getPointValues();

    /**
     * Return the internal representation of geo_point or point doc values as a {@link SortedNumericDocValues}.
     * A point is encoded as a long that can be decoded by using
     * {@link org.elasticsearch.common.geo.GeoPoint#resetFromEncoded(long)}
     */
    public abstract SortedNumericDocValues getSortedNumericDocValues();

}

I think this will simplify quite a bit things and it will prevent some unnecessary castings.
This applies to other classes like IndexPointFieldData and AbstractPointIndexFieldData.

iverase · 2022-08-17T13:27:14Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java

@@ -744,9 +744,9 @@ public final MultiGeoPointValues geoPointValues(LeafReaderContext context) {

        public static class Fielddata extends GeoPoint {

-            protected final IndexGeoPointFieldData indexFieldData;
+            protected final IndexPointFieldData<org.elasticsearch.common.geo.GeoPoint> indexFieldData;


Cannot we use here IndexGeoPointFieldData instead?

I believe this is resolved by your commit at 26b11a6

iverase · 2022-08-17T13:32:11Z

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

+ * To facilitate maximizing the use of common code between GeoPoint and projected CRS
+ * we introduced this ElasticPoint as an interface of commonality.
+ */
+public interface ElasticPoint {


I think we should rename this interface to represent better what it does. Maybe Point2d or BasePoint or SpatialPoint?

++

There's nothing elastic-specific about this interface, as far as I can see, so it should have a broader name.

OK. Let's go for SpatialPoint. I think BasePoint sounds more like an abstract class than an interface, and Point2d sounds too much like the 2d classes in Lucene, which this is not related to at all.

iverase · 2022-08-17T13:34:31Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/stats/EnumCounters.java

@@ -118,11 +118,9 @@ public String toString() {
        StringBuilder buf = new StringBuilder("[");
        boolean first = true;
        for (E e : enums) {
+            if (first == false) buf.append(", ");


This seems not to belong to this PR?

++. Stray automatic refactoring? I think the change is okay, but confusing that it's being done here.

Ha, ha! This was part of an earlier PR that Ignacio also pointed out was not relevant to that PR, so I pulled it into a separate, isolated PR at #88505. But that has yet to be approved. Perhaps you could review it, and this trivial change can stop popping up in my PRs? 😸

At least it is removed from this PR now.

iverase · 2022-08-17T13:39:48Z

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

-        GeoPoint bottomRight = new GeoPoint(bottom, right);
-        return new GeoBoundingBox(topLeft, bottomRight);
+        GeoBoundsParser bounds = new GeoBoundsParser(parser);
+        return (GeoBoundingBox) bounds.parseBoundingBox();


Is this casting not avoidable?

For example making the parser like: protected abstract static class BoundsParser<T extends BoundingBox<?>>?

OK. Done in c8b9ee3

iverase · 2022-08-17T13:44:52Z

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

@@ -53,7 +53,7 @@ public final boolean isFloatingPoint() {
    @Override
    public final SortedNumericDocValues longValues(LeafReaderContext ctx) {
        final MultiGeoPointValues multiGeoPointValues = valuesSource.geoPointValues(ctx);
-        final GeoPointValues values = org.elasticsearch.index.fielddata.FieldData.unwrapSingleton(multiGeoPointValues);
+        final GeoPointValues values = (GeoPointValues) org.elasticsearch.index.fielddata.FieldData.unwrapSingleton(multiGeoPointValues);


Is this casting necessary?

I believe this is resolved by your commit at 26b11a6

iverase · 2022-08-17T13:45:18Z

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

        final GeoPoint... fromPoints
    ) {
-        final GeoPointValues singleValues = FieldData.unwrapSingleton(geoPointValues);
+        final GeoPointValues singleValues = (GeoPointValues) FieldData.unwrapSingleton(geoPointValues);


Is this casting necessary?

I believe this is resolved by your commit at 26b11a6

iverase · 2022-08-17T13:53:39Z

server/src/main/java/org/elasticsearch/index/fielddata/plain/AbstractLeafGeoPointFieldData.java


-    public AbstractLeafGeoPointFieldData(ToScriptFieldFactory<MultiGeoPointValues> toScriptFieldFactory) {
+    public AbstractLeafGeoPointFieldData(ToScriptFieldFactory<MultiPointValues<GeoPoint>> toScriptFieldFactory) {


I would prefer if at this level we were using MultiGeoPointValues instead?

I believe this is resolved by your commit at 26b11a6

iverase · 2022-08-17T13:54:04Z

server/src/main/java/org/elasticsearch/index/fielddata/plain/LatLonPointDVLeafFieldData.java

@@ -22,7 +23,7 @@ final class LatLonPointDVLeafFieldData extends AbstractLeafGeoPointFieldData {
    private final LeafReader reader;
    private final String fieldName;

-    LatLonPointDVLeafFieldData(LeafReader reader, String fieldName, ToScriptFieldFactory<MultiGeoPointValues> toScriptFieldFactory) {
+    LatLonPointDVLeafFieldData(LeafReader reader, String fieldName, ToScriptFieldFactory<MultiPointValues<GeoPoint>> toScriptFieldFactory) {


I would prefer if at this level we were using MultiGeoPointValues instead?

I believe this is resolved by your commit at 26b11a6

iverase · 2022-08-17T13:54:12Z

server/src/main/java/org/elasticsearch/index/fielddata/plain/LatLonPointIndexFieldData.java

+        ValuesSourceType valuesSourceType,
+        ToScriptFieldFactory<MultiPointValues<GeoPoint>> toScriptFieldFactory
+    ) {
+        super(fieldName, valuesSourceType, toScriptFieldFactory);


I would prefer if at this level we were using MultiGeoPointValues instead?

I believe this is resolved by your commit at 26b11a6

not-napoleon

This looks mostly okay to me, but the Writable ValuesSourceType is extremely concerning. If it needs to be there, let's talk, and if it doesn't need to be there, I'll approve once it's dropped.

not-napoleon · 2022-08-17T17:18:17Z

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

+    static final ParseField LEFT_FIELD = new ParseField("left");
+    static final ParseField RIGHT_FIELD = new ParseField("right");
+    static final ParseField WKT_FIELD = new ParseField("wkt");
+    public static final ParseField BOUNDS_FIELD = new ParseField("bounds");


Maybe it's obvious to folks who work with this code a lot, but I'm unclear why some of these are public and others aren't. I'd appreciate a comment. (I see that it was like this in GeoBoundingBox where you relocated this from, so it's possible the reasoning is lost to the mists of time.)

When I look at the usages of these, it seems the public ones are used by classes outside this package (production code as well as test code). In particular ParsedGeoBounds uses these for parsing bounds used in aggregations.

So I think the root cause is that there are multiple queries that have a partially shared concept of geo bounds. Ideally it would be nice to imagine them all having the same, single, implementation. But that could be a behaviour change, which I think is certainly out of scope for this PR.

Yeah, I don't think we need to change it. I just want a comment saying it's on purpose and why.

not-napoleon · 2022-08-23T14:53:40Z

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

+ * To facilitate maximizing the use of common code between GeoPoint and projected CRS
+ * we introduced this ElasticPoint as an interface of commonality.
+ */
+public interface ElasticPoint {


++

There's nothing elastic-specific about this interface, as far as I can see, so it should have a broader name.

not-napoleon · 2022-08-23T19:08:33Z

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

-            builder.field(LAT_FIELD.getPreferredName(), topLeft.lat());
-            builder.field(LON_FIELD.getPreferredName(), topLeft.lon());
+            builder.field(LAT_FIELD.getPreferredName(), topLeft.getY());
+            builder.field(LON_FIELD.getPreferredName(), topLeft.getX());


Is there any reason to think that someone in the future might get confused if longitude maps to X or Y? Should we have some static function that wraps this logic? Right now, we replicate this knowledge in a few spots (e.g. the GeoPoint constructor below), so maybe there's some sense in putting that mapping in one place?

I feel like it's probably obvious to folks who work with this stuff, but I don't trust that things which are obvious today will be obvious in the future.

I agree 100%. I want to move in a direction that isolates this kind of mapping to a single place. Right now there are many, many places that depend on secret knowledge of the mapping and the correct order (for example some methods take (double y, double x) and others take (double x, double y). In the earlier Elastic code (and earlier Lucene code), y, x was the norm, but over time things have swung over to x, y being much more common. What I'm keen to do is iteratively minimize the differences. Some earlier PR's I did in the Spring already downplayed the y, x cases (in code and documentation), but that was a small step. This PR here now actually does improve things quite a bit (IMHO); but it also brings some things into the light, like the one you observe here. I think more can (and should) be done, but I do not want to change too much in one go. So this PR priorizes the changes required to enable the work on Cartesian, but does not do everything that could be done. This feels like a safer compromise.

And the creation of the SpatialPoint (neé ElasticPoint) interface leads to one option for where we could place this kind of knowledge. But I think we should think more about that before actually taking that step. There could be better options.

Sounds good. I'm happy to keep this on the roadmap and address it in a future PR after more thought.

not-napoleon · 2022-08-24T13:52:25Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroid.java

-
-    long count();
-}
+public interface GeoCentroid extends CentroidAggregation {}


Is this interface still necessary?

This was because I wrote the CartesianCentroid stuff, and then Ignacio wanted the pure refactoring parts pulled out. The Cartesian work is in a separate PR on top of this work. If I move this change into that PR it will cause that PR to contain a lot of test code changes that are geo-specific (not cartesian). My goal is for this PR to contain only refactoring (moving Geo-specific code around) and the other PR to contain primarily Cartesian specific stuff (minimize Geo changes). So for the purpose of that split, this is a good change :-)

not-napoleon · 2022-08-24T13:58:30Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCentroid.java

+
+    protected abstract ElasticPoint centroidFromStream(StreamInput in) throws IOException;
+
+    protected abstract void centroidToStream(StreamOutput out) throws IOException;


I'd like for abstract functions to have some java doc explaining what implementations need to do. In this particular case, I'm unclear what these are for since we already have a stream constructor and a doWriteTo method.

These methods encapsulate the parts of the Geo vs. Cartesian that differ. In particular this is the y, x versus x, y we discussed in another comment. This move is a step towards trying to get that particular issue isolated. I think more can be done (eg. moving that knowledge into the SpatialPoint or some other class), but this step feels like a step in the right direction.

Yeah, I think they're fine, I just want them to have javadoc.

not-napoleon · 2022-08-29T16:07:51Z

.../java/org/elasticsearch/xpack/spatial/index/fielddata/plain/AbstractShapeIndexFieldData.java

+import org.elasticsearch.xpack.spatial.index.fielddata.IndexShapeFieldData;
+import org.elasticsearch.xpack.spatial.index.fielddata.ShapeValues;
+
+public abstract class AbstractShapeIndexFieldData implements IndexShapeFieldData {


Javadoc please, ideally with some hints as to why and how one would build a concrete implementation of this.

OK. Added some.

not-napoleon · 2022-08-29T16:11:29Z

.../java/org/elasticsearch/xpack/spatial/index/fielddata/plain/AbstractShapeIndexFieldData.java

+        XFieldComparatorSource.Nested nested,
+        boolean reverse
+    ) {
+        throw new IllegalArgumentException("can't sort on geo_shape field without using specific sorting feature, like geo_distance");


is it correct to specify geo_shape and geo_distance here? might this abstraction have a non-geo specialization down the road? I would prefer if it got the field name from the concrete instance, and didn't offer a suggestion for the sorting feature.

I've moved the exception into the extending class, where the specific names have meaning.

not-napoleon · 2022-08-29T16:14:25Z

...in/java/org/elasticsearch/xpack/spatial/index/fielddata/plain/LatLonShapeIndexFieldData.java

+        int bucketSize,
+        BucketedSort.ExtraData extra
+    ) {
+        throw new IllegalArgumentException("can't sort on geo_shape field without using specific sorting feature, like geo_distance");


As above, I'd prefer not to hard code the type names in the error message. Also, with the repeated error message here, consider centralizing it to AggregationErrors - I'm trying to make that a thing.

I've done a minimal change, but not yet made use of AggregationErrors. I'll read up on that and learn about it.

not-napoleon · 2022-08-29T16:18:43Z

...in/java/org/elasticsearch/xpack/spatial/search/aggregations/support/ShapeValuesProvider.java

+
+import java.io.IOException;
+
+public abstract class ShapeValuesProvider extends ValuesSource {


Why are we calling this a ValuesProvider instead of a ValuesSource? I really don't want to add another distinction of terms to an already confusing part of the code unless we're very clear what the new terminology means.

No idea. Seems wrong to me too. I've fixed it in 33cb378.

not-napoleon · 2022-08-29T16:21:23Z

.../java/org/elasticsearch/xpack/spatial/search/aggregations/support/ShapeValuesSourceType.java

+
+import java.io.IOException;
+
+public abstract class ShapeValuesSourceType implements Writeable, ValuesSourceType {


Why does this need to be Writeable? ValuesSourceType implementations are intended to be stateless, which means they should never need to be written. It's actually quite concerning to me if we need a writeable values source type.

I traced this back and it was introduced by Tal Levy in 2020 as:

public class GeoShapeValuesSourceType implements Writeable, ValuesSourceType

I do not know why it was writable. My vote is we investigate that separate from this PR because my goal with this PR was to leave behaviour as unchanged as possible, just moving around code to enable the Cartesian work.

I notice that the writeTo method is empty and it would be interesting to simply remove it and see what PR building does. My default thinking is to save that for another PR, but if you have a strong opinion here, we can do it here directly.

So, I thought a CI test of this would be a good idea. I pushed that in 541e47f. Let's see how it goes and then make a decision.

In preparation for supporting CartesianPoint and CartesianShape in aggregations, this PR adds a common interface between GeoPoint and CartesianPoint, and then uses that to split out some key common code that will be used in CartesianPoint and CartesianShape aggregations

Co-authored-by: Ignacio Vera <[email protected]>

It extends ValuesSource, and is extended by GeoShapeValuesSource. There is no reason for the suffix `Provider`.

craigtaverner · 2022-09-05T16:01:48Z

.../java/org/elasticsearch/xpack/spatial/index/fielddata/plain/AbstractShapeIndexFieldData.java

+        throw new IllegalArgumentException("can't sort on geo_shape field without using specific sorting feature, like geo_distance");
+    }
+
+    public static class GeoBuilder implements IndexFieldData.Builder {


This builder is not needed, we can just use a lambda.

Based on Ignacio's work in 050df95, we fix the BoundingBox generics, and also add a little more specificity to the previous generics (replace <?> with <? extends SpatialPoint>).

craigtaverner · 2022-09-06T13:05:01Z

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

-        builder.endObject();
-        return builder;
-    }
-
    public XContentBuilder toXContentFragment(XContentBuilder builder, boolean buildLatLonFields) throws IOException {


The value of buildLatLonFields is true almost always, and is also something geo-specific. We should get rid of it and have another method for the false case that only exists in the geo-implementation.

craigtaverner · 2022-09-06T13:05:54Z

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

+        return builder;
+    }
+
+    public abstract XContentBuilder toXContentFragment(XContentBuilder builder, boolean buildLatLonFields) throws IOException;


buildLatLonField sounds very geo-specific. We should be able to get rid of this for now.

craigtaverner · 2022-09-06T13:10:24Z

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

+    static final ParseField RIGHT_FIELD = new ParseField("right");
+    static final ParseField WKT_FIELD = new ParseField("wkt");
+    public static final ParseField BOUNDS_FIELD = new ParseField("bounds");
+    public static final ParseField LAT_FIELD = new ParseField("lat");


LAT and LON are geo-specific and should be in GeoBoundingBox

not-napoleon

This looks good. As noted, there's follow up work, but this looks like a big improvement to me, so let's get it in. Thanks for taking this on!

craigtaverner added :Analytics/Geo Indexing, search aggregations of geo points and shapes :Analytics/Aggregations Aggregations >refactoring >tech debt labels Aug 16, 2022

craigtaverner requested a review from iverase August 16, 2022 14:51

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 16, 2022

elasticsearchmachine added the v8.5.0 label Aug 16, 2022

craigtaverner force-pushed the elastic_point_generics branch 3 times, most recently from 7d30449 to 38e4dcb Compare August 17, 2022 14:03

iverase reviewed Aug 17, 2022

View reviewed changes

craigtaverner requested a review from not-napoleon August 17, 2022 15:19

not-napoleon requested changes Aug 29, 2022

View reviewed changes

craigtaverner and others added 3 commits September 5, 2022 17:11

Simplify generics (by Ignacio)

050df95

Co-authored-by: Ignacio Vera <[email protected]>

Refactor ElasticPoint to SpatialPoint

2e592b5

craigtaverner force-pushed the elastic_point_generics branch from 26b11a6 to 2e592b5 Compare September 5, 2022 15:12

Rename ShapeValuesProvider to ShapeValuesSource

33cb378

It extends ValuesSource, and is extended by GeoShapeValuesSource. There is no reason for the suffix `Provider`.

craigtaverner commented Sep 5, 2022

View reviewed changes

craigtaverner added 4 commits September 5, 2022 18:23

Code review, mostly AbstractShapeIndexFieldData

d0ed852

Reverted trivial refactoring

31629e9

Removed unused Writable interface implementation

541e47f

Further generics refinements

c8b9ee3

Based on Ignacio's work in 050df95, we fix the BoundingBox generics, and also add a little more specificity to the previous generics (replace <?> with <? extends SpatialPoint>).

craigtaverner commented Sep 6, 2022

View reviewed changes

Removed some geo-specific code from BoundingBox

1032ffc

not-napoleon approved these changes Sep 6, 2022

View reviewed changes

craigtaverner merged commit b68d62b into elastic:main Sep 7, 2022

craigtaverner mentioned this pull request Sep 7, 2022

Centroid aggregation for cartesian points and shapes #89216

Merged

iverase mentioned this pull request Sep 15, 2022

Remove ShapeValues from Geo specific classes in favour of GeoShapeValues #90100

Merged

This was referenced Sep 20, 2022

Cartesian support for aggregations #87804

Open

Centroid aggregation for Cartesian points and shapes #90156

Closed

Painless script support for Cartesian Points #90182

Open

craigtaverner deleted the elastic_point_generics branch October 20, 2023 11:44


		public AbstractLeafGeoPointFieldData(ToScriptFieldFactory<MultiGeoPointValues> toScriptFieldFactory) {
		public AbstractLeafGeoPointFieldData(ToScriptFieldFactory<MultiPointValues<GeoPoint>> toScriptFieldFactory) {


		protected abstract ElasticPoint centroidFromStream(StreamInput in) throws IOException;

		protected abstract void centroidToStream(StreamOutput out) throws IOException;


		import java.io.IOException;

		public abstract class ShapeValuesProvider extends ValuesSource {


		import java.io.IOException;

		public abstract class ShapeValuesSourceType implements Writeable, ValuesSourceType {

Refactor GeoPoint and GeoShape with generics #89388

Refactor GeoPoint and GeoShape with generics #89388

Conversation

craigtaverner commented Aug 16, 2022

elasticsearchmachine commented Aug 16, 2022

iverase 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

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

not-napoleon 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

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

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

not-napoleon left a comment

Choose a reason for hiding this comment