Make document parsing aware of runtime fields#65210
Conversation
Runtime fields are defined in a separate runtime section in the mappings. Since the runtime section was introduced, runtime fields are not taken into account when parsing documents. That means that if a document gets indexed that holds a field that's already defined as a runtime field, the field gets dynamically mapped as a concrete field although it will always be shadowed by the runtime field defined with the same name. A more sensible default would be to instead consider runtime fields like ordinary mapped fields, so a dynamic update is not necessary whenever a field is defined as part of the runtime section. As a consequence, the field does not get indexed. If users prefer to keep indexing the field although it is shadowed, we consider this an exception, and they can do so by mapping the field under properties explicitly. Relates to elastic#62906
|
Pinging @elastic/es-search (Team:Search) |
| return null; | ||
| } | ||
|
|
||
| private static class NoOpFieldMapper extends FieldMapper { |
There was a problem hiding this comment.
It is quite a shame to have to define all these methods (not all of them are abstract but I wanted to make sure that none of them is called), as only two are effectively needed: parseCreateField and copyTo.
There was a problem hiding this comment.
It is, but 🤷. I think there are worse things.
There was a problem hiding this comment.
Yeah it would be nice to somehow build an interface here but I can't see how that would easily work with ObjectMappers. For the future.
nik9000
left a comment
There was a problem hiding this comment.
Left some initial stuff. I imagine there are a ton of other tests worth adding but I haven't thought them through so I don't know what they are. And, you know, maybe we have enough tests and I'm wrong.
| .startObject("properties") | ||
| .startObject("field").field("type", "keyword").endObject() | ||
| .endObject() | ||
| .endObject().endObject(); |
There was a problem hiding this comment.
When we hit this file with the formatter one day it'll make this look like garbage. Would you be ok running the formatter on this now and playing with the output some to make it look ok when the formatter runs?
There was a problem hiding this comment.
You can do something like:
createDocumentMapper(topMapping(b -> {
b.startObject("runtime");
{
b.startObject("field").field("type", "test").endObject();
}
b.endObject();
b.startObject("properties");
{
b.startObject("field").field("type", "keyword").endObject();
}
b.endObject();
});
Keeps the formatter happy and there's a bit less ceremony around builders.
| return null; | ||
| } | ||
|
|
||
| private static class NoOpFieldMapper extends FieldMapper { |
There was a problem hiding this comment.
It is, but 🤷. I think there are worse things.
| .startObject("path1.path2.path3.field").field("type", "test").endObject() | ||
| .endObject() | ||
| .startObject("properties") | ||
| .startObject("path1").field("type", "object").field("enabled", false).endObject() |
There was a problem hiding this comment.
What happens if enabled isn't set? I think we should continue to do nothing if enabled is actually true.
There was a problem hiding this comment.
look at the test below for dynamic mappings. We map the objects but not the leaves. I agree it is debatable. The point is the objects get mapped the first time they are seen regardless of what they hold. For instance if you index a doc with an empty object, the object still gets mapped, which makes me think that it is the correct behaviour to map objects under properties as they may hold concrete fields in the future.
On the other hand, when we introduce the dynamic:runtime mode, given that everything is runtime we may not want to create objects under properties at all.
| @Override | ||
| public Builder getMergeBuilder() { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
Is this going to cause problems if there is a dynamic mapper added elsewhere? Let's say we send a document with two fields, one of which gets mapped by the dynamic template as a keyword, and the other as a runtime field. In that case, there will be a call back to the master with the new Mapping which will contain the new dynamic mapper, plus this NoOpFieldMapper, and it will blow up when we try to serialize it I think?
There was a problem hiding this comment.
I don't think so because the purpose of the no-op mapper at the moment is only to be used when parsing a document. It is not added to the dynamic mappers. Actually, its purpose is solely not to cause a dynamic mapping update. Keep me honest though :)
There was a problem hiding this comment.
Gotcha, I had misunderstood where we were creating these.
romseygeek
left a comment
There was a problem hiding this comment.
LGTM. I left one suggestion around test formatting.
| return null; | ||
| } | ||
|
|
||
| private static class NoOpFieldMapper extends FieldMapper { |
There was a problem hiding this comment.
Yeah it would be nice to somehow build an interface here but I can't see how that would easily work with ObjectMappers. For the future.
| @Override | ||
| public Builder getMergeBuilder() { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
Gotcha, I had misunderstood where we were creating these.
| .startObject("properties") | ||
| .startObject("field").field("type", "keyword").endObject() | ||
| .endObject() | ||
| .endObject().endObject(); |
There was a problem hiding this comment.
You can do something like:
createDocumentMapper(topMapping(b -> {
b.startObject("runtime");
{
b.startObject("field").field("type", "test").endObject();
}
b.endObject();
b.startObject("properties");
{
b.startObject("field").field("type", "keyword").endObject();
}
b.endObject();
});
Keeps the formatter happy and there's a bit less ceremony around builders.
|
run elasticsearch-ci/2 |
|
run elasticsearch-ci/packaging-sample-windows |
Runtime fields are defined in a separate runtime section in the mappings. Since the runtime section was introduced, runtime fields are not taken into account when parsing documents. That means that if a document gets indexed that holds a field that's already defined as a runtime field, the field gets dynamically mapped as a concrete field although it will always be shadowed by the runtime field defined with the same name. A more sensible default would be to instead consider runtime fields like ordinary mapped fields, so a dynamic update is not necessary whenever a field is defined as part of the runtime section. As a consequence, the field does not get indexed. If users prefer to keep indexing the field although it is shadowed, we consider this an exception, and they can do so by mapping the field under properties explicitly. Relates to elastic#62906
Runtime fields are defined in a separate runtime section in the mappings. Since the runtime section was introduced, runtime fields are not taken into account when parsing documents. That means that if a document gets indexed that holds a field that's already defined as a runtime field, the field gets dynamically mapped as a concrete field although it will always be shadowed by the runtime field defined with the same name. A more sensible default would be to instead consider runtime fields like ordinary mapped fields, so a dynamic update is not necessary whenever a field is defined as part of the runtime section. As a consequence, the field does not get indexed. If users prefer to keep indexing the field although it is shadowed, we consider this an exception, and they can do so by mapping the field under properties explicitly. Relates to #62906
Runtime fields are defined in a separate runtime section in the mappings. Since the runtime section was introduced, runtime fields are not taken into account when parsing documents. That means that if a document gets indexed that holds a field that's already defined as a runtime field, the field gets dynamically mapped as a concrete field although it will always be shadowed by the runtime field defined with the same name.
A more sensible default would be to instead consider runtime fields like ordinary mapped fields, so a dynamic update is not necessary whenever a field is defined as part of the runtime section. As a consequence, the field does not get indexed. If users prefer to keep indexing the field although it is shadowed, we consider this an exception, and they can do so by mapping the field under properties explicitly.
Relates to #62906