Centroid aggregation for cartesian points and shapes by craigtaverner · Pull Request #89216 · elastic/elasticsearch

craigtaverner · 2022-08-09T15:40:25Z

Based on the groundwork to Refactor GeoPoint and GeoShape with generics this PR provides support for centroid aggregations over Cartesian point and shape fields.

Closes #90156

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

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

iverase · 2022-09-15T07:47:38Z

.../elasticsearch/xpack/spatial/search/aggregations/support/CartesianShapeValuesSourceType.java

I would really like that CartesianShapeValuesSource returns a CartesianShapeValues. There are methods in ShapeValues that are geo specific and should only be present in GeoShapeValues. There is something that feels not right at the moment.

The previous PR was supposed to make sure that ShapeValues had no geo-specific code, but I can already see that I left at least CoordinateEncoder.Geo there, so I'll review this and fix it. I suspect there will be more cases where the previous PR did not quite complete the generalization of the code.

I notice also the use of LatLonGeometry and LatLonGeometryRelationVisitor. However, despite their names, those two classes are not geo-specific and look like they work perfectly well for cartesian data too. Perhaps they should be renamed?

The biggest problem is not in ShapeValues but ShapeValue. For example the method # relate(LatLonGeometry latLonGeometry) is for geo.

I think GeoshapeValue and Cartesianshapevalue are meant to support different things and should not be represented with the same ShapeValue.

But that is what I meant above. The method relate(LatLonGeometry) does not have anything geo-specific in it. All that code, and the code it calls is specific to the Lucene Component2D mechanism and is identical between geo and cartesian. The only mistake here is that the classes are incorrectly named LatLon..., while they have nothing to do with geo, and are used for both geo and cartesian data. They all work directly on the Geometry and Component2D interfaces, both of which are used for both geo and cartesian data.

So I think I can fix this by renaming the classes prefixed with LatLon something else. Then it will be more clear.

I have still not fixed the incorrectly named LatLon* classes that have nothing to do with Geo, and are used by both geo and cartesian. That could be done anytime. It is tech debt that precedes this work.

I am getting confused, I think you are not working on top of the latest main, still exposing ShapeValues on geo / cartesian classes

Yes, indeed. But I've fixed that now. Reverted the commit that moved back to GeoShapeValues.

That commit was based on the assumption that ShapeValues has geo-specific code in it. And that is still not true. I remember when I approved that before I said that I wanted to re-evaluate my assumptions and reconsider that work once we did the Cartesian PR. Now this is the Cartesian PR and it looks to me like the generalization is in fact a very good idea. We make a whole stack of code common between Cartesian and Geo. And this is all enabled by the Lucene Component2D interface which is what actually makes this stack common.

I think you assumptions are wrong, please don't expose ShapeValues as being the same for geo and cartesian, they are not.

iverase · 2022-09-15T15:19:54Z

.../spatial/src/main/java/org/elasticsearch/xpack/spatial/script/CartesianPointFieldScript.java

I see this is not used anywhere, maybe we should remove it until we decide to have runtime support

Agreed. I also remember we discussed with the painless team the option of building a completely different fields API for script for this, so none of the prototype code here is of value. I created an issue to discuss that at #90182

Fixed in 3fa5603

iverase · 2022-09-15T15:20:54Z

server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java

This come from an unused class, let's remove everything? we can revisit later.

Agreed. I also remember we discussed with the painless team the option of building a completely different fields API for script for this, so none of the prototype code here is of value. I created an issue to discuss that at #90182

Fixed in 3fa5603

iverase · 2022-09-21T11:44:01Z

server/src/main/java/org/elasticsearch/index/fielddata/GeoPointScriptDocValues.java

can we remove this?

I believe I did that in the same commit that removed the script classes.

iverase · 2022-09-21T11:51:41Z

...tial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/CartesianShapeValues.java

There is something that is not right here because ShapeValues.ShapeValue contains geo specific code. For example this means that CartesianShapeValue supports calling #relate(LatLonGeometry) which is not right. I think we ought to move the geo specific methods out of ShapeValue and only leave the generic ones?

There is no geo-specific code in ShapeValues or in ShapeValues.ShapeValue. I double checked and the only mistake is in class naming. For some reason classes are named LatLon* when they have no geo-specific behaviour. I commented on this above, and also pushed a new commit that further reduces the code duplication in ShapeValues.

If you can find any geo-specific method in ShapeValues or in ShapeValues.ShapeValue, please let me know, because I cannot find them. It was made generic in the previous PR, there is nothing geo-specific left.

elasticsearchmachine · 2022-09-22T10:07:09Z

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

iverase

Sorry, you just reverted the changes we did before. I don't want ShapeValue to be used indistinctly between geo and cartesian, they are different things.

iverase · 2022-09-22T10:54:57Z

...main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/ShapeValuesSource.java

I disagree here, please do not remove the generic. GeoShapeValuesSource should return GeoShapeValues.

elasticsearchmachine · 2022-09-22T11:59:06Z

Hi @craigtaverner, I've created a changelog YAML for you.

iverase · 2022-09-27T09:45:35Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/20_geo_centroid.yml

Maybe we can rename this file to 10_centroid so it can contain geo and cartesian implementations?

Renamed to 20_centroid.yml in 2710f73

iverase · 2022-09-27T09:45:36Z

x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/util/GeoTestUtils.java

Agree, this method sees weird here, I am ok to address it in a follow up.

Earlier work generalized GeoShapeValues so that all geo-specific code could be injected, and there was no need for objects of type GeoShapeValues. However, this advantage was not taken, and instances of GeoShapeValues, and now also CartesianShapeValues, were being created with different behaviors leading to a lot of duplicate code. This unnecessary duplication is now entirely removed for the three common cases: * Empty ShapeValues * ShapeValues using BinaryDocValues (in ShapeValueSourceType classes) * ShapeValues wrapping other ShapeValues and replacing missing values Bring back GeoShapeValues We found a way to bring back GeoShapeValues (and the new CartesianShapeValues) minimizing changes to the Geo-code while still achieving the removal of duplicated code.

However, this is largely a duplicate of geo-centroid docs since they are essentially identical behaviour. We should consider merging them.

This can be re-introduced in another PR

Work on isAggregatable caused a minor logic conflict. When that work was done, Point and Shape were not aggregatable, but now they are.

iverase

LGTM

craigtaverner added :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.5.0 labels Aug 9, 2022

craigtaverner force-pushed the more_cartesian2 branch from fb6a81e to 4f8269b Compare August 9, 2022 16:09

iverase reviewed Aug 10, 2022

View reviewed changes

.../java/org/elasticsearch/xpack/spatial/search/aggregations/support/ShapeValuesSourceType.java Outdated Show resolved Hide resolved

iverase reviewed Aug 10, 2022

View reviewed changes

...in/java/org/elasticsearch/xpack/spatial/search/aggregations/support/ShapeValuesProvider.java Outdated Show resolved Hide resolved

craigtaverner force-pushed the more_cartesian2 branch 6 times, most recently from 3fc7c78 to f0d1ec0 Compare August 17, 2022 14:18

craigtaverner force-pushed the more_cartesian2 branch 3 times, most recently from dd3dcd0 to d954cdd Compare September 7, 2022 09:58

iverase reviewed Sep 15, 2022

View reviewed changes

craigtaverner changed the title ~~More cartesian point and shape support, especially aggs (take 2)~~ Centroid aggregation for cartesian points and shapes Sep 20, 2022

This was referenced Sep 20, 2022

Cartesian support for aggregations #87804

Open

Centroid aggregation for Cartesian points and shapes #90156

Closed

iverase reviewed Sep 21, 2022

View reviewed changes

craigtaverner force-pushed the more_cartesian2 branch from 3fa5603 to dd17a50 Compare September 21, 2022 13:54

csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022

craigtaverner force-pushed the more_cartesian2 branch from 678c347 to 9c07b41 Compare September 22, 2022 10:02

craigtaverner marked this pull request as ready for review September 22, 2022 10:06

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

iverase requested changes Sep 22, 2022

View reviewed changes

craigtaverner added the >enhancement label Sep 22, 2022

craigtaverner force-pushed the more_cartesian2 branch from 06bb685 to 8496596 Compare September 27, 2022 09:01

craigtaverner mentioned this pull request Sep 27, 2022

Geo cartesian code deduplication #90387

Open

iverase reviewed Sep 27, 2022

View reviewed changes

craigtaverner force-pushed the more_cartesian2 branch 3 times, most recently from eb84fcd to a081ded Compare September 27, 2022 15:13

craigtaverner added 10 commits September 28, 2022 11:28

First draft of cartesian-centroid docs

9c99a5d

However, this is largely a duplicate of geo-centroid docs since they are essentially identical behaviour. We should consider merging them.

Renamed to match geo version of class

f0dda4a

Reverted code de-duplication for GeoShapeValues

c232cbd

This can be re-introduced in another PR

Fix failing tests after merge from main

81b094b

Work on isAggregatable caused a minor logic conflict. When that work was done, Point and Shape were not aggregatable, but now they are.

Fixed failing docs tests due to float precision

6407b24

Fixed some remaining geo_ code in cartesian_centroid docs

04011f8

Rename file since it contains both geo and cartesian

6a8bcf1

Fixed incorrect results in cartesian-centroid docs

4c096aa

Make cartesian test less sensitive to floating point errors

bc74445

craigtaverner force-pushed the more_cartesian2 branch from 1e6dc72 to bc74445 Compare September 28, 2022 09:29

iverase approved these changes Sep 28, 2022

View reviewed changes

craigtaverner merged commit 4c5d246 into elastic:main Sep 28, 2022

craigtaverner mentioned this pull request Sep 29, 2022

More cartesian point and shape support, especially aggs #88163

Closed

craigtaverner deleted the more_cartesian2 branch September 29, 2022 19:36

craigtaverner added the release highlight label Nov 21, 2022

craigtaverner mentioned this pull request Nov 21, 2022

8.6 release highlights cartesian #91746

Merged

Conversation

craigtaverner commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 22, 2022

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

craigtaverner commented Aug 9, 2022 •

edited

Loading

iverase Sep 15, 2022 •

edited

Loading