-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Lock down _id
field
#9842
Lock down _id
field
#9842
Conversation
@@ -122,12 +122,15 @@ public IdFieldMapper build(BuilderContext context) { | |||
@Override | |||
public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException { | |||
IdFieldMapper.Builder builder = id(); | |||
if (parserContext.indexVersionCreated().onOrAfter(Version.V_2_0_0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it move one line up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good point, will move it up.
LGTM |
Maybe it should have an entry in the migration guide? |
LGTM^2 |
the reason the percolator was using the _id field was that the type part of the uid wouldn't be needed to be loaded in memory via field data and to prevent the splitting of the type and id at percolate time. I'm less concerned about the type being loaded in memory, since at some point _uid will be doc values by default. But I'm not a big fan of the type/id splitting. Maybe the percolator should have its own field that can be used for id lookup? But this can potentially be explored in a different issue. But LGTM and worry about that later. |
There are already at least two reasons why we would need doc values on
So maybe we should have doc values on _uid (at least by default, maybe enforced). I think this would also fix this percolator concern and it would be fine to work with the _uid instead of _id? |
There are two implications to this change. First, percolator now uses _uid internally, extracting the id portion when needed. Second, sorting on _id is no longer possible, since you can no longer index _id. However, _uid can still be used to sort, and is better anyways as indexing _id just to make it available to fielddata for sorting is wasteful. see elastic#8143 closes elastic#9842
@jpountz yes that makes sense and fix the concern. I lean towards enforcing doc values on the _uid field. |
@rjernst labels please? |
There are two implications to this change.
First, percolator now uses _uid internally, extracting the id portion
when needed. Second, sorting on _id is no longer possible, since you
can no longer index _id. However, _uid can still be used to sort, and
is better anyways as indexing _id just to make it available to
fielddata for sorting is wasteful.
see #8143