Rework geo mappers to index value by value #71696

romseygeek · 2021-04-14T14:52:37Z

The various geo field mappers are organised in a hierarchy that shares
parsing and indexing code. This ends up over-complicating things,
particularly when we have some mappers that accept multiple values
and others that only accept singletons. It also leads to confusing
behaviour around ignore_malformed behaviour: geo fields will ignore
all values if a single one is badly formed, while all other field mappers
will only ignore the problem value and index the rest. Finally, this
structure makes adding index-time scripts to geo_point needlessly
complex.

This commit refactors the indexing logic of the hierarchy to move the
individual value indexing logic into the concrete implementations,
and aligns the ignore_malformed behaviour with that of other mappers.

It contains two breaking changes:

the geo field mappers no longer check for external field values on the
parse context. This added considerable complication to the refactored
parse methods, and is unused anywhere in our codebase, but may
impact plugin-based field mappers which expect to use geo fields
as multifields
the geo_point field mapper now passes geohashes to its multifields
one-by-one, instead of formatting them into a comma-delimited
string and passing them all at once. Completion multifields using
this as an input should still behave as normal because by default
they would split this combined geohash string on the commas in any
case, but keyword subfields may look different.

Fixes #69601

elasticmachine · 2021-04-14T14:52:41Z

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

romseygeek

geo_point is the only mapper that actually parses multifields (passing a geohash String down as an external value) and all of the other mappers just silently ignore any defined multifields. As a followup we should deprecate adding multifields to those mappers that do not support it, and throw an error in v8.

romseygeek · 2021-04-14T14:57:03Z

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

        }
+        // TODO phase out geohash (which is currently used in the CompletionSuggester)
+        multiFields.parse(this, context.createExternalValueContext(geometry.geohash()));


Note: this changes the way that multiple values are sent to sub fields, in that we now call multifields.parse() once for each value, instead of combining them all together. This should not cause any change in behaviour for completion fields because the completion field mapper by default uses an analyzer that splits the combined values up; however, it will change the output of keyword subfields. The new behaviour is in line with what all other field mappers do so I think it's worth the change, but we should document it.

I also think this new behavior is more intuitive. It seemed weird before that we would pass the string [geohash1, geohash2, ...] to subfields. Could we add a quick test to document/ solidify the new behavior?

I've added a test to GeoPointFieldMapperTests

Thanks! This could also be considered a breaking change, I agree we should document it.

romseygeek · 2021-04-14T15:01:00Z

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

@@ -35,12 +33,14 @@ public void testExternalValues() throws Exception {
        assertThat(doc.rootDoc().getField("field.bool"), notNullValue());
        assertThat(doc.rootDoc().getField("field.bool").stringValue(), is("T"));

+        /*


The ExternalFieldMapper tests that values set by a parent field mapper are propagated down to its multifields for a few field types, including a point and shape mapper. However, no other mapper sets points or shapes as an external field value, so this test was the only place that was using this logic for geo fields. Given that it is never used in production code and adds a fair amount of complexity to the indexing logic, I've removed it and commented out the tests.

I also think this logic is okay to remove. Like @romseygeek, I couldn't find any user-facing behavior this would change. I guess plugin authors could have been relying on this, but it seems very unlikely. In general, we'd like to remove support for external value parsing (#56063), and this is in line with that goal.

One note: could we just remove these test checks? There's not a strong reason to leave the logic here if we don't plan to restore it.

Still this is a breaking change so maybe we need to document it accordingly?

I've marked it as >breaking and will add notes to the migration docs in the backport PR

jtibshirani

I'm not deeply familiar with the geo mapping code, but I looked over the the changes we discussed around external value parsing and they make sense to me.

jtibshirani · 2021-04-14T21:46:04Z

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


        /**
         * Parses the given value, then formats it according to the 'format' string.
         *
         * Used by value fetchers to validate and format geo objects
         */
        public Object parseAndFormatObject(Object value, String format) {
-            Parsed geometry;
+            Object[] formatted = new Object[1];


Maybe it'd be more straightforward to use SetOnce instead of an array.

jtibshirani · 2021-04-14T21:51:20Z

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

@@ -1068,14 +1068,14 @@ public void testWithDynamicTemplates() throws Exception {
        ParsedDocument doc = mapper.parse(source("1", b -> b.field(field, "41.12,-71.34"), null, Map.of(field, "points")));
        IndexableField[] fields = doc.rootDoc().getFields(field);
        assertThat(fields, arrayWithSize(2));
-        assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE));
-        assertThat(fields[1].fieldType(), sameInstance(LatLonPoint.TYPE));
+        assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE));


Small comment, probably clearest to keep the order where we check field[0] then field[1]?

++, I've swapped these around so that they're in order

jtibshirani · 2021-04-14T21:58:48Z

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

@@ -35,12 +33,14 @@ public void testExternalValues() throws Exception {
        assertThat(doc.rootDoc().getField("field.bool"), notNullValue());
        assertThat(doc.rootDoc().getField("field.bool").stringValue(), is("T"));

+        /*


I also think this logic is okay to remove. Like @romseygeek, I couldn't find any user-facing behavior this would change. I guess plugin authors could have been relying on this, but it seems very unlikely. In general, we'd like to remove support for external value parsing (#56063), and this is in line with that goal.

One note: could we just remove these test checks? There's not a strong reason to leave the logic here if we don't plan to restore it.

jtibshirani · 2021-04-14T22:32:19Z

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

        }
+        // TODO phase out geohash (which is currently used in the CompletionSuggester)
+        multiFields.parse(this, context.createExternalValueContext(geometry.geohash()));


I also think this new behavior is more intuitive. It seemed weird before that we would pass the string [geohash1, geohash2, ...] to subfields. Could we add a quick test to document/ solidify the new behavior?

iverase · 2021-04-15T08:21:21Z

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

-            }
-            context.addIgnoredField(mappedFieldType.name());
-        }
+    public final void parse(ParseContext context) throws IOException {


Maybe we should remove the comment on this method as there is not parsing logic and add documentation ton the new index() method.

jtibshirani

This makes sense to me! I'll defer to @iverase for the final approval since I'm not an expert on this code.

One suggestion: I think it'd help to describe the two breaking changes in a PR comment (or in the description) -- this makes it much easier to track down for release notes.

romseygeek · 2021-04-19T07:54:13Z

Thanks @jtibshirani. I've updated the PR description to include the two breaking changes.

iverase

lgtm, I think this changes make easier to understand the mappers. Thanks!

The various geo field mappers are organised in a hierarchy that shares parsing and indexing code. This ends up over-complicating things, particularly when we have some mappers that accept multiple values and others that only accept singletons. It also leads to confusing behaviour around ignore_malformed behaviour: geo fields will ignore all values if a single one is badly formed, while all other field mappers will only ignore the problem value and index the rest. Finally, this structure makes adding index-time scripts to geo_point needlessly complex. This commit refactors the indexing logic of the hierarchy to move the individual value indexing logic into the concrete implementations, and aligns the ignore_malformed behaviour with that of other mappers. It contains two breaking changes: * The geo field mappers no longer check for external field values on the parse context. This added considerable complication to the refactored parse methods, and is unused anywhere in our codebase, but may impact plugin-based field mappers which expect to use geo fields as multifields * The geo_point field mapper now passes geohashes to its multifields one-by-one, instead of formatting them into a comma-delimited string and passing them all at once. Completion multifields using this as an input should still behave as normal because by default they would split this combined geohash string on the commas in any case, but keyword subfields may look different. Fixes elastic#69601

The various geo field mappers are organised in a hierarchy that shares parsing and indexing code. This ends up over-complicating things, particularly when we have some mappers that accept multiple values and others that only accept singletons. It also leads to confusing behaviour around ignore_malformed behaviour: geo fields will ignore all values if a single one is badly formed, while all other field mappers will only ignore the problem value and index the rest. Finally, this structure makes adding index-time scripts to geo_point needlessly complex. This commit refactors the indexing logic of the hierarchy to move the individual value indexing logic into the concrete implementations, and aligns the ignore_malformed behaviour with that of other mappers. It contains two breaking changes: * The geo field mappers no longer check for external field values on the parse context. This added considerable complication to the refactored parse methods, and is unused anywhere in our codebase, but may impact plugin-based field mappers which expect to use geo fields as multifields * The geo_point field mapper now passes geohashes to its multifields one-by-one, instead of formatting them into a comma-delimited string and passing them all at once. Completion multifields using this as an input should still behave as normal because by default they would split this combined geohash string on the commas in any case, but keyword subfields may look different. Fixes #69601

#71696 introduced a regression to the various shape field mappers, where they would no longer handle null values. This commit fixes that regression and adds a testNullValues method to MapperTestCase to ensure that all field mappers correctly handle nulls. Fixes #71874

elastic#71696 introduced a regression to the various shape field mappers, where they would no longer handle null values. This commit fixes that regression and adds a testNullValues method to MapperTestCase to ensure that all field mappers correctly handle nulls. Fixes elastic#71874

#71696 introduced a regression to the various shape field mappers, where they would no longer handle null values. This commit fixes that regression and adds a testNullValues method to MapperTestCase to ensure that all field mappers correctly handle nulls. Fixes #71874

Rework geo mappers to index value by value

28552b7

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.13.0 labels Apr 14, 2021

romseygeek requested review from jtibshirani and iverase April 14, 2021 14:52

romseygeek self-assigned this Apr 14, 2021

elasticmachine added the Team:Search Meta label for search team label Apr 14, 2021

romseygeek commented Apr 14, 2021

View reviewed changes

jtibshirani reviewed Apr 14, 2021

View reviewed changes

iverase reviewed Apr 15, 2021

View reviewed changes

romseygeek added 3 commits April 15, 2021 13:02

tests; feedback

6b6ad29

unlock test

f2cdb81

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

7415732

romseygeek added the >breaking label Apr 15, 2021

romseygeek added 3 commits April 15, 2021 13:48

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

f45a756

tests

aae6f69

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

7239f79

jtibshirani reviewed Apr 15, 2021

View reviewed changes

romseygeek requested a review from iverase April 19, 2021 07:54

iverase approved these changes Apr 19, 2021

View reviewed changes

romseygeek merged commit 289d202 into elastic:master Apr 19, 2021

romseygeek deleted the mapper/geo-indexing branch April 19, 2021 11:38

wwang500 mentioned this pull request Apr 19, 2021

[Transform] geo_bounds aggregation failed with NPE error #71874

Closed

romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Apr 20, 2021

Add breaking changes note for elastic#71696

7147be1

romseygeek added a commit that referenced this pull request Apr 20, 2021

Add breaking changes note for #71696 (#71905)

afcb3a1

romseygeek mentioned this pull request Apr 21, 2021

Add null-field checks to shape field mappers #71999

Merged

romseygeek mentioned this pull request Apr 21, 2021

Add null-field checks to shape field mappers (#71999) #72025

Merged

This was referenced Jun 7, 2021

GeoShapeIndexer#prepareForIndex should be called for computing the doc value centroid #73856

Merged

GeoShapeIndexer#prepareForIndex should be called for computing the doc value centroid (#73856) #73875

Merged

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

Rework geo mappers to index value by value #71696

Rework geo mappers to index value by value #71696

Uh oh!

Conversation

romseygeek commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 14, 2021

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!

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!

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

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!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Apr 19, 2021

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romseygeek commented Apr 14, 2021 •

edited

Loading