From 077958eb2f39b9fe00994ae2846bf3ad3515f81b Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Mon, 9 Jun 2025 12:40:54 +0200 Subject: [PATCH] Throw better exception for unsupported aggregations over shape fields --- .../xpack/spatial/SpatialPlugin.java | 30 +---- .../UnsupportedAggregationsTests.java | 121 ++++++++++++++++++ 2 files changed, 122 insertions(+), 29 deletions(-) create mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/UnsupportedAggregationsTests.java diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java index b9d6d5fe20e08..c8de01e326f99 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialPlugin.java @@ -26,12 +26,8 @@ import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashGridAggregator; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileGridAggregator; -import org.elasticsearch.search.aggregations.metrics.CardinalityAggregationBuilder; -import org.elasticsearch.search.aggregations.metrics.CardinalityAggregator; import org.elasticsearch.search.aggregations.metrics.GeoBoundsAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.GeoCentroidAggregationBuilder; -import org.elasticsearch.search.aggregations.metrics.ValueCountAggregationBuilder; -import org.elasticsearch.search.aggregations.metrics.ValueCountAggregator; import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; @@ -162,9 +158,7 @@ public List> getAggregationExtentions() { return List.of( this::registerGeoShapeCentroidAggregator, this::registerGeoShapeGridAggregators, - SpatialPlugin::registerGeoShapeBoundsAggregator, - SpatialPlugin::registerValueCountAggregator, - SpatialPlugin::registerCardinalityAggregator + SpatialPlugin::registerGeoShapeBoundsAggregator ); } @@ -408,28 +402,6 @@ private void registerGeoShapeGridAggregators(ValuesSourceRegistry.Builder builde ); } - private static void registerValueCountAggregator(ValuesSourceRegistry.Builder builder) { - builder.register(ValueCountAggregationBuilder.REGISTRY_KEY, GeoShapeValuesSourceType.instance(), ValueCountAggregator::new, true); - } - - private static void registerCardinalityAggregator(ValuesSourceRegistry.Builder builder) { - builder.register( - CardinalityAggregationBuilder.REGISTRY_KEY, - GeoShapeValuesSourceType.instance(), - (name, valuesSourceConfig, precision, executionMode, context, parent, metadata) -> new CardinalityAggregator( - name, - valuesSourceConfig, - precision, - // Force execution mode to null - null, - context, - parent, - metadata - ), - true - ); - } - private ContextParser checkLicense(ContextParser realParser, LicensedFeature.Momentary feature) { return (parser, name) -> { if (feature.check(getLicenseState()) == false) { diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/UnsupportedAggregationsTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/UnsupportedAggregationsTests.java new file mode 100644 index 0000000000000..1e9a1b19a38d1 --- /dev/null +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/UnsupportedAggregationsTests.java @@ -0,0 +1,121 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.spatial.search.aggregations; + +import org.apache.lucene.document.Document; +import org.elasticsearch.common.geo.Orientation; +import org.elasticsearch.geometry.Point; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.lucene.spatial.BinaryShapeDocValuesField; +import org.elasticsearch.plugins.SearchPlugin; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.metrics.CardinalityAggregationBuilder; +import org.elasticsearch.search.aggregations.metrics.ValueCountAggregationBuilder; +import org.elasticsearch.xpack.spatial.LocalStateSpatialPlugin; +import org.elasticsearch.xpack.spatial.index.mapper.GeoShapeWithDocValuesFieldMapper; +import org.elasticsearch.xpack.spatial.index.mapper.ShapeFieldMapper; +import org.elasticsearch.xpack.spatial.util.GeoTestUtils; + +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; + +public class UnsupportedAggregationsTests extends AggregatorTestCase { + + @Override + protected List getSearchPlugins() { + return List.of(new LocalStateSpatialPlugin()); + } + + public void testCardinalityAggregationOnGeoShape() { + MappedFieldType fieldType = new GeoShapeWithDocValuesFieldMapper.GeoShapeWithDocValuesFieldType( + "geometry", + true, + true, + randomBoolean(), + Orientation.RIGHT, + null, + null, + null, + false, + Collections.emptyMap() + ); + BinaryShapeDocValuesField field = GeoTestUtils.binaryGeoShapeDocValuesField("geometry", new Point(0, 0)); + CardinalityAggregationBuilder builder = new CardinalityAggregationBuilder("cardinality").field("geometry"); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> testCase(iw -> { + Document doc = new Document(); + doc.add(field); + iw.addDocument(doc); + }, agg -> {}, new AggTestConfig(builder, fieldType))); + assertThat(exception.getMessage(), equalTo("Field [geometry] of type [geo_shape] is not supported for aggregation [cardinality]")); + } + + public void testCardinalityAggregationOnShape() { + MappedFieldType fieldType = new ShapeFieldMapper.ShapeFieldType( + "geometry", + true, + true, + Orientation.RIGHT, + null, + false, + Collections.emptyMap() + ); + BinaryShapeDocValuesField field = GeoTestUtils.binaryCartesianShapeDocValuesField("geometry", new Point(0, 0)); + CardinalityAggregationBuilder builder = new CardinalityAggregationBuilder("cardinality").field("geometry"); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> testCase(iw -> { + Document doc = new Document(); + doc.add(field); + iw.addDocument(doc); + }, agg -> {}, new AggTestConfig(builder, fieldType))); + assertThat(exception.getMessage(), equalTo("Field [geometry] of type [shape] is not supported for aggregation [cardinality]")); + } + + public void testValueCountAggregationOnGeoShape() { + MappedFieldType fieldType = new GeoShapeWithDocValuesFieldMapper.GeoShapeWithDocValuesFieldType( + "geometry", + true, + true, + randomBoolean(), + Orientation.RIGHT, + null, + null, + null, + false, + Collections.emptyMap() + ); + BinaryShapeDocValuesField field = GeoTestUtils.binaryGeoShapeDocValuesField("geometry", new Point(0, 0)); + ValueCountAggregationBuilder builder = new ValueCountAggregationBuilder("cardinality").field("geometry"); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> testCase(iw -> { + Document doc = new Document(); + doc.add(field); + iw.addDocument(doc); + }, agg -> {}, new AggTestConfig(builder, fieldType))); + assertThat(exception.getMessage(), equalTo("Field [geometry] of type [geo_shape] is not supported for aggregation [value_count]")); + } + + public void testValueCountAggregationOShape() { + MappedFieldType fieldType = new ShapeFieldMapper.ShapeFieldType( + "geometry", + true, + true, + Orientation.RIGHT, + null, + false, + Collections.emptyMap() + ); + BinaryShapeDocValuesField field = GeoTestUtils.binaryCartesianShapeDocValuesField("geometry", new Point(0, 0)); + ValueCountAggregationBuilder builder = new ValueCountAggregationBuilder("cardinality").field("geometry"); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> testCase(iw -> { + Document doc = new Document(); + doc.add(field); + iw.addDocument(doc); + }, agg -> {}, new AggTestConfig(builder, fieldType))); + assertThat(exception.getMessage(), equalTo("Field [geometry] of type [shape] is not supported for aggregation [value_count]")); + } +}