disallow creating geo_shape mappings with deprecated parameters by iverase · Pull Request #70850 · elastic/elasticsearch

iverase · 2021-03-25T08:19:25Z

With the introduction of BKD-based geo shape indexing in #32039, the prefix tree indexing method has been deprecated. From 8.0.0, it will not be allowed to create new mappings using deprecated parameters, and therefore all new mappings will be using the BKD indexing strategy.

elasticmachine · 2021-03-25T08:19:28Z

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

romseygeek

Thanks @iverase! I left a suggestion about how to improve the error message.

server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldTypeTests.java

romseygeek · 2021-03-25T09:04:24Z

server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java

            multiFields, copyTo, indexer, parser);
+        if (builder.indexCreatedVersion.onOrAfter(Version.V_8_0_0)) {
+            throw new IllegalArgumentException("mapper [" + name()
+                + "] of type [geo_shape] with deprecated parameters is no longer allowed");


It would be good to be a bit clearer about what is causing the error here. Maybe instead of having the version check here we should move it to containsDeprecatedParameter so that the error message can explicitly contain the parameters that were used and are no longer allowed?

What I have done is to collect the used deprecated parameters in the builder so we can report back to the user. wdyt?

romseygeek

Much better, thanks! I made one more suggestion but looks good otherwise.

romseygeek · 2021-03-25T10:58:43Z

server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java

        super(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), Lucene.KEYWORD_ANALYZER),
            builder.ignoreMalformed.get(), builder.coerce.get(), builder.ignoreZValue.get(), builder.orientation.get(),
            multiFields, copyTo, indexer, parser);
+        if (builder.indexCreatedVersion.onOrAfter(Version.V_8_0_0)) {


Can we move this into the Builder constructor? The earlier we throw the better, in general.

I have tried but I have issues with this test:

elasticsearch/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

Line 270 in 91c700b

public final void testDeprecatedBoost() throws IOException {

It seems that checking of unknown parameters happen after the builder.

…#70850) Searchable snapshots classes have been spread over multiple packages since the creation of the plugin's project (mea culpa). With the addition of the shared cache and other factorization of IndexInput's code it becomes less obvious to navigate the plugin codebase. Backport of #70814 for 7.12

iverase · 2021-04-29T10:49:45Z

@romseygeek, I have changed the approach and I throw an error as soon as possible (on the GeoShapeFieldMapper's type parser).

I run into issue with two test on MapperTestCase which assume you can create those mappings in version 8.0.0:

testDeprecatedBoost: Do you have any suggestion here, I am not sure how to tackle it? Maybe we can introduce a method in MapperTestCase like supportsVersion(version) to check if we can run the test?

testIndexTimeStoredFieldsAccess: I add a check that we are creating a mapping service with the correct version.

romseygeek · 2021-04-29T11:13:59Z

server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapperTests.java

+    protected MapperService createMapperService(XContentBuilder mappings) throws IOException {
+        Version version = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0);
+        return createMapperService(version, mappings);
+    }


How about we also override createMapperService(Version, XContentBuilder) and do an assumeFalse that the version is less than Version.V_8_0_0? It's final in the base class at the moment but I think it's fine to make it overrideable.

That works too. Implemented.

…rsion

romseygeek

A couple of nits but we're nearly there I think.

server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapperTests.java

romseygeek · 2021-04-29T12:04:50Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

                minimalMapping(b);
                b.field("store", true);
            }));
+            assumeTrue("Mapper service is too old",


Do we still need this check in the base class?

Yes, we call the implementor for a service mapper and it can be returned theoretically in any version so we need to check, On the other hand we can force the implementor to return a specific version?

It goes through createFieldMapper(XContentBuilder) which is overridden in LegacyGeoShapeFieldMapperTests though, so we should already be picking a working version?

Yes but we are creating a working version with version < 8.0.0. For this test to work the version needs to be >= 8.0.0?

#72474 is merged so we should be able to remove this

romseygeek · 2021-04-29T12:06:57Z

...main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java

+                throw new IllegalArgumentException("using deprecated parameters " + Arrays.toString(deprecatedParams.toArray())
+                    + " in mapper [" + name + "] of type [geo_shape] is no longer allowed");
+            }
            builder = new LegacyGeoShapeFieldMapper.Builder(


Do we have a test for this?

For a followup: do we want to merge this class into GeoShape now that everything is basic licensed?

Yes, I added one

There are a couple of nits before merging them but yes, in the future we should merge them.

romseygeek

LGTM, thanks @iverase!

* Removes docs and references for the following `geo_shape` mapping parameters: * `tree` * `tree_levels` * `strategy` * `distance_error_pct` * Updates a related breaking change. Relates to #70850

The following properties have been removed from the geo_shape data type in 8.0: "strategy", "tree", "tree_levels", "precision", "distance_error_pct", "points_only". This PR adds a deprecation info API check for indexes and templates that have any of these properties in their mappings. Relates #42404 #70850

disallow creating geo_shape mappings with deprecated parameters

edfd032

iverase added >breaking :Analytics/Geo Indexing, search aggregations of geo points and shapes v8.0.0 labels Mar 25, 2021

iverase requested a review from romseygeek March 25, 2021 08:19

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 25, 2021

iverase mentioned this pull request Mar 25, 2021

BKD-based geo shapes #32039

Closed

22 tasks

romseygeek requested changes Mar 25, 2021

View reviewed changes

better error message

40ad9bc

romseygeek approved these changes Mar 25, 2021

View reviewed changes

iter

d707cf0

iverase added 6 commits April 29, 2021 09:30

Merge branch 'master' into LegacyGeoShapeFieldMapper

ab9063e

iter

82995d1

move check to n GeoShapeFieldMapper's type parser

4b4a5e3

typo

44edae5

iter

1b5ecf8

iter

ff7fd85

romseygeek reviewed Apr 29, 2021

View reviewed changes

make possible to override creation of mapper service with specific ve…

6d66d6a

…rsion

romseygeek reviewed Apr 29, 2021

View reviewed changes

iverase added 3 commits April 29, 2021 14:19

Add test for GeoShapeWithDocValues

e93344b

update comment

4f3a451

Merge branch 'master' into LegacyGeoShapeFieldMapper

75b827d

romseygeek approved these changes Apr 30, 2021

View reviewed changes

iverase merged commit 4fff378 into elastic:master Apr 30, 2021

iverase deleted the LegacyGeoShapeFieldMapper branch April 30, 2021 09:09

brianseeders mentioned this pull request Apr 30, 2021

[8.0] Failing ES Promotion: Various failures with mapper_parsing_exception elastic/kibana#98984

Closed

iverase mentioned this pull request May 3, 2021

[CI] GeoBoundingBoxQueryGeoShapeIT testLimit2BoundingBox failing #72602

Closed

spalger mentioned this pull request May 3, 2021

[ftr/esArchiver] disable geo_shape field of maps saved objects in archives elastic/kibana#99119

Merged

jrodewig mentioned this pull request Jun 23, 2021

[DOCS] Remove deprecated geo_shape parameters #74519

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

This was referenced Aug 13, 2021

Update Deprecation Info API checks for 8.0 #42404

Closed

Feature/geo shape parameter deprecation #76626

Closed

Adding deprecation checks for geo_shape parameters #76627

Merged

hkulekci mentioned this pull request Feb 17, 2022

Elasticsearch 8.0 support hkulekci/ElasticsearchDSL#2

Merged

23 tasks

iverase mentioned this pull request May 15, 2023

Remove legacy-geo module #96097

Open

Conversation

iverase commented Mar 25, 2021

Uh oh!

elasticmachine commented Mar 25, 2021

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

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 commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

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!

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!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iverase commented Apr 29, 2021 •

edited

Loading