Support cartesian_bounds aggregation on point and shape #91298

craigtaverner · 2022-11-03T20:38:00Z

Fixes #90157

elasticsearchmachine · 2022-11-03T20:38:25Z

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

elasticsearchmachine · 2022-11-03T20:38:47Z

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

elasticsearchmachine · 2022-11-03T20:39:20Z

Hi @craigtaverner, I've updated the changelog YAML for you.

And added new CartesianBoundsIT and CartesianCentroidIT

* Removing the neg/pos left/right split used in bounds because this is only relevant to GeoBounds * Revert GeoBounds to no longer share common code with CartesianBounds (including aggregators) * Make Point and Shape aggregators share common code

iverase

I left some minor comments specially for the visibility of Shapes.ShapeValue.
Two things:

You have some weird file named jffi4926421121997545579.dylib which seems some left over.

~~2) We need to check if it is ok to change the API of GeoBounds, not sure if that is an. API we cannot change in a minor.~~
I check things again and everything is clear.

iverase · 2022-11-10T13:52:50Z

...lugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/ShapeValues.java

@@ -88,7 +90,7 @@ public T missing(String missing) {
     * thin wrapper around a {@link GeometryDocValueReader} which encodes / decodes values using
     * the provided decoder (could be geo or cartesian)
     */
-    protected abstract static class ShapeValue implements ToXContentFragment {
+    public abstract static class ShapeValue implements ToXContentFragment {


Can we make it protected again?

Done. My recent refactoring to remove the dateline logic from cartesian removed the need for this, so that was a good catch, thanks!

iverase · 2022-11-10T13:54:33Z

...asticsearch/xpack/spatial/search/aggregations/metrics/CartesianBoundsAggregationBuilder.java

+    @Override
+    public Version getMinimalSupportedVersion() {
+        // TODO: Should we have a version here? We only release this for 8.5 or 8.6
+        return Version.V_EMPTY;


Should be add 8.6 here?

I noticed that none of the other aggregations had values set here, even when recently added.

OK. I spoke too soon, while the first 4 I looked at did not specify anything, a more careful analysis shows that 4 of the 13 known Geo aggregations did have settings:

GeoHexGridAggregationBuilder -> Version.V_8_1_0

GeoLineAggregationBuilder -> Version.V_7_11_0

GeoGridQueryBuilder -> Version.V_8_3_0

GeoTileGridAggregationBuilder -> Version.V_7_0_0

So I will add Version.V_8_6_0 to the current one.

Also added it to the centroid aggregation, which was missing too!

iverase · 2022-11-10T13:57:06Z

.../elasticsearch/xpack/spatial/search/aggregations/metrics/CartesianShapeBoundsAggregator.java

+            public void collect(int doc, long bucket) throws IOException {
+                if (values.advanceExact(doc)) {
+                    maybeResize(bucket);
+                    ShapeValues.ShapeValue value = values.value();


Please let use here CartesianShapeValues.CartesianShapeValue instead?

iverase · 2022-11-10T14:18:57Z

.../src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java

@@ -43,15 +44,15 @@ public void testSingleValuedField() throws Exception {

        assertSearchResponse(response);

-        GeoBounds geoBounds = response.getAggregations().get(aggName);
+        SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);


Can be use here GeoBounds instead?

I was in the process of refactoring. I'll finish the job and then you can decide.

iverase · 2022-11-10T14:19:10Z

.../src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java

@@ -69,25 +70,28 @@ public void testSingleValuedField_getProperty() throws Exception {
        assertThat(global.getAggregations(), notNullValue());
        assertThat(global.getAggregations().asMap().size(), equalTo(1));

-        GeoBounds geobounds = global.getAggregations().get(aggName);
+        SpatialBounds<GeoPoint> geobounds = global.getAggregations().get(aggName);


The same here

I've done this for the two dateline tests, but all the rest moved into shared code.

iverase · 2022-11-10T14:19:20Z

.../src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java

@@ -98,15 +102,15 @@ public void testMultiValuedField() throws Exception {

        assertSearchResponse(response);

-        GeoBounds geoBounds = response.getAggregations().get(aggName);
+        SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);


And here, and every place where we know the concrete class

An earlier commit started generalizing Geo/Cartesian integration tests for centroid and bounds aggregations, but did not complete the job. This commit completes the work, since almost all tests are identical between cartesian and geo, except tests related to the dateline.

iverase · 2022-11-10T17:42:40Z

x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java

+                usage.track(
+                    SpatialStatsAction.Item.CARTESIANBOUNDS,
+                    CartesianBoundsAggregationBuilder.PARSER    // TODO: Should we have a license for this?
+                )


No, license for CartesianBounds should be the same as for GeoBounds, so it is basic.

OK. Removed the comment.

iverase

LGTM

* main: (163 commits) [DOCS] Edits frequent items aggregation (elastic#91564) Handle providers of optional services in ubermodule classloader (elastic#91217) Add `exportDockerImages` lifecycle task for exporting docker tarballs (elastic#91571) Fix CSV dependency report output file location in DRA CI job Fix variable placeholder for Strings.format calls (elastic#91531) Fix output dir creation in ConcatFileTask (elastic#91568) Fix declaration of dependencies in DRA snapshots CI job (elastic#91569) Upgrade Gradle Enterprise plugin to 3.11.4 (elastic#91435) Ingest DateProcessor (small) speedup, optimize collections code in DateFormatter.forPattern (elastic#91521) Fix inter project handling of generateDependenciesReport (elastic#91555) [Synthetics] Add synthetics-* read to fleet-server (elastic#91391) [ML] Copy more settings when creating DF analytics destination index (elastic#91546) Reduce CartesianCentroidIT flakiness (elastic#91553) Propagate last node to reinitialized routing tables (elastic#91549) Forecast write load during rollovers (elastic#91425) [DOCS] Warn about potential overhead of named queries (elastic#91512) Datastream unavailable exception metadata (elastic#91461) Generate docker images and dependency report in DRA ci job (elastic#91545) Support cartesian_bounds aggregation on point and shape (elastic#91298) Add support for EQL samples queries (elastic#91312) ... # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java

elasticsearchmachine · 2022-11-21T10:26:48Z

@craigtaverner according to this PR's labels, I need to update the changelog YAML, but I can't because the PR is closed. Please either update the changelog yourself on the appropriate branch, or adjust the labels. Specifically:

The PR is labelled release highlight but the changelog has no highlight section

Support cartesian_bounds aggregation on point and shape

2af3c58

craigtaverner added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Nov 3, 2022

craigtaverner requested a review from iverase November 3, 2022 20:38

elasticsearchmachine added the v8.6.0 label Nov 3, 2022

Update docs/changelog/91298.yaml

21d7a72

craigtaverner added 7 commits November 3, 2022 21:39

Update docs/changelog/91298.yaml

50ddaed

Fixed cartesian parse exceptions to mimic previous behavior

f94ee9c

Re-added removed interfaces to fix failing tests

28e917c

Generalized AbstractGeoTestCase for use in Cartesian tests

e4317ae

And added new CartesianBoundsIT and CartesianCentroidIT

Fixed bug in encoding missing cartesian points

794cb53

Simplify cartesian bounds

d40ba6e

* Removing the neg/pos left/right split used in bounds because this is only relevant to GeoBounds * Revert GeoBounds to no longer share common code with CartesianBounds (including aggregators) * Make Point and Shape aggregators share common code

Fixed geo comment

596ee0d

iverase requested changes Nov 10, 2022

View reviewed changes

iverase reviewed Nov 10, 2022

View reviewed changes

craigtaverner added 4 commits November 10, 2022 15:39

Delete incorrectly added file

5b2f9e5

Set correct minimum version for new cartesian aggs

4194cf0

Fixed specific types for ShapeValues

84cc481

iverase reviewed Nov 10, 2022

View reviewed changes

Removed licensing comment

ee930e6

craigtaverner force-pushed the cartesian_aggs_boundingbox branch from 51b1b06 to ee930e6 Compare November 10, 2022 18:10

craigtaverner added 2 commits November 14, 2022 11:11

GeoBoundsIT expects wrapLongitude(false)

2b502c1

Fixed CartesianBoundsIT flakiness and removed geo-specific code

c5ff54e

Reduced CartesianCentroidIT flakiness by normalizing to floats

f0c9ea2

iverase approved these changes Nov 14, 2022

View reviewed changes

craigtaverner merged commit 34e8da6 into elastic:main Nov 14, 2022

exalate-issue-sync bot mentioned this pull request Nov 15, 2022

Cartesian support for aggregations #87804

Open

craigtaverner mentioned this pull request Nov 16, 2022

Added documentation for cartesian-bounds aggregation #91623

Merged

craigtaverner added the release highlight label Nov 21, 2022

craigtaverner mentioned this pull request Nov 21, 2022

8.6 release highlights cartesian #91746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cartesian_bounds aggregation on point and shape #91298

Support cartesian_bounds aggregation on point and shape #91298

craigtaverner commented Nov 3, 2022 •

edited

Loading

elasticsearchmachine commented Nov 3, 2022

elasticsearchmachine commented Nov 3, 2022

elasticsearchmachine commented Nov 3, 2022

iverase left a comment •

edited

Loading

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

craigtaverner Nov 10, 2022

craigtaverner Nov 10, 2022

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

iverase Nov 10, 2022 •

edited

Loading

iverase Nov 10, 2022

craigtaverner Nov 10, 2022

iverase left a comment

elasticsearchmachine commented Nov 21, 2022

Support cartesian_bounds aggregation on point and shape #91298

Support cartesian_bounds aggregation on point and shape #91298

Conversation

craigtaverner commented Nov 3, 2022 • edited Loading

elasticsearchmachine commented Nov 3, 2022

elasticsearchmachine commented Nov 3, 2022

elasticsearchmachine commented Nov 3, 2022

iverase 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

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

iverase Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Nov 21, 2022

craigtaverner commented Nov 3, 2022 •

edited

Loading

iverase left a comment •

edited

Loading

iverase Nov 10, 2022 •

edited

Loading