Convert geo field mappers to Parametrized form by romseygeek · Pull Request #63836 · elastic/elasticsearch

romseygeek · 2020-10-16T16:38:21Z

Relates to #62988

elasticmachine · 2020-10-16T16:38:24Z

Pinging @elastic/es-search (:Search/Mapping)

romseygeek · 2020-10-19T13:44:56Z

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

        }
-        return null;
+        throw new IllegalArgumentException("Unknown strategy [" + strategyName + "]");
+    }


This ended up catching a pre-existing bug in GeoShapeQueryTests, which was asking for TERM instead of term as a strategy, and therefore ending up falling back on null here, which was getting translated to recursive once the mapper had been parsed. Silently falling back to recursive is a bug, I think, although on backport we will need to change this to emit a warning instead, as you can't change the strategy once it has been set and so existing mappings with badly-spelled term strategies will have to stay using recursive

Do you think this is a breaking change?

I'd say it was a bugfix rather than a breaking change, I think?

I opened #63975 to emit a deprecation warning for this case in 7.x

Existing mappings sort of fix themselves, btw, so my above comment was not correct. Once a user has added a mapping with a bad strategy, it will serialize itself as recursive, so we don't need to worry about parsing mappings from previous versions.

Yes, that is what I checked so the only change is that creating a mapping will throw an error when before it was successful.

romseygeek · 2020-10-19T13:46:24Z

server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java

-            ? AbstractShapeGeometryFieldMapper.Defaults.IGNORE_Z_VALUE
-            : shapeMapper.ignoreZValue();
+        boolean coerce = shapeMapper != null && shapeMapper.coerce();
+        boolean ignoreZValue = shapeMapper == null || shapeMapper.ignoreZValue();


Whether or not a parameter has been explicitly set is of interest only to the mapper itself, and its serialization code; the PR changes these getters on the mappers to return simple booleans.

romseygeek · 2020-10-19T13:51:25Z

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

+        }
+    }
+
+    protected String[] getParseMinimalWarnings() {


These needed to be changed to getters, rather than just asserting in themselves, so that the checks for deprecated boost parameters didn't get confused by the additional warnings.

romseygeek · 2020-10-19T14:00:18Z

This is a big change, because unfortunately due to the class hierarchies I had to change all the geo mappers at the same time. On the plus side, these are the only remaining mappers to convert, so once this is done we can collapse FieldMapper and ParametrizedFieldMapper together and remove a lot of dead code.

romseygeek · 2020-10-19T15:12:20Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

nik9000

Looks right to me but I left a request and a question. I genuinely don't know the answer to the question.

nik9000 · 2020-10-19T17:08:39Z

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


+    @Override
+    protected void addMultiFields(ParseContext context, Geometry geometry) {
+


It looks like the old code had a comment that this was intentionally a noop. I think it is worth keeping that.

nik9000 · 2020-10-19T17:10:12Z

...gin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapper.java

                CartesianPoint o = (CartesianPoint)other;
                oX = o.getX();
                oY = o.getY();
-            } else if (other instanceof ParsedCartesianPoint == false) {


We don't need to be equal to ParsedCartesianPoints?

If we're comparing against a ParsedCartesianPoint, then the earlier instanceof CartesianPoint branch will already have matched, so this branch is never called.

++. I hadn't pulled it up.

romseygeek · 2020-10-21T08:56:59Z

Thanks @nik9000, I added the comments back in.

iverase

LGTM, It is a nice simplification.

romseygeek · 2020-10-21T14:03:56Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

romseygeek · 2020-10-21T14:19:50Z

@elasticmachine update branch

romseygeek · 2020-10-21T14:27:22Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

romseygeek · 2020-10-21T14:28:20Z

@elasticmachine run elasticsearch-ci/default-distro (gradle download timeout)

romseygeek · 2020-10-21T16:57:40Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

romseygeek · 2020-10-22T07:22:46Z

@elasticmachine update branch

Relates to elastic#62988

Relates to #62988

Convert geo field mappers to Parametrized form

9d48343

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.11.0 labels Oct 16, 2020

romseygeek self-assigned this Oct 16, 2020

elasticmachine added the Team:Search Meta label for search team label Oct 16, 2020

duh

6ef3e68

romseygeek mentioned this pull request Oct 16, 2020

Convert remaining FieldMappers to parametrized form #62988

Closed

28 tasks

romseygeek added 7 commits October 17, 2020 12:08

tests

61de96c

Merge remote-tracking branch 'origin/master' into mapper/geo-params

9fcd17d

points_only fixes

db49a69

geopoint takes arrays

393631d

tests

76feae0

legacy -> bkd merges

9e1307a

wut

b4d892f

romseygeek requested a review from iverase October 19, 2020 13:41

romseygeek commented Oct 19, 2020

View reviewed changes

romseygeek requested a review from nik9000 October 19, 2020 14:00

romseygeek added 2 commits October 19, 2020 15:42

remove unnecessary bit

ff0269d

Merge remote-tracking branch 'origin/master' into mapper/geo-params

c49b627

nik9000 requested changes Oct 19, 2020

View reviewed changes

romseygeek added 2 commits October 21, 2020 09:49

Merge remote-tracking branch 'origin/master' into mapper/geo-params

b53b33b

add comments back in

ec447e9

romseygeek mentioned this pull request Oct 21, 2020

Emit a warning when an unknown geo strategy is passed in #63975

Closed

iverase approved these changes Oct 21, 2020

View reviewed changes

nik9000 approved these changes Oct 21, 2020

View reviewed changes

elasticmachine and others added 2 commits October 21, 2020 10:19

Merge branch 'master' into mapper/geo-params

c62cbd8

Add to breaking changes

7bddf07

Merge branch 'master' into mapper/geo-params

d731542

romseygeek merged commit bfaf304 into elastic:master Oct 22, 2020

romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Oct 22, 2020

Convert geo field mappers to Parametrized form (elastic#63836)

7fa1af8

Relates to elastic#62988

romseygeek added a commit that referenced this pull request Oct 22, 2020

Convert geo field mappers to Parametrized form (#63836) (#64051)

0bffbf0

Relates to #62988

iverase mentioned this pull request Feb 23, 2021

With ES 7.10.2 unable to make geo_point field not-indexed (Regression from 7.5.2) #69401

Closed

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


		@Override
		protected void addMultiFields(ParseContext context, Geometry geometry) {

Conversation

romseygeek commented Oct 16, 2020

Uh oh!

elasticmachine commented Oct 16, 2020

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 commented Oct 19, 2020

Uh oh!

romseygeek commented Oct 19, 2020

Uh oh!

nik9000 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

romseygeek commented Oct 21, 2020

Uh oh!

romseygeek commented Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants