Skip to content

Add index-time scripts to geo_point field mapper #71861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 20, 2021

Conversation

romseygeek
Copy link
Contributor

This commit adds the ability to define an index-time geo_point field
with a script parameter, allowing you to calculate points from other
values within the indexed document.

@romseygeek romseygeek added >feature :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.13.0 labels Apr 19, 2021
@romseygeek romseygeek requested review from javanna and iverase April 19, 2021 16:14
@romseygeek romseygeek self-assigned this Apr 19, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Apr 19, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two small comments, and I think that we also need to update the docs for geo_point field. I defer to @iverase for the final LGTM ;)

});
if (hasScript) {
throw new MapperParsingException("failed to parse field [" + fieldType().name() + "] of type + " + contentType() + "]",
new IllegalArgumentException("Cannot index data directly into a field with a [script] parameter"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grr I forgot about the mappers that override parse when I added this to the base class. Is it worth extracting the check to a common method so that it can be called from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to rework parse/parseCreateField anyway, as it's very confusing that we have these two methods with the same signature that do almost exactly the same thing. Can we consider this a band-aid until we tidy up in a followup?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, anyways the test is unified and tests the error message, so we ensure that it is always the same one

return new GeoPointFieldMapper(name, ft, multiFieldsBuilder.build(this, contentPath),
copyTo.build(), geoParser, this);
}
return new GeoPointFieldMapper(name, ft, geoParser, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to use two different constructors with or without the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class hierarchy of the geo mappers makes things complicated here, but I can change this so that there's just a single constructor at the expense of adding extra params to all the other class constructors if you think it's less confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion and I cannot judge if it makes things better. Your call ;)

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The change make sense to me. I agree with @javanna that we need to update docs.

@romseygeek
Copy link
Contributor Author

we also need to update the docs for geo_point field

I knew I'd forgotten something in the initial commit :). Thanks for the reviews!

@romseygeek romseygeek merged commit ee3510b into elastic:master Apr 20, 2021
@romseygeek romseygeek deleted the mapper/geo-point-index-scripts branch April 20, 2021 09:24
romseygeek added a commit that referenced this pull request Apr 20, 2021
This commit adds the ability to define an index-time geo_point field
with a script parameter, allowing you to calculate points from other
values within the indexed document.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants